Possible BUG in "deflate" algorythm
Reported by
egor.iva...@gmail.com,
Dec 26 2016
|
||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36 Example URL: Steps to reproduce the problem: Unfortunately it's quite hard to provide a working test case. I can just try and explain what I have found. I had a resource loaded in several chunks of data. First chunk contained all headers with "Content-Encoding:deflate", other two were "deflate encoded" body with one weirdness: the first chunk contained just 1 byte, the second - the rest. After some research I found that encoded body did not contain zlib headers so GzipSourceStream in GzipSourceStream::FilterData was trying to do inflate only for one byte and instead of getting en error it succeeded. Hence it was unable to do inflate for a second chunk of data. I've made a small test with zlib and discovered that it wont return error for just one byte in input buffer so code in GzipSourceStream wont be able to detect zlib headers absence. The correct sequence of calls should be like: 1) add zlib headers; 2) inflate first chunk with 1only one byte; 3) inflate the rest. I also found this behavior in previous versions of Chrome which used GZipFilter. I hope it will help you. What is the expected behavior? Successfully loaded resource, like: js or css file. What went wrong? I got the error: net::ERR_CONTENT_DECODING_FAILED Did this work before? No Chrome version: 57.0.2962.0 canary Channel: stable OS Version: 10.0 Flash Version: Shockwave Flash 24.0 r0
,
Dec 27 2016
,
Dec 28 2016
Note that GzipSourceStream is actually a new class, and isn't being used in Chrome 55, though it's possible both the old and completely refactored versions of the filter code had the same bug. Could you see if Chrome Canary has the same issue? You can safely install Chrome Canary alongside Chrome stable, and then uninstall it at your leisure, without losing settings on stable (That's not the case with other pre-release channels). If Chrome Canary has the issue, it would be great if you could provide us a net-internals log with byte logging enabled (Instructions: https://sites.google.com/a/chromium.org/dev/for-testers/providing-network-details - byte logging option is under the capture tab. Note that the log will include cookies and credentials sent to/received from sites visited while net-internals is open. If you use Canary, and don't log in to any sites with it, you shouldn't have anything to worry about).
,
Dec 29 2016
Actually, I've checked this error in my custom Chrome 51 build, Chrome 55 and Canary. All of them had a similar error. Sadly, I've found this thing using our client's private site so it could be impossible to provide whole net log due to security reasons. Here are two files with some data from chrome://net-internals from events menu. it's URL_REQUEST data, one from a broken attempt the other from a good one. I hope it will help.
,
Dec 29 2016
Thanks for that, should be all we need to try and repro. Random comment: Be careful of the use of "chunked" in reference to HTTP, I had assumed you meant chunked transfer-encoding, which, admittedly, shouldn't matter if this is indeed an issue at the URLRequestJob layer.
,
Dec 29 2016
Looks like I can repro. The issue looks to only affect the path where we're getting a deflate response without a deflate header, so if you just attach a deflate header (Assumed to be 0x78 0x01), you'll avoid the issue. Obviously we need to fix that (Though it would be nice if we could just force everyone to either send or not send a gzip header...), but it won't make it to stable for a couple months.
,
Dec 29 2016
Seems like deflate over HTTP is fundamentally flawed. We try decompressing without adding a deflate header, which works if there's already one in the stream. If that fails, we try deflating with adding a header. The problem is if we've already consumed data when we get the error (Or worse, already passed data on upstream), we aren't correcting adding the deflate header. The problem is that there doesn't seem to actually be any guarantee that the first bytes after the (Possibly not present) deflate header are not a valid deflate header, so however many bytes we require before continuing to process the response, there's no guarantee we have enough data. So any "fix" involving buffering data is really only a partially functional hack, unless we buffer the entire partial response in memory holding it back from the renderer until it's all received, which we're obviously not going to do. I'm going to arbitrarily buffer up to 100 bytes until we either get a decompression error, or start decoding bytes, but it doesn't seem like we can do better than best-effort here. You're much better off either sending the deflate header, or using gzip.
,
Dec 29 2016
Wow, I haven't thought about "chunks" in that meaning, sorry for that. Concerning this issue the bad thing is I'm not quite sure what leads to this strange fractured body. I think it could be my proxy server's fault. It happens only when using HTTPS over TLS 1.0 protocol. So for me I've find a temporary solution just to switch to TLS 1.2. Anyway, thanks for you help.
,
Dec 29 2016
Independent of that, you should be sending the 2-byte deflate header. Per above comment, decompressing a response without that header is best-effort at best, and it's not really a good idea to have a production service depend on best-effort behavior.
,
Dec 29 2016
I don't have access to the client's server where the response was originally encrypted so I can't add headers needed. All that I can do is to figure out why only one first byte was sent and fix it. Since it seems that it's a rare case and this issue was in Chrome for years I think it should be ok for me right now just to avoid sending such a bad byte sequence. At least it's all that I can do.
,
Jan 3 2017
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e66c81aea915be21bb56cb458f5efe8703aea78e commit e66c81aea915be21bb56cb458f5efe8703aea78e Author: mmenke <mmenke@chromium.org> Date: Tue Jan 03 20:07:58 2017 Fix bug in deflate code. For historical reasons, deflate-encoded streams may or may not have zlib headers. In the case zlib headers appear to be missing, we add the headers and then re-send the data we've received to zlib. If we discovered this problem on a read other than the first zlib body read, however, we would only re-send the result of the most recent read, rather than the entire body, resulting in an error. This CL adds some buffering of the body data until we're (somewhat) confident the original stream had a zlib header. Since there's no gaurantee that streams with and without zlib headers are actually distinguishable from each other, this doesn't resolve all potential issues, but should at least fix some cases. BUG= 677001 Review-Url: https://codereview.chromium.org/2604233002 Cr-Commit-Position: refs/heads/master@{#441197} [modify] https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e/net/filter/gzip_source_stream.cc [modify] https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e/net/filter/gzip_source_stream.h [modify] https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e/net/filter/gzip_source_stream_unittest.cc [modify] https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e/net/filter/mock_source_stream.cc [modify] https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e/net/filter/mock_source_stream.h
,
Jan 3 2017
Let's hope that sticks, but don't think we should spend too much time on making zlib-header-sniffing work better. |
||||
►
Sign in to add a comment |
||||
Comment 1 by kkaluri@chromium.org
, Dec 27 2016Labels: TE-NeedsTriageHelp