Bug 106789 - [SOUP] work around a glib bug exposed by the new redirect code
Summary: [SOUP] work around a glib bug exposed by the new redirect code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-14 06:34 PST by Dan Winship
Modified: 2013-09-09 05:31 PDT (History)
8 users (show)

See Also:


Attachments
patch to use read_async instead of skip_async (4.00 KB, patch)
2013-01-14 06:37 PST, Dan Winship
no flags Details | Formatted Diff | Diff
updated for comments (4.05 KB, patch)
2013-01-15 07:23 PST, Dan Winship
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Winship 2013-01-14 06:34:30 PST
qv https://bugzilla.gnome.org/show_bug.cgi?id=691489

Since it will presumably be a while before we depend on a new-enough glib to have the fix, work around this by using read_async() rather than skip_async().

(I'm not 100% sure that this completely fixes the bug there, because I had a hard time reproducing it reliably. So, testing welcome. Note that if you're testing with epiphany, you need to rm -rf ~/.cache/epiphany/ after it crashes.)
Comment 1 Dan Winship 2013-01-14 06:37:35 PST
Created attachment 182567 [details]
patch to use read_async instead of skip_async

(intentionally not setting cq? because I'd like people to test it and confirm that it does help first...)
Comment 2 Martin Robinson 2013-01-14 16:13:14 PST
Comment on attachment 182567 [details]
patch to use read_async instead of skip_async

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

Looks reasonable to me.

> Source/WebCore/ChangeLog:5
> +        https://bugzilla.gnome.org/show_bug.cgi?id=691489

I'm a little worried that this might confuse the tools which parse ChangeLogs, so probably better to put this into the description.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:671
> +            // This can switch to using skip_async after we depend on glib > 2.35.4

Probably good to put a link to the bug.
Comment 3 Dan Winship 2013-01-15 07:23:41 PST
Created attachment 182763 [details]
updated for comments

This removes the bgo URL form ChangeLog and adds it to ResourceHandleSoup.cpp.


If we're going to put out a point release to deal with bug 106903 we should probably get this in too.
Comment 4 Dan Winship 2013-01-21 08:18:45 PST
Comment on attachment 182763 [details]
updated for comments

ok, well, not sure anyone is actually testing this, but at least no one has said "i tested with it and it didn't fix the bug", so let's commit it
Comment 5 WebKit Review Bot 2013-01-21 08:24:41 PST
Comment on attachment 182763 [details]
updated for comments

Clearing flags on attachment: 182763

Committed r140338: <http://trac.webkit.org/changeset/140338>
Comment 6 WebKit Review Bot 2013-01-21 08:24:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Sergio Villar Senin 2013-01-22 11:53:30 PST
Dan I think we need to revert it. The current libsoup stack does not handle the G_MAXSIZE value for count in read_async as it tries to create a buffer of that size.
Comment 8 Sergio Villar Senin 2013-01-22 11:57:14 PST
(In reply to comment #7)
> Dan I think we need to revert it. The current libsoup stack does not handle the G_MAXSIZE value for count in read_async as it tries to create a buffer of that size.

The problem is actually located in read_internal() in gconverterinputstream.c the first time it tries to read something from the underlying string.
Comment 9 Dan Winship 2013-01-22 12:00:00 PST
ugh, the G_MAXSSIZE should have changed to READ_BUFFER_SIZE... The code now would be a buffer overrun if someone returned a redirect response that was larger than READ_BUFFER_SIZE. (Which the test suite never does... actually, that might be a good test...)
Comment 10 Alberto Garcia 2013-09-09 05:31:56 PDT
This has already been reverted in http://trac.webkit.org/changeset/151013

Since we depend on a glib version that doesn't have this bug we can close this.