Bug 74970 – [Coverity] Address use-after-free report in MemoryCache

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

[Coverity] Address use-after-free report in MemoryCache

[Greg Billock]

Created attachment 120105 [details]
Patch

[Alexey Proskuryakov]

What does this use-after-free report mean? Is this just a false positive that you're shutting up?

[Greg Billock]

Basically, yes. Coverity is worried that evict(revalidatingResource) can delete revalidatingResource, and then it gets used below. But canDelete() is clearly false if isLoaded() is true. That's beyond Coverity's abilities, but usually that means humans have a hard time deciphering such facts as well. :-)

[Alexey Proskuryakov]

OK. Maybe that's worth a comment in code in addition to the ASSERT.

CC'ing some folks who worked on this code in case something is this function jumps out as incorrect once this assertion for documenting behavior is added.

[Greg Billock]

Created attachment 120191 [details]
Patch

[Greg Billock]

Added a comment.

[Alexey Proskuryakov]

Comment on attachment 120191 [details]
Patch

+    // The evict() call can delete revalidatingResource, which we then use
+    // below. Assert explicitly that this can't happen.

I'd state this differently:

    // Calling evict() can potentially delete the resource. This won't happen here because <...>

[Greg Billock]

Created attachment 120223 [details]
Patch

[Greg Billock]

Done.

[Eric Seidel (no email)]

Comment on attachment 120223 [details]
Patch

OK.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   daea471..578c1d3  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 103458 = daea471373d620a3fbc3e7dbc138f927d151a5c9
r103460 = 578c1d3f4da422dcdedb98687c619ba79bfeb36f
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167249 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp
CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


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

[WebKit Review Bot]

Comment on attachment 120223 [details]
Patch

Clearing flags on attachment: 120223

Committed r103513: <http://trac.webkit.org/changeset/103513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.