Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2314073003:
Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. (Closed)
DescriptionHandle non-HTTP/1.1 requests more gracefully in net::HttpServer.
Currently, HTTP/1.0 requests are causing http_server.cc to crash.
Code shouldn't crash when it is supplied with bad data. Simply
move into an error state and close the connection instead.
BUG=
Committed: https://crrev.com/f955ff0a59acc5e389d83a3497407f92989277fe
Cr-Commit-Position: refs/heads/master@{#419327}
Patch Set 1 #Patch Set 2 : Add unittests. #Patch Set 3 : Close the connection on a parsing error #
Total comments: 6
Patch Set 4 : More tests #
Total comments: 4
Patch Set 5 : . #
Messages
Total messages: 31 (18 generated)
slan@chromium.org changed reviewers: + davidben@chromium.org, mmenke@chromium.org
This handles malformed incoming requests a little more gracefully than crashing. PTAL!
The CQ bit was checked by slan@chromium.org to run a CQ dry run
Description was changed from ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing net_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and refuse to handle the request instead. BUG= ========== to ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and refuse to handle the request instead. BUG= ==========
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.
On 2016/09/06 22:16:13, slan wrote: > This handles malformed incoming requests a little more gracefully than crashing. > PTAL! Just blackholing the request and all subsequent requests to that socket doesn't seem all that graceful to me. Could you please file a bug for this? I'm not sure what the point is of this change.
Description was changed from ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and refuse to handle the request instead. BUG= ========== to ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and close the connection instead. BUG= ==========
Description was changed from ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and close the connection instead. BUG= ========== to ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and close the connection instead. BUG= ==========
On 2016/09/07 16:47:18, mmenke wrote: > On 2016/09/06 22:16:13, slan wrote: > > This handles malformed incoming requests a little more gracefully than > crashing. > > PTAL! > > Just blackholing the request and all subsequent requests to that socket doesn't > seem all that graceful to me. Could you please file a bug for this? I'm not > sure what the point is of this change. As discussed offline, the connection is now closed when a bad request comes in. PTAL!
Friendly ping!
On 2016/09/12 19:36:09, slan wrote: > Friendly ping! I'm sorry, this dropped of my radar. I'll try to get to it today. My main concern is the HasClosed connect check / passing that data through a sidechannel, wonder if we could do something a bit cleaner.
https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server.... net/server/http_server.h:101: // recv data. If parsing fails, the connection will be closed. Here's my suggestion: Return false on error, leave *pos as 0 when we haven't reached the end of the headers yet. Hrm...the HTTP header parsing code state-machine is ugly. :( https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server_... net/server/http_server_unittest.cc:197: }; We try not to use Gmock in net - it makes tests significantly harder to follow, because of all the baked-in magic, and a lot of committers aren't familiar with it. Can we just do something like HttpServerTest, and make sure the request fails with ERR_CONNECTION_CLOSED, instead? Think that's a better test of the API exposed to consumers of the server. https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server_... net/server/http_server_unittest.cc:503: ASSERT_FALSE(RunUntilRequestsReceived(1)); Maybe also a test with "GET /test foo\r\n\r\n" and "GET / test\r\n\r\n". Fine to just have an array of test cases and loop through them, since they should all fail the same way.
PTAL! https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server.h File net/server/http_server.h (right): https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server.... net/server/http_server.h:101: // recv data. If parsing fails, the connection will be closed. On 2016/09/12 20:35:04, mmenke wrote: > Here's my suggestion: Return false on error, leave *pos as 0 when we haven't > reached the end of the headers yet. Hrm...the HTTP header parsing code > state-machine is ugly. :( Done. https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server_... net/server/http_server_unittest.cc:197: }; On 2016/09/12 20:35:04, mmenke wrote: > We try not to use Gmock in net - it makes tests significantly harder to follow, > because of all the baked-in magic, and a lot of committers aren't familiar with > it. > > Can we just do something like HttpServerTest, and make sure the request fails > with ERR_CONNECTION_CLOSED, instead? Think that's a better test of the API > exposed to consumers of the server. OK, I removed the use of GMock and added checks for two things: 1. Proper delegate use - I added some very simple state to assert that the delegate methods were called in the right order. We expect to receive events when the connection is opened, and when it is closed upon seeing the bad protocol. 2. That the server actually hangs up - I added checks on the socket used by the client to test that the server actually disconnects when it parses the bad header. (I tried to modify the test to use URLFetcher, but I couldn't find where to override the HTTP version that's used for the request. So I went lower-level - I think sending bytes over the socket is a little clearer anyway.) PTAL at the additions and LMK what you think, Matt. https://codereview.chromium.org/2314073003/diff/40001/net/server/http_server_... net/server/http_server_unittest.cc:503: ASSERT_FALSE(RunUntilRequestsReceived(1)); On 2016/09/12 20:35:04, mmenke wrote: > Maybe also a test with "GET /test foo\r\n\r\n" and "GET / test\r\n\r\n". Fine > to just have an array of test cases and loop through them, since they should all > fail the same way. Done.
The CQ bit was checked by slan@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.
Sorry I didn't get to this yesterday, was out (And sorry for general slowness on this CL) https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server.... net/server/http_server.cc:239: // If pos is 0, we read all the data in read_buf, but haven't yet finished nit: avoid "we" in comments, due to ambiguity ("We the object", "We the developers", "We the user", "We the television network", etc). Prefer passive voice. https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:507: ASSERT_FALSE(client.socket().IsConnected()); This is potentially racy - the server could exit the message loop before the client socket sees that it's closed. Think can just do this to fix that: std::string response; EXPECT_FALSE(client.socket().Read(response)); EXPECT_EQ("", response);
PTAL! https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server.cc File net/server/http_server.cc (right): https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server.... net/server/http_server.cc:239: // If pos is 0, we read all the data in read_buf, but haven't yet finished On 2016/09/15 17:40:12, mmenke wrote: > nit: avoid "we" in comments, due to ambiguity ("We the object", "We the > developers", "We the user", "We the television network", etc). Prefer passive > voice. Done. https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server_... File net/server/http_server_unittest.cc (right): https://codereview.chromium.org/2314073003/diff/60001/net/server/http_server_... net/server/http_server_unittest.cc:507: ASSERT_FALSE(client.socket().IsConnected()); On 2016/09/15 17:40:12, mmenke wrote: > This is potentially racy - the server could exit the message loop before the > client socket sees that it's closed. > > Think can just do this to fix that: > > std::string response; > EXPECT_FALSE(client.socket().Read(response)); > EXPECT_EQ("", response); Done.
The CQ bit was checked by slan@chromium.org to run a CQ dry run
LGTM!
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 slan@chromium.org
The CQ bit was checked by slan@chromium.org
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 ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and close the connection instead. BUG= ========== to ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and close the connection instead. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and close the connection instead. BUG= ========== to ========== Handle non-HTTP/1.1 requests more gracefully in net::HttpServer. Currently, HTTP/1.0 requests are causing http_server.cc to crash. Code shouldn't crash when it is supplied with bad data. Simply move into an error state and close the connection instead. BUG= Committed: https://crrev.com/f955ff0a59acc5e389d83a3497407f92989277fe Cr-Commit-Position: refs/heads/master@{#419327} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f955ff0a59acc5e389d83a3497407f92989277fe Cr-Commit-Position: refs/heads/master@{#419327} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
