Bug 82882 – [GTK] Invalid read from WebKit::DOMObjectCache::clearByFrame

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Milan Crha]

I run Evolution under valgrind today, and because it adapted WebkitGtk as its mailer renderer, I observed these valgrind warnings (see below). This is with webkitgtk-1.8.0 tarball release.

The first is when opening a message in a separate window in evolution (double click in a message list).
The second is when closing the message window.

==3268== Thread 1:
==3268== Invalid read of size 4
==3268== at 0x7E93DF0: WebKit::DOMObjectCache::clearByFrame(WebCore::Frame*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x83ED832: WebCore::FrameLoader::commitProvisionalLoad() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x83D7819: WebCore::DocumentLoader::commitLoad(char const*, int) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x841F027: WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x840A4D4: WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x841E4DD: WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x8546E19: WebCore::readCallback(_GObject*, _GAsyncResult*, void*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0xA3A37F0: async_ready_callback_wrapper (ginputstream.c:470)
==3268== by 0xA3BBB7A: g_simple_async_result_complete (gsimpleasyncresult.c:767)
==3268== by 0xA3BBBAC: complete_in_idle_cb (gsimpleasyncresult.c:779)
==3268== by 0xAD76800: g_idle_dispatch (gmain.c:4634)
==3268== by 0xAD740AA: g_main_dispatch (gmain.c:2515)
==3268== by 0xAD74D6B: g_main_context_dispatch (gmain.c:3052)
==3268== by 0xAD74F4E: g_main_context_iterate (gmain.c:3123)
==3268== by 0xAD75377: g_main_loop_run (gmain.c:3317)
==3268== by 0x39E4F517FC: gtk_main (gtkmain.c:1362)
==3268== by 0x403603: main (main.c:681)
==3268== Address 0x34bfb760 is 16 bytes inside a block of size 24 free'd
==3268== at 0x4A0662E: free (vg_replace_malloc.c:366)
==3268== by 0xAD7C332: standard_free (gmem.c:98)
==3268== by 0xAD7C4F5: g_free (gmem.c:252)
==3268== by 0xAD945D7: g_slice_free1 (gslice.c:1111)
==3268== by 0x7E93FD7: WebKit::DOMObjectCache::forget(void*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7EE1DA9: webkit_dom_document_finalize(_GObject*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0xA8EA32C: g_object_unref (gobject.c:3018)
==3268== by 0x7E93E20: WebKit::DOMObjectCache::clearByFrame(WebCore::Frame*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x83ED832: WebCore::FrameLoader::commitProvisionalLoad() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x83D7819: WebCore::DocumentLoader::commitLoad(char const*, int) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x841F027: WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x840A4D4: WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x841E4DD: WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x8546E19: WebCore::readCallback(_GObject*, _GAsyncResult*, void*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0xA3A37F0: async_ready_callback_wrapper (ginputstream.c:470)
==3268== by 0xA3BBB7A: g_simple_async_result_complete (gsimpleasyncresult.c:767)
==3268== by 0xA3BBBAC: complete_in_idle_cb (gsimpleasyncresult.c:779)
==3268== by 0xAD76800: g_idle_dispatch (gmain.c:4634)
==3268== by 0xAD740AA: g_main_dispatch (gmain.c:2515)
==3268== by 0xAD74D6B: g_main_context_dispatch (gmain.c:3052)
==3268== by 0xAD74F4E: g_main_context_iterate (gmain.c:3123)
==3268== by 0xAD75377: g_main_loop_run (gmain.c:3317)
==3268== by 0x39E4F517FC: gtk_main (gtkmain.c:1362)
==3268== by 0x403603: main (main.c:681)
 
 
==3268== Invalid read of size 4
==3268== at 0x7E93DF0: WebKit::DOMObjectCache::clearByFrame(WebCore::Frame*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7EBC301: webkit_web_frame_core_frame_gone (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7EA44CC: WebKit::FrameLoaderClient::frameLoaderDestroyed() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x83E9B61: WebCore::FrameLoader::~FrameLoader() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x8478F3B: WebCore::Frame::~Frame() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x84963EB: WebCore::Page::~Page() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7ED1347: webkit_web_view_dispose(_GObject*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x57F8A16: web_view_dispose (e-web-view.c:796)
==3268== by 0x1682BAB6: mail_display_dispose (e-mail-display.c:318)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4FB2E7D: gtk_scrolled_window_forall (gtkscrolledwindow.c:1265)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==3268== by 0xA8DE7C8: g_type_class_meta_marshal (gclosure.c:970)
==3268== by 0xA8DE113: g_closure_invoke (gclosure.c:777)
==3268== by 0xA8FAAE4: signal_emit_unlocked_R (gsignal.c:3663)
==3268== by 0xA8F9966: g_signal_emit_valist (gsignal.c:3296)
==3268== by 0xA8F9EAC: g_signal_emit (gsignal.c:3352)
==3268== by 0x39E50895CD: gtk_widget_dispose (gtkwidget.c:10666)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4E8B923: gtk_box_forall (gtkbox.c:1856)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==3268== by 0xA8DE7C8: g_type_class_meta_marshal (gclosure.c:970)
==3268== by 0xA8DE113: g_closure_invoke (gclosure.c:777)
==3268== by 0xA8FAAE4: signal_emit_unlocked_R (gsignal.c:3663)
==3268== by 0xA8F9966: g_signal_emit_valist (gsignal.c:3296)
==3268== by 0xA8F9EAC: g_signal_emit (gsignal.c:3352)
==3268== by 0x39E50895CD: gtk_widget_dispose (gtkwidget.c:10666)
==3268== by 0x57E6783: preview_pane_dispose (e-preview-pane.c:143)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4E8B923: gtk_box_forall (gtkbox.c:1856)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==3268== by 0xA8DE7C8: g_type_class_meta_marshal (gclosure.c:970)
==3268== by 0xA8DE113: g_closure_invoke (gclosure.c:777)
==3268== by 0xA8FAAE4: signal_emit_unlocked_R (gsignal.c:3663)
==3268== by 0xA8F9966: g_signal_emit_valist (gsignal.c:3296)
==3268== by 0xA8F9EAC: g_signal_emit (gsignal.c:3352)
==3268== by 0x39E50895CD: gtk_widget_dispose (gtkwidget.c:10666)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==3268== by 0xA8DE7C8: g_type_class_meta_marshal (gclosure.c:970)
==3268== by 0xA8DE113: g_closure_invoke (gclosure.c:777)
==3268== by 0xA8FAAE4: signal_emit_unlocked_R (gsignal.c:3663)
==3268== by 0xA8F9966: g_signal_emit_valist (gsignal.c:3296)
==3268== by 0xA8F9EAC: g_signal_emit (gsignal.c:3352)
==3268== by 0x39E50895CD: gtk_widget_dispose (gtkwidget.c:10666)
==3268== by 0x16829FAD: mail_browser_dispose (e-mail-browser.c:533)
==3268== Address 0x1bebfa10 is 16 bytes inside a block of size 24 free'd
==3268== at 0x4A0662E: free (vg_replace_malloc.c:366)
==3268== by 0xAD7C332: standard_free (gmem.c:98)
==3268== by 0xAD7C4F5: g_free (gmem.c:252)
==3268== by 0xAD945D7: g_slice_free1 (gslice.c:1111)
==3268== by 0x7E93FD7: WebKit::DOMObjectCache::forget(void*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7EE1DA9: webkit_dom_document_finalize(_GObject*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0xA8EA32C: g_object_unref (gobject.c:3018)
==3268== by 0x7E93E20: WebKit::DOMObjectCache::clearByFrame(WebCore::Frame*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7EBC301: webkit_web_frame_core_frame_gone (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7EA44CC: WebKit::FrameLoaderClient::frameLoaderDestroyed() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x83E9B61: WebCore::FrameLoader::~FrameLoader() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x8478F3B: WebCore::Frame::~Frame() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x84963EB: WebCore::Page::~Page() (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x7ED1347: webkit_web_view_dispose(_GObject*) (in /build/local/lib/libwebkitgtk-3.0.so.0.13.1)
==3268== by 0x57F8A16: web_view_dispose (e-web-view.c:796)
==3268== by 0x1682BAB6: mail_display_dispose (e-mail-display.c:318)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4FB2E7D: gtk_scrolled_window_forall (gtkscrolledwindow.c:1265)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==3268== by 0xA8DE7C8: g_type_class_meta_marshal (gclosure.c:970)
==3268== by 0xA8DE113: g_closure_invoke (gclosure.c:777)
==3268== by 0xA8FAAE4: signal_emit_unlocked_R (gsignal.c:3663)
==3268== by 0xA8F9966: g_signal_emit_valist (gsignal.c:3296)
==3268== by 0xA8F9EAC: g_signal_emit (gsignal.c:3352)
==3268== by 0x39E50895CD: gtk_widget_dispose (gtkwidget.c:10666)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4E8B923: gtk_box_forall (gtkbox.c:1856)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==3268== by 0xA8DE7C8: g_type_class_meta_marshal (gclosure.c:970)
==3268== by 0xA8DE113: g_closure_invoke (gclosure.c:777)
==3268== by 0xA8FAAE4: signal_emit_unlocked_R (gsignal.c:3663)
==3268== by 0xA8F9966: g_signal_emit_valist (gsignal.c:3296)
==3268== by 0xA8F9EAC: g_signal_emit (gsignal.c:3352)
==3268== by 0x39E50895CD: gtk_widget_dispose (gtkwidget.c:10666)
==3268== by 0x57E6783: preview_pane_dispose (e-preview-pane.c:143)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4E8B923: gtk_box_forall (gtkbox.c:1856)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==3268== by 0xA8DE7C8: g_type_class_meta_marshal (gclosure.c:970)
==3268== by 0xA8DE113: g_closure_invoke (gclosure.c:777)
==3268== by 0xA8FAAE4: signal_emit_unlocked_R (gsignal.c:3663)
==3268== by 0xA8F9966: g_signal_emit_valist (gsignal.c:3296)
==3268== by 0xA8F9EAC: g_signal_emit (gsignal.c:3352)
==3268== by 0x39E50895CD: gtk_widget_dispose (gtkwidget.c:10666)
==3268== by 0xA8E52B9: g_object_run_dispose (gobject.c:1061)
==3268== by 0x39E4ECE1CA: gtk_container_destroy (gtkcontainer.c:1370)
==3268== by 0xA8E10A3: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)

[Milan Crha]

Created attachment 135063 [details]
proposed webkit patch

for webkit;

Basically, if everything goes correctly then the weakRefNotify() is never called, thus objectDead is always FALSE, thus the 'while' dereferences the 'data' which is already freed in the last loop cycle. I tested with valgrind and it is happy with this patch included.

Just for a reference, here's the code I talk about (without patch applied):

        gboolean objectDead = FALSE;
        g_object_weak_ref(data->object, weakRefNotify, &objectDead);
        // We need to check objectDead first, otherwise the cache data
        // might be garbage already.
        while (!objectDead && data->timesReturned > 0) {
            // If this is the last unref we are going to do,
            // disconnect the weak ref. We cannot do it afterwards
            // because the object might be dead at that point.
            if (data->timesReturned == 1)
                g_object_weak_unref(data->object, weakRefNotify, &objectDead);
            data->timesReturned--;
            g_object_unref(data->object);
        }

[WebKit Review Bot]

Attachment 135063 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 135063 [details]
proposed webkit patch

Needs a ChangeLog.

[Milan Crha]

Are you kidding me? This almost two-liner fixes not-so-obvious error in the code which is still there in 1.9.6, waiting for a review for almost 5 months, and you reject it because of missing ChangeLog? Come on...

[Xan Lopez]

(In reply to comment #4)
> Are you kidding me? This almost two-liner fixes not-so-obvious error in the code which is still there in 1.9.6, waiting for a review for almost 5 months, and you reject it because of missing ChangeLog? Come on...

Writing a ChangeLog should be a matter of 5 minutes. If you don't do it someone else has to, I don't see what's so shocking about being strict in this regard.

About the patch, one question:

You mention "if everything goes correctly the weakRefNotify is never called", because we disable it before doing the last unref. Right? In that same block we'l decrease timesReturned, so while objectDead will still be FALSE timesReturned should be 0 (since we only do the last unref when it's 1). So we shouldn't really enter the loop again. I guess I'm missing something because there's indeed a valgrind warning, so what am I getting wrong?

[Milan Crha]

(In reply to comment #5)
> (In reply to comment #4)
> > Are you kidding me? This almost two-liner fixes not-so-obvious error in the code which is still there in 1.9.6, waiting for a review for almost 5 months, and you reject it because of missing ChangeLog? Come on...
> 
> Writing a ChangeLog should be a matter of 5 minutes. If you don't do it someone else has to, I don't see what's so shocking about being strict in this regard.

It's "shocking" when asked doing so after 5 months of waiting for a review. I moved quite far away from this during those 5 months.

> About the patch, one question:
> 
> You mention "if everything goes correctly the weakRefNotify is never called", because we disable it before doing the last unref. Right?

You didn't quote the right passage of the explanation from comment #1. (See below.)

> In that same block we'l decrease timesReturned, so while objectDead will still be FALSE timesReturned should be 0 (since we only do the last unref when it's 1). So we shouldn't really enter the loop again. I guess I'm missing something because there's indeed a valgrind warning, so what am I getting wrong?

That's what I thought about this too, the issue is not obvious on the first look.

The issue is not with g_object_unref() call, valgrind doesn't claim on it, the issue is with the 'data' structure, which is freed together with data->object. Following your 'while' steps (see code in comment #1) it's like this:
   : let data->timesReturned be 1
   : objectDead is FALSE
   the g_object_weak_unref() is called
   data->timesReturned is decreased to 0
   g_object_unref() is called
   : so far so good, 'data->object' is freed,
   : together with 'data' itself, but objectDead is still FALSE
   : then it comes to the 'while' clause
   while (!objectDead && data->timesReturned > 0)
   : and because objectDead is FALSE, and 'data' is freed, then it dereferences freed memory

[Xan Lopez]

(In reply to comment #6)
> The issue is not with g_object_unref() call, valgrind doesn't claim on it, the issue is with the 'data' structure, which is freed together with data->object. Following your 'while' steps (see code in comment #1) it's like this:
>    : let data->timesReturned be 1
>    : objectDead is FALSE
>    the g_object_weak_unref() is called
>    data->timesReturned is decreased to 0
>    g_object_unref() is called
>    : so far so good, 'data->object' is freed,
>    : together with 'data' itself, but objectDead is still FALSE
>    : then it comes to the 'while' clause
>    while (!objectDead && data->timesReturned > 0)
>    : and because objectDead is FALSE, and 'data' is freed, then it dereferences freed memory

Ah, indeed. 'data' is freed in the object's ::finalize, which calls the DOMObjectCache function 'forget'. Tricky.

So I think the patch is OK, but I'd put an extra comment explaining what's going on here, since it's not really trivial to see. If you can do that plus a ChangeLog I'll be happy to r+, thanks a lot for figuring this out.

[Milan Crha]

I'm afraid it's as hard to spot as to explain, at least in a comment in the code. I would do the ChangeLog, but I do not have checkout of webkit handy, I really moved elsewhere between those 5 months. I'm sorry.

[Claudio Saavedra]

Created attachment 169489 [details]
2012-10-18  Claudio Saavedra  <[e-mail]>

[GTK] Invalid read from WebKit::DOMObjectCache::clearByFrame
        https://bugs.webkit.org/show_bug.cgi?id=82882

        Reviewed by NOBODY (OOPS!).

        Based on a patch by Milan Crha <[e-mail]>

        Prevent an invalid access to a pointer while clearing the DOM
        object cache.
        * bindings/gobject/DOMObjectCache.cpp:
        (WebKit::DOMObjectCache::clearByFrame): Prevent an invalid access.

[Xan Lopez]

Comment on attachment 169489 [details]
2012-10-18  Claudio Saavedra  <[e-mail]>

Looks good, thank you.

[WebKit Review Bot]

Comment on attachment 169489 [details]
2012-10-18  Claudio Saavedra  <[e-mail]>

Clearing flags on attachment: 169489

Committed r131820: <http://trac.webkit.org/changeset/131820>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.