Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604233002:
Fix bug in deflate code. (Closed)
DescriptionFix 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
Committed: https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e
Cr-Commit-Position: refs/heads/master@{#441197}
Patch Set 1 #Patch Set 2 : Cleanups #
Total comments: 24
Patch Set 3 : Fix leak #
Total comments: 2
Patch Set 4 : Response to comments #Patch Set 5 : Missed one #
Total comments: 2
Patch Set 6 : Revert bypassing replay state #
Messages
Total messages: 25 (16 generated)
mmenke@chromium.org changed reviewers: + xunjieli@chromium.org
[xunjieli]: PTAL. While I'm not too enamored of this approach, other ways to handle mixing the replayed data with the new data seemed much worse, or I would be stuck with buffering just 1 or 2 bytes, which may actually be good enough, but still seems too limited, given the hackiness of the code. [rdsmith]: FYI. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:130: input_state_ = STATE_COMPRESSED_BODY; This used to be able to go through the add zlib header path, but that didn't make any sense, so just skip the sniff step now. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:177: if (!replay_data_.size()) { Worth noting while we do go through both branches in tests, in no test is the check false twice in a row, since replay_data_ is never longer than 1 byte. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:202: return result; No unit tests exercise this line, unfortunately. Testing it seemed like more effort than it was worth. https://codereview.chromium.org/2604233002/diff/20001/net/filter/mock_source_... File net/filter/mock_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/mock_source_... net/filter/mock_source_stream.cc:23: EXPECT_TRUE(results_.empty()); Generally, DCHECKs should check for test fixture bugs, EXPECTs for bugs in underlying code... The updated tests ran into this DCHECK until I fixed he bug, so thought it best to switch.
The CQ bit was checked by mmenke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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 mmenke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. Thanks for looking into the deflate hack. I agree that the recursive call into FilterData() in replay state isn't pretty, but I don't have see an alternative approach. The one you have is clear enough. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:30: const int kMaxZlibHeaderSniffBytes = 1000; Do you mean 100 instead of 1000? In the linked bug, you mentioned 100. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:130: input_state_ = STATE_COMPRESSED_BODY; On 2016/12/29 22:39:31, mmenke (Out Dec 17 to Jan 2) wrote: > This used to be able to go through the add zlib header path, but that didn't > make any sense, so just skip the sniff step now. Acknowledged. You are right. The old state change doesn't make sense in this case. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:152: continue; I believe this is equivalent to "break". If so, can we s/continue/break to be consistent with other switch cases? https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:176: case STATE_REPLAY_DATA: { nit: I am assuming this state only applies to deflate stream, if so maybe add a DCHECK_EQ(TYPE_DEFLATE, type()). https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:177: if (!replay_data_.size()) { On 2016/12/29 22:39:31, mmenke (Out Dec 17 to Jan 2) wrote: > Worth noting while we do go through both branches in tests, in no test is the > check false twice in a row, since replay_data_ is never longer than 1 byte. nit: Can we not enter STATE_REPLAY_DATA, if |replay_data_| is empty on line 196? maybe change "!replay_data_.size()" to "replay_data_.empty()"? I find the latter is easier to read. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:202: return result; On 2016/12/29 22:39:31, mmenke (Out Dec 17 to Jan 2) wrote: > No unit tests exercise this line, unfortunately. Testing it seemed like more > effort than it was worth. Acknowledged. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:203: continue; I believe this is equivalent to "break". If so, can we s/continue/break to be consistent with other switch cases? https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:47: // Deflate responses may or may not have a zlib header. In this state until For future reference, could you add a link to bug 677001? https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream_unittest.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:312: TEST_P(GzipSourceStreamTest, GzipCorrectnessWithSmallInputBuffer) { Could you remove this test? After the AddReadResult() change, this one test is equivalent to GzipCorrectness with ReadResultType::ONE_BYTE_AT_A_TIME. https://codereview.chromium.org/2604233002/diff/20001/net/filter/mock_source_... File net/filter/mock_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/mock_source_... net/filter/mock_source_stream.cc:23: EXPECT_TRUE(results_.empty()); On 2016/12/29 22:39:31, mmenke (Out Dec 17 to Jan 2) wrote: > Generally, DCHECKs should check for test fixture bugs, EXPECTs for bugs in > underlying code... The updated tests ran into this DCHECK until I fixed he bug, > so thought it best to switch. Acknowledged. https://codereview.chromium.org/2604233002/diff/20001/net/filter/mock_source_... net/filter/mock_source_stream.cc:68: // Doesn't make any sense to have both an error and data. It makes sense to check |len| when error != OK. Could you move the two DCHECKs above inside the "if (error != OK)" ? The second DCHECK is equivalent to the one that you added. https://codereview.chromium.org/2604233002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:190: scoped_refptr<IOBuffer> reaplay_buffer_( nit: Do you mean replay_buffer instead of reaplay_buffer?
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Thanks for the feedback, great catches! https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:30: const int kMaxZlibHeaderSniffBytes = 1000; On 2017/01/03 16:21:17, xunjieli wrote: > Do you mean 100 instead of 1000? In the linked bug, you mentioned 100. I meant 1000, out of paranoia (And because the cost of the larger buffer is basically 0). The compression tables of the first deflate block should be under 100 bytes, anyways, but might as well be careful. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:152: continue; On 2017/01/03 16:21:17, xunjieli wrote: > I believe this is equivalent to "break". If so, can we s/continue/break to be > consistent with other switch cases? Done. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:176: case STATE_REPLAY_DATA: { On 2017/01/03 16:21:17, xunjieli wrote: > nit: I am assuming this state only applies to deflate stream, if so maybe add a > DCHECK_EQ(TYPE_DEFLATE, type()). Done. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:177: if (!replay_data_.size()) { On 2017/01/03 16:21:17, xunjieli wrote: > On 2016/12/29 22:39:31, mmenke (Out Dec 17 to Jan 2) wrote: > > Worth noting while we do go through both branches in tests, in no test is the > > check false twice in a row, since replay_data_ is never longer than 1 byte. > > nit: Can we not enter STATE_REPLAY_DATA, if |replay_data_| is empty on line 196? > maybe change "!replay_data_.size()" to "replay_data_.empty()"? I find the latter > is easier to read. Done (x2). You're certainly right about empty(), beyond readability, empty() may also be more efficient on some platforms. Not sure skipping STATE_REPLAY_DATA really gets us much, but don't object to doing so. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:203: continue; On 2017/01/03 16:21:17, xunjieli wrote: > I believe this is equivalent to "break". If so, can we s/continue/break to be > consistent with other switch cases? Done. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:47: // Deflate responses may or may not have a zlib header. In this state until On 2017/01/03 16:21:17, xunjieli wrote: > For future reference, could you add a link to bug 677001? SGTM, Done. https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream_unittest.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:312: TEST_P(GzipSourceStreamTest, GzipCorrectnessWithSmallInputBuffer) { On 2017/01/03 16:21:17, xunjieli wrote: > Could you remove this test? > After the AddReadResult() change, this one test is equivalent to GzipCorrectness > with ReadResultType::ONE_BYTE_AT_A_TIME. Nice catch! https://codereview.chromium.org/2604233002/diff/20001/net/filter/mock_source_... File net/filter/mock_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/mock_source_... net/filter/mock_source_stream.cc:68: // Doesn't make any sense to have both an error and data. On 2017/01/03 16:21:17, xunjieli wrote: > It makes sense to check |len| when error != OK. Could you move the two DCHECKs > above inside the "if (error != OK)" ? > The second DCHECK is equivalent to the one that you added. I think you mean add them to an else clause for that if (i.e., only check them in the OK case)? Done. Also switched the DCHECK_GE to DCHECK_EQ (Which is what it was supposed to be) https://codereview.chromium.org/2604233002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:190: scoped_refptr<IOBuffer> reaplay_buffer_( On 2017/01/03 16:21:18, xunjieli wrote: > nit: Do you mean replay_buffer instead of reaplay_buffer? Done (The underscore at the end was also wrong).
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/2604233002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:30: const int kMaxZlibHeaderSniffBytes = 1000; On 2017/01/03 18:25:41, mmenke wrote: > On 2017/01/03 16:21:17, xunjieli wrote: > > Do you mean 100 instead of 1000? In the linked bug, you mentioned 100. > > I meant 1000, out of paranoia (And because the cost of the larger buffer is > basically 0). The compression tables of the first deflate block should be under > 100 bytes, anyways, but might as well be careful. Acknowledged. https://codereview.chromium.org/2604233002/diff/80001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/80001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:156: if (!replay_data_.empty()) { Ah, sorry.. I hadn't realized that |replay_data_| can be empty here too. I was referring to skipping the replay state farther down the file, something like: case STATE_REPLAY_DATA: { int result = FilterData(); ... if (replay_data_.empty()) { // Skip going into REPLAY_DATA. std::move(replay_data_); } else { // Back up resulting state, and return state to STATE_REPLAY_DATA. replay_state_ = input_state_; input_state_ = STATE_REPLAY_DATA;; } } Given that there are two scenarios where |replay_data_| can be empty when entering STATE_REPLAY_DATA, I agree that the if-conditional check is the cleanest way. Could you revert this chunk to what you had before? Thanks!
https://codereview.chromium.org/2604233002/diff/80001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2604233002/diff/80001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:156: if (!replay_data_.empty()) { On 2017/01/03 18:52:03, xunjieli wrote: > Ah, sorry.. I hadn't realized that |replay_data_| can be empty here too. I was > referring to skipping the replay state farther down the file, something like: > > case STATE_REPLAY_DATA: { > int result = FilterData(); > ... > if (replay_data_.empty()) { > // Skip going into REPLAY_DATA. > std::move(replay_data_); > } else { > // Back up resulting state, and return state to STATE_REPLAY_DATA. > replay_state_ = input_state_; > input_state_ = STATE_REPLAY_DATA;; > } > } > > Given that there are two scenarios where |replay_data_| can be empty when > entering STATE_REPLAY_DATA, I agree that the if-conditional check is the > cleanest way. > > Could you revert this chunk to what you had before? Thanks! Thanks for the explanation! Reverting this change (Keeping the replay_state_ DCHECK I added, though).
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2604233002/#ps100001 (title: "Revert bypassing replay state")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/03 19:06:34, mmenke wrote: > https://codereview.chromium.org/2604233002/diff/80001/net/filter/gzip_source_... > File net/filter/gzip_source_stream.cc (right): > > https://codereview.chromium.org/2604233002/diff/80001/net/filter/gzip_source_... > net/filter/gzip_source_stream.cc:156: if (!replay_data_.empty()) { > On 2017/01/03 18:52:03, xunjieli wrote: > > Ah, sorry.. I hadn't realized that |replay_data_| can be empty here too. I was > > referring to skipping the replay state farther down the file, something like: > > > > case STATE_REPLAY_DATA: { > > int result = FilterData(); > > ... > > if (replay_data_.empty()) { > > // Skip going into REPLAY_DATA. > > std::move(replay_data_); > > } else { > > // Back up resulting state, and return state to STATE_REPLAY_DATA. > > replay_state_ = input_state_; > > input_state_ = STATE_REPLAY_DATA;; > > } > > } > > > > Given that there are two scenarios where |replay_data_| can be empty when > > entering STATE_REPLAY_DATA, I agree that the if-conditional check is the > > cleanest way. > > > > Could you revert this chunk to what you had before? Thanks! > > Thanks for the explanation! Reverting this change (Keeping the replay_state_ > DCHECK I added, though). LGTM!
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483470404910520,
"parent_rev": "372e9685507b02f917bfc49ae0a829b0b93ee6d1", "commit_rev":
"e32f51c6aeefe5b2f4f2e8efaf2f969ca202de7d"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e Cr-Commit-Position: refs/heads/master@{#441197} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e66c81aea915be21bb56cb458f5efe8703aea78e Cr-Commit-Position: refs/heads/master@{#441197} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
