Bug 109287 - [GTK] Crash in webkitURIResponseSetCertificateInfo()
Summary: [GTK] Crash in webkitURIResponseSetCertificateInfo()
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: http://renevier.net/misc/webkit_10922...
Keywords:
: 109225 (view as bug list)
Depends on: 109566 110190
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-08 05:07 PST by Claudio Saavedra
Modified: 2013-02-19 09:51 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.79 KB, patch)
2013-02-08 05:19 PST, Claudio Saavedra
cgarcia: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
test page source (245 bytes, application/x-php)
2013-02-09 10:16 PST, arno.
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2013-02-08 05:07:28 PST
Stacktrace:

#0  0x00007ffff6313647 in webkitURIResponseSetCertificateInfo(_WebKitURIResponse*, WebKit::WebCertificateInfo*) ()
   from /home/claudio/git/gnome/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#1  0x00007ffff6321dc1 in webkitWebViewLoadChanged(_WebKitWebView*, WebKitLoadEvent) ()
   from /home/claudio/git/gnome/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#2  0x00007ffff63795e0 in WebKit::WebPageProxy:idCommitLoadForFrame(unsigned long, WTF:tring const&, bool, unsigned int, WebKit:latformCertificateInfo const&, CoreIPC::MessageDecoder&) ()

Quick analysis:

WebKitWebView's setCertificateToMainResource() is calling webkitURIResponseSetCertificateInfo() and passing an unchecked call to webkit_web_resource_get_response() as the WebKitURIResponse parameter. The docs for webkit_web_resource_get_response() tell that this function can return NULL but webkitURIResponseSetCertificateInfo() doesn't check for this and dereferences directly.

The quick fix would be not to call to webkitURIResponseSetCertificateInfo() if the webresource doesn't have yet a response, but I am not sure whether this is the right thing.
Comment 1 Claudio Saavedra 2013-02-08 05:19:27 PST
Created attachment 187294 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-08 05:23:26 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Build Bot 2013-02-08 06:00:46 PST
Comment on attachment 187294 [details]
Patch

Attachment 187294 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16427616

New failing tests:
http/tests/cache/cached-main-resource.html
Comment 4 Sergio Villar Senin 2013-02-08 07:40:51 PST
*** Bug 109225 has been marked as a duplicate of this bug. ***
Comment 5 arno. 2013-02-08 09:38:08 PST
(In reply to comment #0)
> 
> The quick fix would be not to call to webkitURIResponseSetCertificateInfo() if the webresource doesn't have yet a response, but I am not sure whether this is the right thing.


Then, when response arrives later, it would not get a certificate info. What are the implications of that ?

Would that work better by always registering a notify::response callback on the main response, and calling setCertificateToMainResource from there ?


Also, even with that patch, there still is problem:
at *third* load of a page with 304 code, the page does not load (try to go to http://localhost/tmp/crash.php at 3rd load, network panel in error console show that the request has been aborted). That is because webkitWebViewDecidePolicy receives the 304 response with a null mimetype the second time (but not the first time). That is probably another bug though.
Comment 6 Carlos Garcia Campos 2013-02-09 03:21:12 PST
(In reply to comment #0)
> Stacktrace:
> 
> #0  0x00007ffff6313647 in webkitURIResponseSetCertificateInfo(_WebKitURIResponse*, WebKit::WebCertificateInfo*) ()
>    from /home/claudio/git/gnome/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
> #1  0x00007ffff6321dc1 in webkitWebViewLoadChanged(_WebKitWebView*, WebKitLoadEvent) ()
>    from /home/claudio/git/gnome/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
> #2  0x00007ffff63795e0 in WebKit::WebPageProxy:idCommitLoadForFrame(unsigned long, WTF:tring const&, bool, unsigned int, WebKit:latformCertificateInfo const&, CoreIPC::MessageDecoder&) ()
> 
> Quick analysis:
> 
> WebKitWebView's setCertificateToMainResource() is calling webkitURIResponseSetCertificateInfo() and passing an unchecked call to webkit_web_resource_get_response() as the WebKitURIResponse parameter. The docs for webkit_web_resource_get_response() tell that this function can return NULL but webkitURIResponseSetCertificateInfo() doesn't check for this and dereferences directly.

webkit_we_resource_get_response can return NULL, but it should never happen when setCertificateToMainResource is called, how can I reproduce this crash?

> The quick fix would be not to call to webkitURIResponseSetCertificateInfo() if the webresource doesn't have yet a response, but I am not sure whether this is the right thing.
Comment 7 arno. 2013-02-09 10:09:52 PST
(In reply to comment #6)
> (In reply to comment #0)
> > Stacktrace:
> > 
> > #0  0x00007ffff6313647 in webkitURIResponseSetCertificateInfo(_WebKitURIResponse*, WebKit::WebCertificateInfo*) ()
> >    from /home/claudio/git/gnome/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
> > #1  0x00007ffff6321dc1 in webkitWebViewLoadChanged(_WebKitWebView*, WebKitLoadEvent) ()
> >    from /home/claudio/git/gnome/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
> > #2  0x00007ffff63795e0 in WebKit::WebPageProxy:idCommitLoadForFrame(unsigned long, WTF:tring const&, bool, unsigned int, WebKit:latformCertificateInfo const&, CoreIPC::MessageDecoder&) ()
> > 
> > Quick analysis:
> > 
> > WebKitWebView's setCertificateToMainResource() is calling webkitURIResponseSetCertificateInfo() and passing an unchecked call to webkit_web_resource_get_response() as the WebKitURIResponse parameter. The docs for webkit_web_resource_get_response() tell that this function can return NULL but webkitURIResponseSetCertificateInfo() doesn't check for this and dereferences directly.
> 
> webkit_we_resource_get_response can return NULL, but it should never happen when setCertificateToMainResource is called, how can I reproduce this crash?
> 
> > The quick fix would be not to call to webkitURIResponseSetCertificateInfo() if the webresource doesn't have yet a response, but I am not sure whether this is the right thing.
Comment 8 arno. 2013-02-09 10:11:30 PST
(In reply to comment #7)

> > webkit_we_resource_get_response can return NULL, but it should never happen when setCertificateToMainResource is called, how can I reproduce this crash?

Oups, looks like I copy/pasted the wrong url.
the crash happens in case of a 304 http code.
If you load http://renevier.net/misc/webkit_109225.php in MiniBrowser and reload it, it should crash.
Comment 9 arno. 2013-02-09 10:16:13 PST
Created attachment 187436 [details]
test page source
Comment 10 Carlos Garcia Campos 2013-02-12 04:54:21 PST
Comment on attachment 187294 [details]
Patch

This is not the right fix, as I said, that situation should never happen and it's actually a bug in WebCore, see bug https://bugs.webkit.org/show_bug.cgi?id=109566
Comment 11 Carlos Garcia Campos 2013-02-19 09:51:28 PST
This is fixed now since webkitURIResponseSetCertificateInfo() doesn't exist anymore.