CFAttributedString: fix allocation, use-after-free and OOB bugs#53
Open
DTW-Thalion wants to merge 1 commit into
Open
CFAttributedString: fix allocation, use-after-free and OOB bugs#53DTW-Thalion wants to merge 1 commit into
DTW-Thalion wants to merge 1 commit into
Conversation
CFMutableAttributedString was unusable: every operation corrupted memory.
* CFATTRIBUTESTRING_SIZE / CFMUTABLEATTRIBUTESTRING_SIZE computed the
instance's extra size as "sizeof(CFRuntimeClass) - sizeof(struct ...)"
instead of "sizeof(struct ...) - sizeof(CFRuntimeBase)", undersizing
the object so CFAttributedStringCreateMutable() wrote its own ivars
past the end of the allocation.
* CFAttributedStringGetBlankAttribute() created a dictionary, cached a
*copy* of it, released the original, and then dereferenced the
released original (use-after-free). Keep a reference to the cached
copy instead.
* InsertAttributesAtIndex() (grow) and RemoveAttributesAtIndex() (shrink)
passed the element count, not a byte count, to CFAllocatorReallocate()
and never updated _attribCap, so the attribute array was reallocated
far too small and overrun.
* CFAttributedStringCoalesce() compared each entry with its predecessor
starting at index range.location, reading element [-1] when the range
began at 0.
The attribute cache relies on CFBag being a counted collection, but
CFBagAddValue() did not increase the count of an already-present value
(GSHashTableAddValue only ever inserts new entries), so uncaching a shared
attribute -- e.g. the blank attribute -- freed it while it was still in
use. Add GSHashTableAddValueCounted() and use it from CFBagAddValue() so
the count tracks the number of references.
With these fixes the CFAttributedString tests pass (creation, attribute
get/set, and growing the attribute array past its initial capacity);
previously they crashed. A separate imbalance in the attribute cache's
reference accounting still leaks some cached attributes; that is left for
a follow-up.
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
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.
CFMutableAttributedString was unusable: every operation corrupted memory.
CFATTRIBUTESTRING_SIZE / CFMUTABLEATTRIBUTESTRING_SIZE computed the
instance's extra size as "sizeof(CFRuntimeClass) - sizeof(struct ...)"
instead of "sizeof(struct ...) - sizeof(CFRuntimeBase)", undersizing
the object so CFAttributedStringCreateMutable() wrote its own ivars
past the end of the allocation.
CFAttributedStringGetBlankAttribute() created a dictionary, cached a
copy of it, released the original, and then dereferenced the
released original (use-after-free). Keep a reference to the cached
copy instead.
InsertAttributesAtIndex() (grow) and RemoveAttributesAtIndex() (shrink)
passed the element count, not a byte count, to CFAllocatorReallocate()
and never updated _attribCap, so the attribute array was reallocated
far too small and overrun.
CFAttributedStringCoalesce() compared each entry with its predecessor
starting at index range.location, reading element [-1] when the range
began at 0.
The attribute cache relies on CFBag being a counted collection, but
CFBagAddValue() did not increase the count of an already-present value
(GSHashTableAddValue only ever inserts new entries), so uncaching a shared
attribute -- e.g. the blank attribute -- freed it while it was still in
use. Add GSHashTableAddValueCounted() and use it from CFBagAddValue() so
the count tracks the number of references.
With these fixes the CFAttributedString tests pass (creation, attribute
get/set, and growing the attribute array past its initial capacity);
previously they crashed. A separate imbalance in the attribute cache's
reference accounting still leaks some cached attributes; that is left for
a follow-up.