WebSockets uses zlib deflate with windowBits = 8 and no zlib/gzip wrapper |
||||
Issue descriptionI discovered this while trying to update zlib to 1.2.11: https://codereview.chromium.org/2691623002 Several WebSocket tests are failing due to this change in zlib: https://github.com/madler/zlib/commit/049578f0a1849f502834167e233f4c1d52ddcbcc Logs and additional information are below the fold. net/websockets provides net::WebSocketDeflater::Initialize() as an interface that allows the caller to choose a value for window_bits. It accepts values in the range [8, 15]. When calling zlib’s deflateInit2, it negates window_bits to use “raw” deflate (that is, to compress to a raw stream not wrapped in a zlib or gzip wrapper). The referenced zlib commit states: > There is a bug in deflate for windowBits == 8 (256-byte window). > As a result, zlib silently changes a request for 8 to a request > for 9 (512-byte window), and sets the zlib header accordingly so > that the decompressor knows to use a 512-byte window. However if > deflateInit2() is used for raw deflate or gzip streams, then there > is no indication that the request was not honored, and the > application might assume that it can use a 256-byte window when > decompressing. This commit returns an error if the user requests > a 256-byte window when using raw deflate or gzip encoding. Previously, all of the requests coming through this interface that specified window_bits = 8 (a 256-byte window) were silently being bumped up to window_bits = 9 (a 512-byte window, see line 259 in the referenced zlib commit). As the commit message states, this could have caused a problem for decompressors that assumed that they could use window_bits = 8. Because of this problem, zlib was updated in 1.2.9 to return Z_STREAM_ERROR when a request to compress to a raw deflate stream with windowBits = 8 is received. Put another way, all of our attempts to compress with window_bits = 8 have always been silently “upgraded” to window_bits = 9, but now we need to specify window_bits = 9 to obtain this behavior. To retain existing behavior through the zlib upgrade, it’s possible to force window_bits to 9 at a minimum when deflating (compressing). To be able to properly inflate (decompress) raw deflate streams that claim to have been compressed with a window size of 256 but that have been “upgraded” to a window size of 512 without warning by pre-1.2.9 zlib, it is also necessary to force window_bits to 9 at a minimum during the inflate operation. I don’t know much about WebSockets or its use of zlib. Please chime in. It looks like Yutaka added this support, but I’m also Ccing other OWNERS of net/websockets. -- The failing tests are in net_unittests: WebSocketDeflateStreamWithClientWindowBitsTest.WindowBits8 WebSocketDeflaterTest.WindowBits8 WebSocketInflaterTest.LargeRandomDeflateInflate These webkit_tests are also failing: http/tests/websocket/permessage-deflate-parameter.html http/tests/websocket/permessage-deflate-window-bits.html virtual/mojo-loading/http/tests/websocket/permessage-deflate-parameter.html virtual/mojo-loading/http/tests/websocket/permessage-deflate-window-bits.html Here’s a try log from linux_chromium_rel_ng: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/387978 [ RUN ] WebSocketDeflateStreamWithClientWindowBitsTest.WindowBits8 Received signal 11 SEGV_MAPERR 000000000000 #0 0x000002df1247 base::debug::StackTrace::StackTrace() #1 0x000002df0dcf base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7f1fb7df5cb0 <unknown> #3 0x000002d67f33 net::WebSocketDeflater::AddBytes() #4 0x000002d664c2 net::WebSocketDeflateStream::Deflate() #5 0x000002d66043 net::WebSocketDeflateStream::WriteFrames() #6 0x0000020e3126 net::(anonymous namespace)::WebSocketDeflateStreamWithClientWindowBitsTest_WindowBits8_Test::TestBody() […] [ RUN ] WebSocketDeflaterTest.WindowBits8 Received signal 11 SEGV_MAPERR 000000000000 #0 0x000002df1247 base::debug::StackTrace::StackTrace() #1 0x000002df0dcf base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7f335bccfcb0 <unknown> #3 0x000002d67f33 net::WebSocketDeflater::AddBytes() #4 0x0000020e7a38 net::(anonymous namespace)::WebSocketDeflaterTest_WindowBits8_Test::TestBody() […] [ RUN ] WebSocketInflaterTest.LargeRandomDeflateInflate ../../net/websockets/websocket_inflater_test.cc:192: Failure Value of: deflater.Initialize(8) Actual: false Expected: true [ FAILED ] WebSocketInflaterTest.LargeRandomDeflateInflate (0 ms) The four WebKit tests all crash as well.
,
Feb 10 2017
,
Feb 10 2017
https://codereview.chromium.org/2690623003 implements the existing behavior, which upgrades windowBits = 8 to 9 during compression. It also does a similar upgrade during decompression, on the theory that existing zlib deflate compressors have silently upgraded 8 to 9 during compression, and the stream to decompress may in fact use a larger window than specified. I would also like to have Chrome never inform anyone else that it’s using windowBits = 8, but I’m not yet sure where in net/websockets (or elsewhere) we can do that.
,
Feb 13 2017
Ouch. It's never worked. Because we never had an independent implementation of DEFLATE to test against, we never noticed. That's disappointing. We have three terrible choices: 1. Continue to treat 8 as 9 as implemented in Mark's CL. 2. Stop advertising client_max_window_bits at all. 3. Abort the connection if the server requests client_max_window_bits=8. We also have three non-terrible choices that are probably an unjustifiable amount of work: 4. Actually fix zlib so that windowsBits = 8 works. 5. Update RFC7692 to change the allowed range of window_bits to [9, 15]. 6. Create a from-scratch DEFLATE implementation. My preference is to do 1. in the short term, and add UMA to figure out what the next step should be. tyoshino@'s research indicated that values of 8 and 9 gave very poor compression, but we don't know whether anyone has adopted them.
,
Feb 13 2017
I’ve done a bit more light reading over the weekend, and I believe that zlib-compressed streams actually reduce the effective sliding window from the 2^windowBits bytes that you’d expect to 2^windowBits - MIN_LOOKAHEAD bytes, where MIN_LOOKAHEAD is 262. So for windowBits = 9 (upgraded from 8), the window is 250 bytes, which actually is small enough to fit in the 256-byte window that a compressor using windowBits = 8 would have. See “Compression Factor Design Quirk” at http://www.zlib.net/zlib_tech.html, and of course, the zlib source. I haven’t done any tests yet, but I believe that this is the case. “Never worked,” while technically true, is also saved by the fact that this zlib quirk means compressing with windowBits = 9 can be safely decompressed with windowBits = 8. Note that this isn’t true for streams compressed with any windowBits > 9, but nothing was ever silently “upgraded” to a window that large. So on this basis, I think that terrible choice (1) isn’t actually that bad. I’d like to continue to “upgrade” 8 to 9 during compression on our end, provided that we’re sure we’re talking to zlib and that it has this quirk. I’d also like to drop the “upgrade” from 8 to 9 on the decompressor side, because it shouldn’t be necessary. The upgrade does concern me. Even though it’s not so silent now, it’ll soon be forgotten after we move it from zlib’s side into our side and close the bug. We should add a test with carefully-chosen data that would actually use the “wrong” half of a real 512-byte window if compressed by a library (or future zlib version) without this limitation, and ensure that we can round-trip through a compression-decompression cycle with 8-upgrade-9 on the compress side and just 8 on the decompress side.
,
Feb 13 2017
P.S. This quirk is, as I understand it, also related to the reason for zlib’s compressor won’t work with windowBits = 8: 2^windowBits - MIN_LOOKAHEAD = -6, and negative anything doesn’t make for a great sliding window. It’d also somewhat explain the tyoshino’s disappointing results: window sizes would have been 250, 250, 762 for windowBits 8, 9, 10 compressing with zlib, where you’d probably have expected 256, 512, 1024.
,
Feb 13 2017
That's good news. If I understand correctly, rather than being "we are out-of-spec when client_max_window_bits is 8" it means "we are very suboptimal when client_max_windows_bits is 8 or 9" which is much easier to live with. Hand-constructing DEFLATE streams is painful, but tyoshino@ is good at that sort of thing. I'd be tempted to just modify //net/websockets/websocket_deflate_stream_fuzzer.cc to vary client_max_window_bits and declare victory. I agree with your proposal to continue to "upgrade" 8 to 9 for compression but not for decompression. If you want to change the CL to do that I can LGTM it to unblock you.
,
Feb 13 2017
How about the inflater? Does it have a similar constant (i.e., MIN_LOOKAHEAD)? https://tools.ietf.org/html/rfc7692#section-7.1.2.1 Absence of this parameter in an extension negotiation offer indicates that the client can receive messages compressed using an LZ77 sliding window of up to 32,768 bytes. Chrome doesn't include server_max_window_bits (see [1]), but can we actually receive a message compressed using a sliding window of full 32768 byes? 1: https://cs.chromium.org/chromium/src/net/websockets/websocket_handshake_stream_create_helper.cc?q=websocket_handshake_stream+package:%5Echromium$&l=38
,
Feb 13 2017
ricea #7: I updated https://codereview.chromium.org/2690623003 to only “upgrade” window_bits = 8 to 9 during deflate, revised the comment to better express what’s happening and why it’s safe, and dropped the inflate side altogether. yhirano #8: The inflater doesn’t have any “negative slop” in its window. With window_bits = 8, a full 256-byte window is usable (, 9/512, 10/1024, etc., 15/32k). We should have no trouble handling this, although if we add a test like the type I proposed in #5, it’d also be prudent to add one to ensure that we can decompress a maximal-distance (32k) back-reference (at window_bits = 15), even if our compressor isn’t able to generate one on its own. Note that the deflater isn’t out of spec. Nothing requires it to use the full 512-byte window that window_bits = 9 implies. In fact, the spec calls this out (also RFC 1951 §3.3): > A compressor may limit further the ranges of values specified in > the previous section and still be compliant; for example, it may > limit the range of backward pointers to some value smaller than > 32K.
,
Feb 13 2017
https://codereview.chromium.org/2690623003/
,
Feb 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d9a6251dfe87183075dc16cfa134e41dc4cee0d commit 6d9a6251dfe87183075dc16cfa134e41dc4cee0d Author: mark <mark@chromium.org> Date: Wed Feb 15 06:15:29 2017 Update zlib to 1.2.11 Reapply and regenerate all local patches to upstream zlib 1.2.11 Explicitly specify 9 as the minimum windowBits value (representing a 512-byte window) during compression in net/websockets even when 8 (representing 256) is received. This was previously silently done during compression. Because of how zlib's deflate is implemented, when windowBits is 9, it will produce a stream that can be decompressed with a 250-byte or larger window. Changes in 1.2.9 (31 Dec 2016) - Fix contrib/minizip to permit unzipping with desktop API [Zouzou] - Improve contrib/blast to return unused bytes - Assure that gzoffset() is correct when appending - Improve compress() and uncompress() to support large lengths - Fix bug in test/example.c where error code not saved - Remedy Coverity warning [Randers-Pehrson] - Improve speed of gzprintf() in transparent mode - Fix inflateInit2() bug when windowBits is 16 or 32 - Change DEBUG macro to ZLIB_DEBUG - Avoid uninitialized access by gzclose_w() - Allow building zlib outside of the source directory - Fix bug that accepted invalid zlib header when windowBits is zero - Fix gzseek() problem on MinGW due to buggy _lseeki64 there - Loop on write() calls in gzwrite.c in case of non-blocking I/O - Add --warn (-w) option to ./configure for more compiler warnings - Reject a window size of 256 bytes if not using the zlib wrapper - Fix bug when level 0 used with Z_HUFFMAN or Z_RLE - Add --debug (-d) option to ./configure to define ZLIB_DEBUG - Fix bugs in creating a very large gzip header - Add uncompress2() function, which returns the input size used - Assure that deflateParams() will not switch functions mid-block - Dramatically speed up deflation for level 0 (storing) - Add gzfread(), duplicating the interface of fread() - Add gzfwrite(), duplicating the interface of fwrite() - Add deflateGetDictionary() function - Use snprintf() for later versions of Microsoft C - Fix *Init macros to use z_ prefix when requested - Replace as400 with os400 for OS/400 support [Monnerat] - Add crc32_z() and adler32_z() functions with size_t lengths - Update Visual Studio project files [AraHaan] Changes in 1.2.10 (2 Jan 2017) - Avoid warnings on snprintf() return value - Fix bug in deflate_stored() for zero-length input - Fix bug in gzwrite.c that produced corrupt gzip files - Remove files to be installed before copying them in Makefile.in - Add warnings when compiling with assembler code Changes in 1.2.11 (15 Jan 2017) - Fix deflate stored bug when pulling last block from window - Permit immediate deflateParams changes before any deflate input BUG= 691074 , 691075 Review-Url: https://codereview.chromium.org/2690623003 Cr-Commit-Position: refs/heads/master@{#450585} [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/net/websockets/websocket_deflater.cc [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/BUILD.gn [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/LICENSE [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/README.chromium [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/adler32.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/compress.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/contrib/minizip/iowin32.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/contrib/minizip/unzip.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/contrib/minizip/zip.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/crc32.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/deflate.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/deflate.h [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/google.patch [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/gzguts.h [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/gzlib.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/gzread.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/gzwrite.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/infback.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/inffast.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/inflate.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/inflate.h [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/inftrees.c [delete] https://crrev.com/48cea90831e2f0d14d98bfc9d27d953fe3c1c1a5/third_party/zlib/mozzconf.h [add] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/names.h [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/simd.patch [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/simd_stub.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/trees.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/uncompr.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/x86.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/zconf.h [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/zlib.h [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/zutil.c [modify] https://crrev.com/6d9a6251dfe87183075dc16cfa134e41dc4cee0d/third_party/zlib/zutil.h
,
Feb 15 2017
Fixed as discussed in 6d9a6251dfe8. No new tests were added, although it would be a good idea to add the types of tests suggested in comment 5 and comment 9.
,
Feb 16 2017
Sorry for belated reply. The solution taken sounds good. Thank you mark@, ricea@ and yhirano@ for analysis and figuring out it. I'll send a heads up to IETF hybi@ mailing list. |
||||
►
Sign in to add a comment |
||||
Comment 1 by mark@chromium.org
, Feb 10 2017