Bug 82882 - [GTK] Invalid read from WebKit::DOMObjectCache::clearByFrame
Summary: [GTK] Invalid read from WebKit::DOMObjectCache::clearByFrame
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: 2012-04-02 04:16 PDT by Milan Crha
Modified: 2012-10-18 16:07 PDT (History)
4 users (show)

See Also:


Attachments
proposed webkit patch (934 bytes, patch)
2012-04-02 04:24 PDT, Milan Crha
eric: review-
Details | Formatted Diff | Diff
2012-10-18 Claudio Saavedra <csaavedra@igalia.com> (2.85 KB, patch)
2012-10-18 15:33 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Milan Crha 2012-04-02 04:16:39 PDT
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)
Comment 1 Milan Crha 2012-04-02 04:24:12 PDT
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);
        }
Comment 2 WebKit Review Bot 2012-05-31 08:10:32 PDT
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.
Comment 3 Eric Seidel (no email) 2012-08-22 15:44:34 PDT
Comment on attachment 135063 [details]
proposed webkit patch

Needs a ChangeLog.
Comment 4 Milan Crha 2012-08-22 23:42:17 PDT
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...
Comment 5 Xan Lopez 2012-10-17 04:39:17 PDT
(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?
Comment 6 Milan Crha 2012-10-18 00:03:21 PDT
(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
Comment 7 Xan Lopez 2012-10-18 02:56:58 PDT
(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.
Comment 8 Milan Crha 2012-10-18 09:09:46 PDT
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.
Comment 9 Claudio Saavedra 2012-10-18 15:33:35 PDT
Created attachment 169489 [details]
2012-10-18  Claudio Saavedra  <csaavedra@igalia.com>

[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 <mcrha@redhat.com>

        Prevent an invalid access to a pointer while clearing the DOM
        object cache.
        * bindings/gobject/DOMObjectCache.cpp:
        (WebKit::DOMObjectCache::clearByFrame): Prevent an invalid access.
Comment 10 Xan Lopez 2012-10-18 15:37:09 PDT
Comment on attachment 169489 [details]
2012-10-18  Claudio Saavedra  <csaavedra@igalia.com>

Looks good, thank you.
Comment 11 WebKit Review Bot 2012-10-18 16:07:07 PDT
Comment on attachment 169489 [details]
2012-10-18  Claudio Saavedra  <csaavedra@igalia.com>

Clearing flags on attachment: 169489

Committed r131820: <http://trac.webkit.org/changeset/131820>
Comment 12 WebKit Review Bot 2012-10-18 16:07:11 PDT
All reviewed patches have been landed.  Closing bug.