Skip to content

CFAttributedString: fix allocation, use-after-free and OOB bugs#53

Open
DTW-Thalion wants to merge 1 commit into
gnustep:masterfrom
DTW-Thalion:fix/cfattributedstring-alloc
Open

CFAttributedString: fix allocation, use-after-free and OOB bugs#53
DTW-Thalion wants to merge 1 commit into
gnustep:masterfrom
DTW-Thalion:fix/cfattributedstring-alloc

Conversation

@DTW-Thalion

Copy link
Copy Markdown
Contributor

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant