Bug 121123 - Web Inspector: Do not try to parse incomplete HTTP requests
Summary: Web Inspector: Do not try to parse incomplete HTTP requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 121121
  Show dependency treegraph
 
Reported: 2013-09-10 14:17 PDT by Andre Moreira Magalhaes
Modified: 2013-09-16 08:48 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.28 KB, patch)
2013-09-10 14:25 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch with GTK tests (7.12 KB, patch)
2013-09-12 11:50 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch addressing Carlos review (4.02 KB, patch)
2013-09-13 08:23 PDT, Andre Moreira Magalhaes
cgarcia: review-
cgarcia: commit-queue-
Details | Formatted Diff | Diff
Patch addressing Carlos review (new round) (4.20 KB, patch)
2013-09-16 07:06 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2013-09-10 14:17:37 PDT
When working on a patch for bug #121121 I found an issue with the InspectorServer where it would try to parse an HTTP message before receiving the full message and thus fail connecting with the chromedevtools plugin.

What happens is that the WebSocketServerConnection receives buffers on WebSocketServerConnection::didReceiveSocketStreamData and calls WebSocketServerConnection::readHTTPMessage which then checks if we have a valid request by calling HTTPRequest::parseHTTPRequestFromBuffer. If the request is valid it tries to parse the message and clears the buffer, otherwise it continues adding data to the internal buffer until we have a valid request.

The problem is that currently HTTPRequest::parseHTTPRequestFromBuffer considers the request as valid before receiving the full message. To solve this we should make the method check if the request headers end with a blank line otherwise we consider the request as invalid (see also http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html).

Patch to follow.
Comment 1 Radar WebKit Bug Importer 2013-09-10 14:17:55 PDT
<rdar://problem/14958134>
Comment 2 Andre Moreira Magalhaes 2013-09-10 14:25:18 PDT
Created attachment 211243 [details]
Patch
Comment 3 Andre Moreira Magalhaes 2013-09-12 11:50:57 PDT
Created attachment 211453 [details]
Patch with GTK tests

This patch is the same as attachment #211243 [details] but it also includes a patch for the GTK TestInspectorServer.

I did not write a generic test as I am not sure how (or if even possible) to do a similar test using JS+HTML. If there is another option please let me know and I can update the test to not be gtk specific.

Feel free to apply the other patch without the test if you prefer.
Comment 4 WebKit Commit Bot 2013-09-12 11:53:20 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 5 WebKit Commit Bot 2013-09-12 12:59:20 PDT
Comment on attachment 211453 [details]
Patch with GTK tests

Clearing flags on attachment: 211453

Committed r155642: <http://trac.webkit.org/changeset/155642>
Comment 6 Carlos Garcia Campos 2013-09-13 00:59:23 PDT
Comment on attachment 211453 [details]
Patch with GTK tests

View in context: https://bugs.webkit.org/attachment.cgi?id=211453&action=review

I'm late again, sorry, I have some comments, though, please consider fixing them in a follow up commit.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:248
> +    GSocketClient* client = g_socket_client_new();

You are leaking this, please use GRefPtr

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249
> +    GSocketConnection* connection = g_socket_client_connect_to_host(client, "127.0.0.1", 2999, NULL, &error.outPtr());

Use 0 instead of NULL for the cancellable. You are leaking the connection too, please use GRefPtr.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:250
> +    g_assert(!error.get());

Use g_assert_no_error instead, that way if there's an error you will see a message with the error description.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:258
> +    g_output_stream_write(ostream, incompleteRequest, strlen(incompleteRequest), NULL, &error.outPtr());

Use 0 instead of NULL.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:259
> +    g_assert(!error.get());

Use g_assert_no_error instead, that way if there's an error you will see a message with the error description.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264
> +    g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, NULL, NULL, NULL);

Use 0 instead of NULL.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268
> +    // Give a chance for the server to reply.
> +    test->wait(2);
> +    // If we got any answer it means the server replied to an incomplete request, lets fail.
> +    g_assert(String(response).isEmpty());

I don't think this is the proper way to test this. You should use a GAsyncReadyCallback and spin a run loop to get the bytes read with g_input_stream_read_finish(). You can also start a timeout idle to assert when reached. With this code we are always waiting for 2 seconds, making running the tests slower even with it runs correctly. Also you don't need to initialize the buffer, and then convert it to a String to check if it's empty, just check the bytes actually read.
Comment 7 Andre Moreira Magalhaes 2013-09-13 08:23:55 PDT
Created attachment 211555 [details]
Patch addressing Carlos review

(In reply to comment #6)
> (From update of attachment 211453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211453&action=review
> 
> I'm late again, sorry, I have some comments, though, please consider fixing them in a follow up commit.
> 
np, attach patch should address the issues, see comments below.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:248
> > +    GSocketClient* client = g_socket_client_new();
> 
> You are leaking this, please use GRefPtr
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249
> > +    GSocketConnection* connection = g_socket_client_connect_to_host(client, "127.0.0.1", 2999, NULL, &error.outPtr());
> 
> Use 0 instead of NULL for the cancellable. You are leaking the connection too, please use GRefPtr.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:250
> > +    g_assert(!error.get());
> 
> Use g_assert_no_error instead, that way if there's an error you will see a message with the error description.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:258
> > +    g_output_stream_write(ostream, incompleteRequest, strlen(incompleteRequest), NULL, &error.outPtr());
> 
> Use 0 instead of NULL.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:259
> > +    g_assert(!error.get());
> 
> Use g_assert_no_error instead, that way if there's an error you will see a message with the error description.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264
> > +    g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, NULL, NULL, NULL);
> 
> Use 0 instead of NULL.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268
> > +    // Give a chance for the server to reply.
> > +    test->wait(2);
> > +    // If we got any answer it means the server replied to an incomplete request, lets fail.
> > +    g_assert(String(response).isEmpty());
> 
> I don't think this is the proper way to test this. You should use a GAsyncReadyCallback and spin a run loop to get the bytes read with g_input_stream_read_finish(). You can also start a timeout idle to assert when reached. With this code we are always waiting for 2 seconds, making running the tests slower even with it runs correctly. Also you don't need to initialize the buffer, and then convert it to a String to check if it's empty, just check the bytes actually read.
Quoting from IRC:
<andrunko> KaL, hi there, so for bug 121123, I will do the changes you suggested but I am not sure about the last suggestion (do not wait 2 seconds)
<andrunko> the thing is that with the patch (where the test pass) we should wait for a timeout as expected result
<andrunko> that is why we wait 2 seconds to give a chance to the inspector server to reply, if it does /not/ reply we have a successful test
<andrunko> if it does reply the test should fail
<KaL> ah, I see, it's the opposite to what I thought

I changed the timeout to 1 second though.

The test still fails without the fix and works with the fix accordingly.

Thanks both for the reviews.
Comment 8 Carlos Garcia Campos 2013-09-14 00:33:22 PDT
Comment on attachment 211555 [details]
Patch addressing Carlos review

View in context: https://bugs.webkit.org/attachment.cgi?id=211555&action=review

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249
> +    GRefPtr<GSocketClient> client = g_socket_client_new();
> +    GRefPtr<GSocketConnection> connection = g_socket_client_connect_to_host(client.get(), "127.0.0.1", 2999, 0, &error.outPtr());

This is still wrong, you have to adopt the reference with adoptGRef().

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264
> +    g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, 0, 0, 0);

Since the operation might never finish, I think it would be better to pass a cancellable, and cancel the operation before the test finish so that the thread used to implement the async operation exits.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268
>      g_assert(String(response).isEmpty());

You don't need to convert the buffer to a String just to check if it's empty, you can simply check response[0] == '\0'
Comment 9 Andre Moreira Magalhaes 2013-09-16 07:06:52 PDT
Created attachment 211778 [details]
Patch addressing Carlos review (new round)

(In reply to comment #8)
> (From update of attachment 211555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211555&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249
> > +    GRefPtr<GSocketClient> client = g_socket_client_new();
> > +    GRefPtr<GSocketConnection> connection = g_socket_client_connect_to_host(client.get(), "127.0.0.1", 2999, 0, &error.outPtr());
> 
> This is still wrong, you have to adopt the reference with adoptGRef().
> 
Done.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264
> > +    g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, 0, 0, 0);
> 
> Since the operation might never finish, I think it would be better to pass a cancellable, and cancel the operation before the test finish so that the thread used to implement the async operation exits.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268
> >      g_assert(String(response).isEmpty());
> 
> You don't need to convert the buffer to a String just to check if it's empty, you can simply check response[0] == '\0'
Of course, don't ask me why I did that :D. Done
Comment 10 Andre Moreira Magalhaes 2013-09-16 07:09:13 PDT
Btw, TestInspectorServer is hanging on master after 86b826a. The test gets stuck on "/webkit2/WebKitWebInspectorServer/test-open-debugging-session:".

I had to comment this failing testcase to test the updated patch on attachment #211778 [details].
Comment 11 Gustavo Noronha (kov) 2013-09-16 07:44:58 PDT
The timeout is my bad, see https://bugs.webkit.org/show_bug.cgi?id=121383
Comment 12 Carlos Garcia Campos 2013-09-16 08:25:24 PDT
Comment on attachment 211778 [details]
Patch addressing Carlos review (new round)

Thanks!
Comment 13 WebKit Commit Bot 2013-09-16 08:48:54 PDT
Comment on attachment 211778 [details]
Patch addressing Carlos review (new round)

Clearing flags on attachment: 211778

Committed r155861: <http://trac.webkit.org/changeset/155861>
Comment 14 WebKit Commit Bot 2013-09-16 08:48:57 PDT
All reviewed patches have been landed.  Closing bug.