CFTimeZone: retain timezones stored in the name cache (use-after-free)#67
Open
DTW-Thalion wants to merge 3 commits into
Open
CFTimeZone: retain timezones stored in the name cache (use-after-free)#67DTW-Thalion wants to merge 3 commits into
DTW-Thalion wants to merge 3 commits into
Conversation
CFTimeZoneCreate caches each created timezone in _kCFTimeZoneCache, but the cache was made with NULL value callbacks, so it did not retain the stored objects. CFTimeZoneCreate returns the cached entry with CFRetain(old); once the original owner releases that reference the timezone is deallocated while the cache still holds a dangling pointer to it, and the next lookup of the same name returns freed memory (a use-after-free in CFGetTypeID via the returned CFRetain). Give the cache kCFTypeDictionaryValueCallBacks so it retains the timezones it stores. There is no removal path (CFTimeZoneFinalize does not touch the cache), so the cache acts as an intern table holding one reference per distinct zone name for the process lifetime. The insert path returns the original creation reference, so the caller still owns the +1 it expects.
Create a named time zone, release the caller's only reference, then look the same name up again. When the cache stored zones without retaining them the second lookup returned and retained freed memory; under AddressSanitizer this aborts with a heap-use-after-free in CFGetTypeID/CFRetain. With the cache retaining its entries the test passes.
|
|
||
| /* Regression test for the time-zone name cache use-after-free. | ||
|
|
||
| CFTimeZoneCreate caches every created zone in the global name cache. |
There was a problem hiding this comment.
I don't think we want to keep this comment around. Its meaningless outside of this PR.
Contributor
Author
There was a problem hiding this comment.
Trimmed to a one-line description of what the test checks (50fbc99).
There was a problem hiding this comment.
No, remove the entire comment except for the first line "Regression test..."
Contributor
Author
There was a problem hiding this comment.
Done — the comment is now just /* Regression test for the time-zone name cache use-after-free. */ (ac6f02e).
Drop the PR-specific bug narrative from cache_retain.m and keep a short note of what the test checks.
DTW-Thalion
force-pushed
the
fix/cftimezone-cache-retain
branch
from
50fbc99 to
ac6f02e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CFTimeZoneCreatecaches every timezone it builds in_kCFTimeZoneCache, butthe cache is created with NULL value callbacks, so it does not retain the
stored objects:
A later call returns the cached entry via
CFRetain(old). Once the originalowner releases its reference, the timezone is deallocated while the cache
still holds a dangling pointer to it. The next lookup of the same name returns
freed memory, and the
CFRetainon that pointer dereferences it throughCFGetTypeID— a use-after-free.There is no removal path:
CFTimeZoneFinalizedoes not touch the cache, andnothing calls
CFDictionaryRemoveValueon it. So the only correct lifetime isfor the cache to own a reference to each entry.
Fix
Give the cache
kCFTypeDictionaryValueCallBacksso it retains the timezones itstores. The cache then behaves as an intern table holding one reference per
distinct zone name for the process lifetime (bounded by the number of zone
names). The insert path returns the original creation reference, so the caller
still receives the
+1it expects — no double-retain, no leaked callerreference.
Reproduction (ThreadSanitizer)
A multi-threaded harness that creates and releases timezones by name aborts with
a
heap-use-after-freeinCFGetTypeID(reached fromCFTimeZoneCreate'sCFRetainof a cached entry). With this change that use-after-free no longeroccurs.
Relationship to #64
This is a distinct bug from the cache-lock race in #64. #64 fixes the
unlocked
CFDictionaryGetValueracing a concurrent insert's rehash (abucket-array race inside the dictionary); this fixes the lifetime of the stored
objects. They are complementary — with both applied the harness above runs
fully clean under TSan (0 data races, 0 use-after-free, 0 SEGV).
Note: both touch the same
CFDictionaryCreateMutablecall, so whichever landsfirst leaves a trivial one-line rebase for the other (the
NULL→&kCFTypeDictionaryValueCallBacksargument on the relocated call).