Bug 47661 – Invalid base64 data in orphaned-selection-crash-bug32823-2.html

• Status: RESOLVED
• Resolution: LATER
• Component: Tools / Tests

[Sergio Villar Senin]

That test includes this base64 encoded data

iVBORw0KGrkJggg==

which has 1 extra padding '='

[Sergio Villar Senin]

Created attachment 70724 [details]
Fix for the bug

Removes an invalid extra "=" padding character in the data URL

[Sergio Villar Senin]

Adding Ryosuke

[Sergio Villar Senin]

Adding Darin also as he reviewed the patch

[Martin Robinson]

Is it possible to instead make the libsoup Data URI implementation resilient against extra data in the URL?

[Martin Robinson]

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.

[Sergio Villar Senin]

(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

[Ryosuke Niwa]

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

[Sergio Villar Senin]

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

[Ryosuke Niwa]

(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?

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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

[Sergio Villar Senin]

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

[Ahmad Saleem]

I think this was workaround by following commit - https://github.com/WebKit/WebKit/commit/6a237ea157f017c19e1e12273af6bb4c1823050a

Do we need this fix now? Thanks!

[Ryosuke Niwa]

Looks like this issue sorted itself out.