Bug 54959

Summary:

MemoryCache::revalidationSucceeded accesses possibly freed object

Product:

WebKit

Reporter:

Fabrizio <fabrizio.machado>

Component:

Page Loading

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Trivial

CC:

ap, koivisto, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
don't access revalidatingResource without a check, since it may have been freed	none
don't access revalidatingResource without a check, since it may have been freed	none
fix style errors	ap: review-, ap: commit-queue-

Fabrizio

Reported 2011-02-22 07:54:11 PST

In MemoryCache::revalidationSucceeded(), revalidatingResource is blindly accessed after evict() has been called on it: ======================================= evict(revalidatingResource); ASSERT(!m_resources.get(resource->url())); m_resources.set(resource->url(), resource); resource->setInCache(true); resource->updateResponseAfterRevalidation(response); insertInLRUList(resource); int delta = resource->size(); if (resource->decodedSize() && resource->hasClients()) insertInLiveDecodedResourcesList(resource); if (delta) adjustSize(resource->hasClients(), delta); revalidatingResource->switchClientsToRevalidatedResource(); ======================================= An inspection of MemoryCache::evict(CachedResource* resource) shows that it may delete the resource: ======================================= if (resource->canDelete()) delete resource; ======================================= Therefore, if evict() has been called on revalidatingResource, then revalidatingResource must not be accessed without checking, since it may have been deleted.

Attachments
don't access revalidatingResource without a check, since it may have been freed (3.57 KB, patch) 2011-02-22 07:57 PST, Fabrizio	no flags	Details Formatted Diff Diff
don't access revalidatingResource without a check, since it may have been freed (1.66 KB, patch) 2011-02-22 07:59 PST, Fabrizio	no flags	Details Formatted Diff Diff
fix style errors (1.67 KB, patch) 2011-02-22 08:06 PST, Fabrizio	ap: review- ap: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Fabrizio

Comment 1 2011-02-22 07:57:02 PST

Created attachment 83316 [details] don't access revalidatingResource without a check, since it may have been freed

Fabrizio

Comment 2 2011-02-22 07:59:12 PST

Created attachment 83317 [details] don't access revalidatingResource without a check, since it may have been freed

WebKit Review Bot

Comment 3 2011-02-22 08:03:48 PST

Attachment 83317 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/loader/cache/MemoryCache.cpp:121: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/loader/cache/MemoryCache.cpp:122: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/loader/cache/MemoryCache.cpp:123: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/loader/cache/MemoryCache.cpp:124: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 4 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Fabrizio

Comment 4 2011-02-22 08:06:12 PST

Created attachment 83318 [details] fix style errors

Alexey Proskuryakov

Comment 5 2011-02-22 10:37:38 PST

Comment on attachment 83318 [details] fix style errors Even if evict() destroys revalidatingResource, the local variable value doesn't change, so this patch has no effect. Please describe the problem in more detail. Is this something that you observed in practice?

Note You need to log in before you can comment on or make changes to this bug.