Bug 47661 - Invalid base64 data in orphaned-selection-crash-bug32823-2.html
Summary: Invalid base64 data in orphaned-selection-crash-bug32823-2.html
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-14 04:11 PDT by Sergio Villar Senin
Modified: 2010-10-15 00:50 PDT (History)
3 users (show)

See Also:


Attachments
Fix for the bug (1.83 KB, patch)
2010-10-14 04:18 PDT, Sergio Villar Senin
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2010-10-14 04:11:43 PDT
That test includes this base64 encoded data

iVBORw0KGrkJggg==

which has 1 extra padding '='
Comment 1 Sergio Villar Senin 2010-10-14 04:18:32 PDT
Created attachment 70724 [details]
Fix for the bug

Removes an invalid extra "=" padding character in the data URL
Comment 2 Sergio Villar Senin 2010-10-14 05:35:26 PDT
Adding Ryosuke
Comment 3 Sergio Villar Senin 2010-10-14 08:37:49 PDT
Adding Darin also as he reviewed the patch
Comment 4 Martin Robinson 2010-10-14 08:43:29 PDT
Is it possible to instead make the libsoup Data URI implementation resilient against extra data in the URL?
Comment 5 Martin Robinson 2010-10-14 08:55:03 PDT
Looks like you've done that here: https://bugs.webkit.org/show_bug.cgi?id=47666 I suppose we should wait to see if that change fixes this failure.
Comment 6 Sergio Villar Senin 2010-10-14 09:00:27 PDT
(In reply to comment #4)
> Is it possible to instead make the libsoup Data URI implementation resilient against extra data in the URL?

Indeed it's possible, but if the test is wrong I guess we should fix that IMHO
Comment 7 Ryosuke Niwa 2010-10-14 09:37:21 PDT
Are you sure the original crash reproduces with the extra =?  It's quite possible that missing = was intentional because it's a crash test.  The test just needs to produce a crash, it doesn't need to be 'correct'.
Comment 8 Sergio Villar Senin 2010-10-14 10:20:48 PDT
(In reply to comment #7)
> Are you sure the original crash reproduces with the extra =?  It's quite possible that missing = was intentional because it's a crash test.  The test just needs to produce a crash, it doesn't need to be 'correct'.

Thx for the answer Ryosuke. I don't know the code that was causing the crash in detail but it seems that the extra "=" does not have to do with the original problem. Just pinging you to know if that was intentional or not, but I guess I can double-check that.
Comment 9 Ryosuke Niwa 2010-10-14 10:25:41 PDT
(In reply to comment #8)
> Thx for the answer Ryosuke. I don't know the code that was causing the crash in detail but it seems that the extra "=" does not have to do with the original problem. Just pinging you to know if that was intentional or not, but I guess I can double-check that.

Could you download the code BEFORE r66032 (http://trac.webkit.org/changeset/66032) and make sure WebKit crashes with your fix?
Comment 10 Alexey Proskuryakov 2010-10-14 12:38:59 PDT
Note that Safari (or more precisely, CFNetwork) is strict in this respect - "data:image/png;base64,iVBORw0KGrkJggg==" fails to load, while "data:image/png;base64,iVBORw0KGrkJggg=" loads fine (but then fails to render, since the data itself is of course broken). This matches Firefox.

So, I suspect that both this change and the change in bug 47666 are wrong. There is probably some difference in how failed data: load is reported by libsoup, which was making the layout test fail.
Comment 11 Ryosuke Niwa 2010-10-14 12:54:08 PDT
(In reply to comment #10)
> Note that Safari (or more precisely, CFNetwork) is strict in this respect - "data:image/png;base64,iVBORw0KGrkJggg==" fails to load, while "data:image/png;base64,iVBORw0KGrkJggg=" loads fine (but then fails to render, since the data itself is of course broken). This matches Firefox.
> 
> So, I suspect that both this change and the change in bug 47666 are wrong. There is probably some difference in how failed data: load is reported by libsoup, which was making the layout test fail.

Yeah, only if they cc-ed me in the bug 47666, I could have stopped them from making that change :( Or that I should have been more careful after I checked in the test.  Nonetheless, we should verify that this change won't "fix" the crash.
Comment 12 Sergio Villar Senin 2010-10-15 00:50:52 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Note that Safari (or more precisely, CFNetwork) is strict in this respect - "data:image/png;base64,iVBORw0KGrkJggg==" fails to load, while "data:image/png;base64,iVBORw0KGrkJggg=" loads fine (but then fails to render, since the data itself is of course broken). This matches Firefox.
> > 
> > So, I suspect that both this change and the change in bug 47666 are wrong. There is probably some difference in how failed data: load is reported by libsoup, which was making the layout test fail.
> 
> Yeah, only if they cc-ed me in the bug 47666, I could have stopped them from making that change :( Or that I should have been more careful after I checked in the test.  Nonetheless, we should verify that this change won't "fix" the crash.

Thx Ryosuke and Alexey for your comments. I'd most likely reopen 47666 in order to fix it properly. I just would like to know if there is any spec that mentions how to deal with invalid base64 data. I guess that the right behaviour is to fail the load as Safari does.