Summary: | [soup] WebKit crashes when doing a http request | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | arno. <a.renevier> | ||||||||||||||||
Component: | Platform | Assignee: | arno. <a.renevier> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cgarcia, danw, d-r, gustavo, laszlo.gombos, mrobinson, rakuco, svillar, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 74802 | ||||||||||||||||||
Attachments: |
|
Description
arno.
2012-10-01 11:43:46 PDT
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.
Created attachment 166519 [details]
Patch
patch proposal
CCing Sergio who also works on the libsoup backend. Created attachment 166541 [details]
Patch
updated patch: fixes a warning when calling setDefersLoading(true) and there is no timeout source
*** Bug 98157 has been marked as a duplicate of this bug. *** 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. 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. Created attachment 166910 [details]
Patch
updated patch
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. (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; } Created attachment 166924 [details]
Patch
updated patch
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. Created attachment 166938 [details]
Patch
updated patch
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 Created attachment 166943 [details]
Patch
updated patch
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. (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. (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. Committed r130348: <http://trac.webkit.org/changeset/130348> Martin, Arno - thanks for landing this one! |