Bug 121123 – Web Inspector: Do not try to parse incomplete HTTP requests

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Andre Moreira Magalhaes]

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.

[Radar WebKit Bug Importer]

<rdar://problem/14958134>

[Andre Moreira Magalhaes]

Created attachment 211243 [details]
Patch

[Andre Moreira Magalhaes]

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.

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 211453 [details]
Patch with GTK tests

Clearing flags on attachment: 211453

Committed r155642: <http://trac.webkit.org/changeset/155642>

[Carlos Garcia Campos]

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.

[Andre Moreira Magalhaes]

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.

[Carlos Garcia Campos]

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'

[Andre Moreira Magalhaes]

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

[Andre Moreira Magalhaes]

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].

[Gustavo Noronha (kov)]

The timeout is my bad, see https://bugs.webkit.org/show_bug.cgi?id=121383

[Carlos Garcia Campos]

Comment on attachment 211778 [details]
Patch addressing Carlos review (new round)

Thanks!

[WebKit Commit Bot]

Comment on attachment 211778 [details]
Patch addressing Carlos review (new round)

Clearing flags on attachment: 211778

Committed r155861: <http://trac.webkit.org/changeset/155861>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.