Skip to content

GSHashTable: do not count duplicate keys when creating from an array#74

Open
DTW-Thalion wants to merge 1 commit into
gnustep:masterfrom
DTW-Thalion:fix/gshashtable-create-dedup
Open

GSHashTable: do not count duplicate keys when creating from an array#74
DTW-Thalion wants to merge 1 commit into
gnustep:masterfrom
DTW-Thalion:fix/gshashtable-create-dedup

Conversation

@DTW-Thalion

Copy link
Copy Markdown
Contributor

Problem

GSHashTableCreate builds the table from the input array by calling GSHashTableAddKeyValuePair and incrementing _count for every element, with no check for a key that is already present:

bucket = GSHashTableFindBucket (new, keys[idx], _kGSHashTableInsert);
GSHashTableAddKeyValuePair (new, bucket, keys[idx], values[idx]);
new->_count += 1;

For a duplicate key, GSHashTableFindBucket returns the existing bucket, but _count is still incremented, so it ends up larger than the number of stored buckets. Since GSHashTableGetCount returns _count while GSHashTableGetKeysAndValues emits one entry per occupied bucket, the two disagree:

  • CFSetGetCount / CFDictionaryGetCount over-report (e.g. CFSetCreate of {a, b, a} reports 3).
  • CFSetGetValues (and the dictionary/bag equivalents) then read past the filled buckets into uninitialised memory.

This affects CFSetCreate, CFDictionaryCreate and CFBagCreate, which all share this path.

Fix

Guard the insert with the same bucket->count <= 0 check the mutable GSHashTableAddValue path already uses, so a key that is already present is not added or counted again. Creation now behaves consistently with incremental insertion.

Tests

Tests/CFSet/create_duplicates.m and Tests/CFDictionary/create_duplicates.m create a container from an array containing a repeated key and check the count. The dictionary test fails before this change (count 2) and passes after (count 1); both pass after the fix.

GSHashTableCreate added every element of the input array and incremented
_count each time without checking whether the key was already present, so a
repeated key inflated the count past the number of stored buckets.  That made
CFSetGetCount / CFDictionaryGetCount over-report, and left CFSetGetValues
reading uninitialised slots.  Guard the insert with the same bucket->count
check the mutable GSHashTableAddValue path already uses.

Adds Tests/CFSet/create_duplicates.m and Tests/CFDictionary/create_duplicates.m.
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