Bug 98055 – [soup] WebKit crashes when doing a http request

• Component: Platform
• Status: RESOLVED FIXED

[arno.]

Hi,
webkit trunk GtkLauncher crashes at startup in:
#0  0xb70f4237 in WebCore::requestStartedCallback(_SoupSession*, _SoupMessage*, _SoupSocket*, void*) ()
   from /home/arno/webkit/WebKit.upstream/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#1  0xb624c49a in _soup_marshal_VOID__OBJECT_OBJECT () from /usr/lib/i386-linux-gnu/libsoup-2.4.so.1
#2  0xb6085484 in g_closure_invoke () from /usr/lib/i386-linux-gnu/libgobject-2.0.so.0

There are two reasons for the crash:
- soup_add_timeout is called with  d->m_firstRequest.timeoutInterval() * 1000 which evaluates to 0 in my computer (instead of evaluating to INT_MAX)
- WebKit crashes when a request timeout before request-started signal is emitted.

[arno.]

Created attachment 166517 [details]
c testcase: outputs 0 when compiled with -msse2 -mfpmath=sse

c testcase to understand why soup_add_timeout is called with a 0 argument.
This c testcase outputs 0d on i386 platform when compiled with gcc -msse2 -mfpmath=sse overflow.c -o overflow 
when compiled with just gcc  overflow.c -o overflow, it outputs 4294966296d
As default timeout value is double INT_MAX, this explains why
soup_add_timeout(g_main_context_get_thread_default(), d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);
results in a 0 argument.

[arno.]

Created attachment 166519 [details]
Patch

patch proposal

[Martin Robinson]

CCing Sergio who also works on the libsoup backend.

[arno.]

Created attachment 166541 [details]
Patch

updated patch: fixes a warning when calling setDefersLoading(true) and there is no timeout source

[Dominik Röttsches (drott)]

*** Bug 98157 has been marked as a duplicate of this bug. ***

[Dominik Röttsches (drott)]

Comment on attachment 166541 [details]
Patch

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

LGTM, thanks for the fix - I just spotted the early handle deletion issue today. Work on bug 74802 is progressing.

> Source/WebCore/ChangeLog:14
> +        occur. So, reset soupMessage handle data in

occurs

> Source/WebCore/ChangeLog:22
> +        cleanupSoupRequestOperation will be called automatically.

Thanks for fixing this - I just noticed this in the duplicate of this bug. I plan to upload the test and patch for 74802 tomorrow.
I'd change "and crash occur" -> and a crash occurs in sendRequestCallback due to an early deleted handle.

[Dominik Röttsches (drott)]

Comment on attachment 166541 [details]
Patch

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

Forgot to submit this part. (Note, I am not a reviewer, but maybe Martin or Gustavo can do the final review).

> Source/WebCore/ChangeLog:18
> +        deleted, and crash occur in sendRequestCallback. To avoid that, call

-> and a crash occurs in sendRequestCallback due to an early destroyed handle.

[arno.]

Created attachment 166910 [details]
Patch

updated patch

[Martin Robinson]

Comment on attachment 166910 [details]
Patch

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

Okay. Seems reasonable, but consider the following. Please also follow the suggestion above.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:379
> +        g_object_set_data(G_OBJECT(d->m_soupMessage.get()), "handle", NULL);

Use 0 instead of NULL here.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:769
> -        if (d->m_firstRequest.timeoutInterval() > 0) {
> +        guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000;
> +        if (timeoutInterval > 0) {

See below.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:878
> +        guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000;
> +        if (timeoutInterval > 0) {

Maybe it would be clearer to simply check for INT_MAX explicitly and just leave a comment like:

// INT_MAX * 1000 is zero on i386, so we explicitly check this situation.
unsigned timeoutInterval = d->m_firstRequest.timeoutInterval();
if (timeoutInterval && timeoutInterval != INT_MAX) {

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1045
> +        guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000;
> +        if (timeoutInterval > 0) {

Ditto.

[arno.]

(In reply to comment #9)

> Maybe it would be clearer to simply check for INT_MAX explicitly and just leave a comment like:
> 
> // INT_MAX * 1000 is zero on i386, so we explicitly check this situation.
> unsigned timeoutInterval = d->m_firstRequest.timeoutInterval();
> if (timeoutInterval && timeoutInterval != INT_MAX) {
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1045
> > +        guint timeoutInterval = d->m_firstRequest.timeoutInterval() * 1000;
> > +        if (timeoutInterval > 0) {

reading timeoutInterval as unsigned will yield wrong results when for example, timeoutInterval is defined as 0.5
also, I just realize if timeoutInterval is INT_MAX / 2 or ever INT_MAX / 100, it won't work either. So I'm thinking about something like:

        double timeoutInterval = d->m_firstRequest.timeoutInterval();
        // overflowed integer may evaluate either to 0 either to INT_MAX, so we explicitly check this situation.
        if (timeoutInterval * 1000 > INT_MAX) {
            timeoutInterval = 0;
        }

[arno.]

Created attachment 166924 [details]
Patch

updated patch

[Martin Robinson]

Maybe a better fix here is just to change this:

#if !PLATFORM(MAC) || USE(CFNETWORK)
double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX;
#else
// Will use NSURLRequest default timeout unless set to a non-zero value with setDefaultTimeoutInterval().
double ResourceRequestBase::s_defaultTimeoutInterval = 0;
#endif

in ResourceRequestBase.cpp, so that the s_defaultTimeoutInterval for SOUP is 0.

[arno.]

Created attachment 166938 [details]
Patch

updated patch

[Martin Robinson]

Comment on attachment 166938 [details]
Patch

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

Great! This is much, much simpler. Just one small change, I think.  

As an aside, I think this ambiguity between platforms isn't so great. It'd be nice if there was a constant NoTimeout that consumers of this API could check against. That could be another patch though.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:44
> +#if USE(SOUP)
> +// soup timeout is defined with integers milliseconds. We set 0 as default value to avoid integer overflow.
> +double ResourceRequestBase::s_defaultTimeoutInterval = 0;
> +#elif !PLATFORM(MAC) || USE(CFNETWORK)
>  double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX;
>  #else
>  // Will use NSURLRequest default timeout unless set to a non-zero value with setDefaultTimeoutInterval().

This could be folded to be:

#if !USE(SOUP) && (!PLATFORM(MAC) || USE(CFNETWORK))
double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX;
#else
double ResourceRequestBase::s_defaultTimeoutInterval = INT_MAX;
#endif

[arno.]

Created attachment 166943 [details]
Patch

updated patch

[Martin Robinson]

Comment on attachment 166943 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.cpp:41
> +#if USE(SOUP)
> +// soup timeout is defined with integers milliseconds. We set 0 as default value to avoid integer overflow.
> +double ResourceRequestBase::s_defaultTimeoutInterval = 0;
> +#elif !PLATFORM(MAC) || USE(CFNETWORK)

:) This isn't quite what I asked for, but I'll make the change and land it for you.

[arno.]

(In reply to comment #16)
> (From update of attachment 166943 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166943&action=review
> 
> > Source/WebCore/platform/network/ResourceRequestBase.cpp:41
> > +#if USE(SOUP)
> > +// soup timeout is defined with integers milliseconds. We set 0 as default value to avoid integer overflow.
> > +double ResourceRequestBase::s_defaultTimeoutInterval = 0;
> > +#elif !PLATFORM(MAC) || USE(CFNETWORK)
> 
> :) This isn't quite what I asked for, but I'll make the change and land it for you.


Sorry, I made the fix, but for some reason, failed to submit it correctly.
Thanks for making the change yourself.

[Martin Robinson]

(In reply to comment #17)

> Sorry, I made the fix, but for some reason, failed to submit it correctly.
> Thanks for making the change yourself.

No problem! Thanks for the fix. I'm testing the compilation here. If you want to land this sooner, you can always post a new patch and I can use the commit queue.

[Martin Robinson]

Committed r130348: <http://trac.webkit.org/changeset/130348>

[Dominik Röttsches (drott)]

Martin, Arno - thanks for landing this one!