Fix infinite loop in NSCFDictionary/NSCFSet fast enumeration#73
Open
rupertdaniel wants to merge 1 commit into
Open
Fix infinite loop in NSCFDictionary/NSCFSet fast enumeration#73rupertdaniel wants to merge 1 commit into
rupertdaniel wants to merge 1 commit into
Conversation
-[NSCFDictionary countByEnumeratingWithState:objects:count:] and the NSCFSet equivalent created a fresh enumerator on every call. The for/in protocol invokes this method repeatedly until it returns 0, relying on NSFastEnumerationState to carry progress between calls; because a new enumerator restarted from the beginning each time, the method never returned 0 and enumeration looped forever over the first batch. Iterate the backing GSHashTable directly instead, keeping the resume bucket index in state->extra[0]. Add GSHashTableGetKeysFromCursor(), a resumable key iterator that reads live buckets in place, so nothing is allocated per call and there is no snapshot to leak.
Member
|
@HendrikHuebner do you mind reviewing this one as well? We ran into this in our app. |
HendrikHuebner
left a comment
There was a problem hiding this comment.
LGTM. Do we have any tests for this?
Comment on lines
+118
to
+120
| * bucket *cursor, advances *cursor past them, and returns the number written | ||
| * (0 once the table is exhausted). Reads live buckets in place and allocates | ||
| * nothing, so it is suitable as a fast-enumeration primitive. |
There was a problem hiding this comment.
Suggested change
| * bucket *cursor, advances *cursor past them, and returns the number written | |
| * (0 once the table is exhausted). Reads live buckets in place and allocates | |
| * nothing, so it is suitable as a fast-enumeration primitive. | |
| * bucket *cursor, advances *cursor past them, and returns the number of written keys | |
| * (0 once the table is exhausted). |
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.
Summary
-[NSCFDictionary countByEnumeratingWithState:objects:count:]and theNSCFSetequivalent created a new enumerator on every call:The
for/infast-enumeration protocol invokes this method repeatedly until it returns 0, relying onNSFastEnumerationStateto carry progress between calls. Because a brand-new enumerator restarts from the beginning every time, the method never returns 0 — so enumerating any toll-free-bridgedCFDictionary/CFSet(e.g. one created withCFDictionaryCreateMutable) loops forever over the first batch of elements.Fix
Iterate the backing
GSHashTabledirectly, keeping the resume bucket index instate->extra[0]. A new private helperGSHashTableGetKeysFromCursor()reads live buckets in place, so there is no per-call allocation and no snapshot to free/leak. This mirrors the cursor-in-extra[]technique gnustep-base's ownGSDictionaryuses for fast enumeration.Source/GSHashTable.{c,h}— addGSHashTableGetKeysFromCursor().Source/NSCFDictionary.m,Source/NSCFSet.m— implementcountByEnumeratingWithState:objects:count:on top of it.NSFastEnumerationStatestays confined to the.mbridge files; the C core gains only a plainCFIndex/const void **iterator, so no Foundation dependency is introduced intoGSHashTable.Testing
Built with the MSVC toolchain and verified that
for (id key in dict)over aCFDictionaryCreateMutable-created dictionary now enumerates each element once and terminates, where it previously looped indefinitely.