Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648553002:
HttpServer::ParseHeaders: don't DCHECK on bogus headers termination (Closed)
|
Created:
3 years, 11 months ago by Maks Orlovich Modified:
3 years, 11 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHttpServer::ParseHeaders: don't DCHECK on bogus headers termination
DCHECK seems like the wrong approach for invalid wire data, rather handle
it as a parse failure.
BUG=606428
Review-Url: https://codereview.chromium.org/2648553002
Cr-Commit-Position: refs/heads/master@{#445173}
Committed: https://chromium.googlesource.com/chromium/src/+/860f9bdcd6642816bd2a4879bc878f8473927d9f
Patch Set 1 #Patch Set 2 : HttpServer::ParseHeaders: don't DCHECK on bogus headers termination (Rather handle it as a parse fa… #
Total comments: 8
Patch Set 3 : Apply review feedback #Patch Set 4 : Remove test that doesn't belong in this CL. #
Total comments: 12
Patch Set 5 : More refinements based on review feedback. #
Messages
Total messages: 37 (25 generated)
The CQ bit was checked by morlovich@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...
Description was changed from ========== HttpServer::ParseHeaders: don't DCHECK on bogus headers termination DCHECK seems like the wrong approach for invalid wire data, rather handle it as a parse failure. BUG= ========== to ========== HttpServer::ParseHeaders: don't DCHECK on bogus headers termination DCHECK seems like the wrong approach for invalid wire data, rather handle it as a parse failure. BUG=606428 ==========
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_...)
On 2017/01/19 20:46:31, commit-bot: I haz the power wrote: > 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 ASAN failure is probably because the test doesn't give the DeleteSoon a chance to run. Haven't actually figured out what the Windows failure is.
The CQ bit was checked by morlovich@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.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Fix looks good, just some suggestions on the test. https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:233: FROM_HERE, run_loop_quit_func_, base::TimeDelta::FromMilliseconds(1)); Not a fan of this sort of fuziness, particularly if just a PostTask isn't enough. Could we just add a base::RunLoop().RunUntilIdle() call to the test fixture's destructor instead? https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:268: size_t GetNumRequests() { return requests_.size(); } nit const https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:268: size_t GetNumRequests() { return requests_.size(); } optional nit: Think it's a little more common to call this sort of method num_requests(), though I could be wrong. https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:329: EXPECT_EQ(0u, GetNumRequests()); Wait for the client socket to be closed, and verify it gets no data? Could add a helper to TestHttpClient to do that, not sure if any of these tests currently check for client socket closure.
The CQ bit was checked by morlovich@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/2648553002/diff/20001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:233: FROM_HERE, run_loop_quit_func_, base::TimeDelta::FromMilliseconds(1)); On 2017/01/20 15:55:11, mmenke wrote: > Not a fan of this sort of fuziness, particularly if just a PostTask isn't > enough. Could we just add a base::RunLoop().RunUntilIdle() call to the test > fixture's destructor instead? That is a nicer option, indeed. Did that (in TearDown). Thanks! https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:268: size_t GetNumRequests() { return requests_.size(); } On 2017/01/20 15:55:11, mmenke wrote: > nit const Done. https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:268: size_t GetNumRequests() { return requests_.size(); } On 2017/01/20 15:55:11, mmenke wrote: > optional nit: Think it's a little more common to call this sort of method > num_requests(), though I could be wrong. Done. https://codereview.chromium.org/2648553002/diff/20001/net/server/http_server_... net/server/http_server_unittest.cc:329: EXPECT_EQ(0u, GetNumRequests()); On 2017/01/20 15:55:11, mmenke wrote: > Wait for the client socket to be closed, and verify it gets no data? Could add > a helper to TestHttpClient to do that, not sure if any of these tests currently > check for client socket closure. WrongProtocolRequest did that, so I factored out a helper for use in both spots. One other thing I noticed: this test seems to use ASSERT_ almost exclusively, rather than trying to decide between ASSERT_ and EXPECT_ on case-by-case basis, should I be doing the same?
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_...)
Oh, looks like I made some sort of merge mistake. WebSocketTest.RequestWebSocketTrailingJunk is meant for a different CL.
The CQ bit was checked by morlovich@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, mostly just nits. https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:136: bool TestUsedThenDisconnectedWithNoData() { Rather than a bool, suggest: void ExpectUsedThenDisconnectedWithNoData() { ASSERT... ASSERT... ASSERT... } Failures will then tell us which check failed. https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:140: } nit: No braces when if + body take up one line each. https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:147: } nit: --braces https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:222: // We need to run the event loop some to make sure that the memory nit: Avoid "we" in comments - it's a bit ambiguous who "we" is (The code authors? The committers? The user? etc.) https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:253: } nit: --braces https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:272: // Already disconnected nit: Predominant style is to send more comment-y comments with periods (As opposed to just label-y comments)
The CQ bit was checked by morlovich@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...
Any guidance on that EXPECT_ vs. ASSERT_ thing? https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:136: bool TestUsedThenDisconnectedWithNoData() { On 2017/01/20 18:20:57, mmenke wrote: > Rather than a bool, suggest: > > void ExpectUsedThenDisconnectedWithNoData() { > ASSERT... > ASSERT... > ASSERT... > } > > Failures will then tell us which check failed. Done. https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:140: } On 2017/01/20 18:20:57, mmenke wrote: > nit: No braces when if + body take up one line each. Made moot by above. https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:147: } On 2017/01/20 18:20:57, mmenke wrote: > nit: --braces Likewise https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:222: // We need to run the event loop some to make sure that the memory On 2017/01/20 18:20:57, mmenke wrote: > nit: Avoid "we" in comments - it's a bit ambiguous who "we" is (The code > authors? The committers? The user? etc.) Done. https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:253: } On 2017/01/20 18:20:57, mmenke wrote: > nit: --braces Done. https://codereview.chromium.org/2648553002/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:272: // Already disconnected On 2017/01/20 18:20:57, mmenke wrote: > nit: Predominant style is to send more comment-y comments with periods (As > opposed to just label-y comments) Done.
On 2017/01/20 19:00:20, morlovich1 wrote: > Any guidance on that EXPECT_ vs. ASSERT_ thing? Sorry, forgot about that. I'd say choose on a case-by-case basis. If you think code is likely to hang or crash if a check fails, or the rest of the test is completely meaningless if it fails, prefer ASSERT, but consider it best effort only, and don't spend time thinking too much about it. It used to be that if every single test hung, the bots would all get stuck, but now if too many of them do, the bots skip the rest. And bots handle crashes reasonably, too (Except maybe on mobile).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. Is this good to go? (On Monday morning, I guess...)
On 2017/01/20 21:20:31, morlovich1 wrote: > Thanks. Is this good to go? (On Monday morning, I guess...) Indeed it is. Convention is LGTM with comments means good to go, unless the reviewer says otherwise, or the reviewee feels another pass is needed.
On 2017/01/20 21:24:37, mmenke wrote: > On 2017/01/20 21:20:31, morlovich1 wrote: > > Thanks. Is this good to go? (On Monday morning, I guess...) > > Indeed it is. Convention is LGTM with comments means good to go, unless the > reviewer says otherwise, or the reviewee feels another pass is needed. And it's fine to land now - once something passes the trybots, no need to stay around and watch it, unless you feel it's particularly risky. Fine to land Monday, too, of course.
The CQ bit was checked by morlovich@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484947750563310,
"parent_rev": "1340ff2da37b5fcd669e08842cea9223b325a873", "commit_rev":
"860f9bdcd6642816bd2a4879bc878f8473927d9f"}
Message was sent while issue was closed.
Description was changed from ========== HttpServer::ParseHeaders: don't DCHECK on bogus headers termination DCHECK seems like the wrong approach for invalid wire data, rather handle it as a parse failure. BUG=606428 ========== to ========== HttpServer::ParseHeaders: don't DCHECK on bogus headers termination DCHECK seems like the wrong approach for invalid wire data, rather handle it as a parse failure. BUG=606428 Review-Url: https://codereview.chromium.org/2648553002 Cr-Commit-Position: refs/heads/master@{#445173} Committed: https://chromium.googlesource.com/chromium/src/+/860f9bdcd6642816bd2a4879bc87... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/860f9bdcd6642816bd2a4879bc87... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
