Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451833004:
Fix net::BrotliSourceStream to ignore trailing data. (Closed)
DescriptionFix net::BrotliSourceStream to ignore trailing data.
When net::BrotliSourceStream is given a valid input but
with trailing data, BrotliSourceStream will consume valid
input but not the trailing data. This triggers a DCHECK in
FilterSourceStream which asserts that either all bytes
are consumed or bytes written is not 0.
This CL matches the original implementation
(net::BrotliFilter) more closely.
If there is any trailing data, the data will be ignored.
This CL adds a regression test which without the patch
will trigger the DCHECK.
BUG=659311
Committed: https://crrev.com/c4780eac0d48c8b57244a9cbcf8bfe7271b532f4
Cr-Commit-Position: refs/heads/master@{#427770}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added tests #Patch Set 3 : Added two extra tests #
Total comments: 3
Patch Set 4 : remove cast #
Total comments: 14
Patch Set 5 : Address comment and add EOF read #
Total comments: 2
Patch Set 6 : add const #Messages
Total messages: 36 (22 generated)
Description was changed from ========== Fix net::BrotliSourceStream to ignore trailing data. net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. BUG=659311 ========== to ========== Fix net::BrotliSourceStream to ignore trailing data. net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. This CL adds a regression test which without the patch will trigger the DCHECK. BUG=659311 ==========
xunjieli@chromium.org changed reviewers: + eustas@chromium.org
Description was changed from ========== Fix net::BrotliSourceStream to ignore trailing data. net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. This CL adds a regression test which without the patch will trigger the DCHECK. BUG=659311 ========== to ========== Fix net::BrotliSourceStream to ignore trailing data. When net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. This CL adds a regression test which without the patch will trigger the DCHECK. BUG=659311 ==========
eustas@: PTAL. Thanks! mmenke, rdsmith: FYI. https://codereview.chromium.org/2451833004/diff/1/net/filter/brotli_source_st... File net/filter/brotli_source_stream.cc (right): https://codereview.chromium.org/2451833004/diff/1/net/filter/brotli_source_st... net/filter/brotli_source_stream.cc:102: return ERR_CONTENT_DECODING_FAILED; The original code has these two if-conditionals. I somehow missed translating those to the new code.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2451833004/diff/1/net/filter/brotli_source_st... File net/filter/brotli_source_stream.cc (right): https://codereview.chromium.org/2451833004/diff/1/net/filter/brotli_source_st... net/filter/brotli_source_stream.cc:98: return OK; I think the test only covers one of these three new checks. SHould we have coverage of all 3?
SGTM! I will work on the tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm Looks good (since it does not allow developers append extra data after main stream).
Thanks for the review! PTAL. I added two more test cases. For the "decoding_status_ != DecodingStatus::DECODING_IN_PROGRESS" check, I can't find a test case that will fail without the check -- it seems Brotli is robust enough to handle failures twice in a row. Please see inline comments below for details. https://codereview.chromium.org/2451833004/diff/40001/net/filter/brotli_sourc... File net/filter/brotli_source_stream_unittest.cc (right): https://codereview.chromium.org/2451833004/diff/40001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:104: unsigned char response[] = {0x1A, (unsigned char)0xFFFFFFDF, 0x6E, 0x74, 0x74, This is the same as as the one provided by the fuzzer. Brotli consumes first byte and return 0 byte, but we need to mark all bytes as consumed. https://codereview.chromium.org/2451833004/diff/40001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:302: EXPECT_EQ(ERR_CONTENT_DECODING_FAILED, error); Note that removing the "decoding_status_ != DecodingStatus::DECODING_IN_PROGRESS" check, this test still passes. I think Brotli is robust enough that it returns ERR_CONTENT_DECODING_FAILED both times.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2451833004/diff/40001/net/filter/brotli_sourc... File net/filter/brotli_source_stream_unittest.cc (right): https://codereview.chromium.org/2451833004/diff/40001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:302: EXPECT_EQ(ERR_CONTENT_DECODING_FAILED, error); On 2016/10/26 15:14:38, xunjieli wrote: > Note that removing the "decoding_status_ != > DecodingStatus::DECODING_IN_PROGRESS" check, this test still passes. I think > Brotli is robust enough that it returns ERR_CONTENT_DECODING_FAILED both times. Hmm.. I guess the reason is that Brotli doesn't drain input bytes when returning error, so we still have the corrupted bytes when calling Read() for the second time, so decompress correctly gives the error again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... File net/filter/brotli_source_stream_unittest.cc (right): https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:102: // available bytes and return 0. Comment isn't very clear - why does it only consume 1 byte? A leading 0x1A means the body has 0 bytes of data? https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:104: unsigned char response[] = {0x1A, 0xDF, 0x6E, 0x74, 0x74, 0x68}; nit: const kResponse. https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:118: size_t extra_length = 1000; const kExtraLength, or inline it (Removing second reference, per below comment)? https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:121: encoded_len() + extra_length, OK, response_with_extra_data.length()? https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:144: size_t extra_length = 1000; const kExtraLength, or inline it (Removing second reference, per below comment)? https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:148: source()->AddReadResult(extra_data.c_str(), extra_length, OK, extra_data.length()? https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:164: EXPECT_EQ("BROTLI", brotli_stream()->Description()); Should we check that all of the MockSourceStream's data has been consumed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. thanks! https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... File net/filter/brotli_source_stream_unittest.cc (right): https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:102: // available bytes and return 0. On 2016/10/26 16:39:57, mmenke wrote: > Comment isn't very clear - why does it only consume 1 byte? A leading 0x1A > means the body has 0 bytes of data? I do not know. I tried to make sense of the specs at https://tools.ietf.org/html/rfc7932#page-31, but I didn't make much progress. I rephrase it slightly. https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:104: unsigned char response[] = {0x1A, 0xDF, 0x6E, 0x74, 0x74, 0x68}; On 2016/10/26 16:39:57, mmenke wrote: > nit: const kResponse. Done. https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:118: size_t extra_length = 1000; On 2016/10/26 16:39:57, mmenke wrote: > const kExtraLength, or inline it (Removing second reference, per below > comment)? Done. https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:121: encoded_len() + extra_length, OK, On 2016/10/26 16:39:57, mmenke wrote: > response_with_extra_data.length()? Done. https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:144: size_t extra_length = 1000; On 2016/10/26 16:39:57, mmenke wrote: > const kExtraLength, or inline it (Removing second reference, per below > comment)? Done. https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:148: source()->AddReadResult(extra_data.c_str(), extra_length, OK, On 2016/10/26 16:39:57, mmenke wrote: > extra_data.length()? Done. https://codereview.chromium.org/2451833004/diff/60001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:164: EXPECT_EQ("BROTLI", brotli_stream()->Description()); On 2016/10/26 16:39:57, mmenke wrote: > Should we check that all of the MockSourceStream's data has been consumed? It's checked in MockSourceStream's destructor.
LGTM https://codereview.chromium.org/2451833004/diff/80001/net/filter/brotli_sourc... File net/filter/brotli_source_stream_unittest.cc (right): https://codereview.chromium.org/2451833004/diff/80001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:105: unsigned char kResponse[] = {0x1A, 0xDF, 0x6E, 0x74, 0x74, 0x68}; +const (And add it to the reinterpret_casts, to avoid making the compiler sad)
Thanks! https://codereview.chromium.org/2451833004/diff/80001/net/filter/brotli_sourc... File net/filter/brotli_source_stream_unittest.cc (right): https://codereview.chromium.org/2451833004/diff/80001/net/filter/brotli_sourc... net/filter/brotli_source_stream_unittest.cc:105: unsigned char kResponse[] = {0x1A, 0xDF, 0x6E, 0x74, 0x74, 0x68}; On 2016/10/26 17:28:20, mmenke wrote: > +const (And add it to the reinterpret_casts, to avoid making the compiler sad) Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eustas@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2451833004/#ps100001 (title: "add const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix net::BrotliSourceStream to ignore trailing data. When net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. This CL adds a regression test which without the patch will trigger the DCHECK. BUG=659311 ========== to ========== Fix net::BrotliSourceStream to ignore trailing data. When net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. This CL adds a regression test which without the patch will trigger the DCHECK. BUG=659311 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix net::BrotliSourceStream to ignore trailing data. When net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. This CL adds a regression test which without the patch will trigger the DCHECK. BUG=659311 ========== to ========== Fix net::BrotliSourceStream to ignore trailing data. When net::BrotliSourceStream is given a valid input but with trailing data, BrotliSourceStream will consume valid input but not the trailing data. This triggers a DCHECK in FilterSourceStream which asserts that either all bytes are consumed or bytes written is not 0. This CL matches the original implementation (net::BrotliFilter) more closely. If there is any trailing data, the data will be ignored. This CL adds a regression test which without the patch will trigger the DCHECK. BUG=659311 Committed: https://crrev.com/c4780eac0d48c8b57244a9cbcf8bfe7271b532f4 Cr-Commit-Position: refs/heads/master@{#427770} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c4780eac0d48c8b57244a9cbcf8bfe7271b532f4 Cr-Commit-Position: refs/heads/master@{#427770} |
