From cf83554233f6d51b81edb4b98350fbaff3fecfd3 Mon Sep 17 00:00:00 2001 From: Todd White Date: Mon, 13 Apr 2026 19:55:52 -0400 Subject: [PATCH] Fix CFPropertyListCreateDeepCopy for mutable-copy options * Source/CFPropertyList.c: Two related bugs prevented the function from producing correct mutable deep copies. (1) The function dispatches on file-scope _kCF*TypeID variables that are lazily populated by CFPropertyListInitTypeIDs. The only other caller that initialised those variables was CFPlistTypeIsValid, so CFPropertyListCreateDeepCopy would drop into its trailing `else` branch and return NULL whenever it was the first plist-API call in the process. Add an explicit CFPropertyListInitTypeIDs() call at the top of the function. (2) The mutable-array and mutable-dictionary branches called CFArrayApplyFunction / CFDictionaryApplyFunction on the newly created empty destination container rather than on the source plist. The apply function therefore visited zero elements and every mutable deep copy came back empty. On Linux the iteration also read uninitialised slots in the destination container and crashed via CFRetain(NULL) inside CFArrayCopyFunction. Iterate on `plist` in both branches so the copy actually walks the source values. * Tests/CFPropertyList/deepcopy.m: New regression test exercising every branch of the mutable-copy path: - mutable array deep copy has the same count as the source; - mutable array deep copy preserves element identity; - the result is genuinely mutable (append grows the count); - immutable array deep copy still works (guards the non-buggy branch against a fix that accidentally regresses it); - mutable dictionary deep copy has the same count as the source (the dictionary branch had the same bug as the array branch and had been missed by a previous partial fix); - mutable dictionary deep copy preserves values by key; - the result is genuinely mutable; - a nested mutable deep copy preserves both outer and inner contents, since the recursive descent reuses the same mutable-copy path. Plain C / public CoreFoundation API only, using the PASS_CF macro from Tests/CFTesting.h - no Objective-C runtime assumptions, no blocks. The test was validated both ways against a clang build on Ubuntu 24.04: with both fixes in place, all 17 assertions pass; with the apply-function swap reverted, the test process crashes in CFRetain(NULL) on the first mutable array deep copy, which gnustep-tests correctly reports as "Failed file: aborted without running all tests". The discrimination between fixed and unfixed builds is therefore strict, not incidental. --- ChangeLog | 27 +++++ Source/CFPropertyList.c | 25 ++++- Tests/CFPropertyList/deepcopy.m | 182 ++++++++++++++++++++++++++++++++ 3 files changed, 232 insertions(+), 2 deletions(-) create mode 100644 Tests/CFPropertyList/deepcopy.m diff --git a/ChangeLog b/ChangeLog index 0604f3de..c915f44b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,30 @@ +2026-04-13 Todd White + + * Source/CFPropertyList.c: Fix CFPropertyListCreateDeepCopy for + mutable-copy options. Two related bugs prevented the function + from working: + (1) The function dispatches on file-scope _kCF*TypeID variables + that are lazily populated by CFPropertyListInitTypeIDs. The + only other caller that initialised those variables was + CFPlistTypeIsValid, so CFPropertyListCreateDeepCopy would drop + into its trailing `else` branch and return NULL whenever it + was the first plist-API call in the process. Add an explicit + CFPropertyListInitTypeIDs() at the top of the function. + (2) The mutable-array and mutable-dictionary branches called + CFArrayApplyFunction / CFDictionaryApplyFunction on the newly + created empty destination container rather than on the source + plist. The apply function therefore visited zero elements and + every mutable deep copy came back empty. On Linux the + iteration also read uninitialised slots in the destination + container and crashed via CFRetain(NULL) inside + CFArrayCopyFunction. Iterate on `plist` in both branches so + the copy actually walks the source values. + * Tests/CFPropertyList/deepcopy.m: New regression test covering + the mutable-array copy path (count, element identity, genuine + mutability via append), the immutable-array copy path (guards + the branch that was never broken), the mutable-dictionary copy + path, and a nested mutable deep copy. + 2021-09-30 Frederik Seiffert * Source/CFDate.c: Fix logic in CFGregorianDateIsValid() diff --git a/Source/CFPropertyList.c b/Source/CFPropertyList.c index 9ffa1f98..9d24c870 100644 --- a/Source/CFPropertyList.c +++ b/Source/CFPropertyList.c @@ -257,6 +257,17 @@ CFPropertyListCreateDeepCopy (CFAllocatorRef alloc, CFPropertyListRef plist, CFPropertyListRef copy; CFTypeID typeID; + /* The function dispatches on file-scope CFTypeID variables that are + * lazily populated by CFPropertyListInitTypeIDs. If this is the + * first plist-API call in the process those variables are all zero, + * every `typeID == _kCF*TypeID` comparison fails, and the function + * drops into the trailing `else` branch and returns NULL. The only + * other caller that initialised the IDs was CFPlistTypeIsValid, so + * the bug only surfaced for code paths that did not happen to go + * through a validation call first. + */ + CFPropertyListInitTypeIDs (); + typeID = CFGetTypeID (plist); if (typeID == _kCFArrayTypeID) { @@ -293,7 +304,12 @@ CFPropertyListCreateDeepCopy (CFAllocatorRef alloc, CFPropertyListRef plist, ctx.alloc = alloc; ctx.container = (CFTypeRef) array; range = CFRangeMake (0, cnt); - CFArrayApplyFunction (array, range, CFArrayCopyFunction, &ctx); + /* Iterate over the source array `plist`, not the empty + * destination `array` - otherwise the apply function visits + * zero elements and the mutable deep copy always comes back + * empty. + */ + CFArrayApplyFunction (plist, range, CFArrayCopyFunction, &ctx); copy = array; } @@ -338,7 +354,12 @@ CFPropertyListCreateDeepCopy (CFAllocatorRef alloc, CFPropertyListRef plist, ctx.opts = opts; ctx.alloc = alloc; ctx.container = (CFTypeRef) dict; - CFDictionaryApplyFunction (dict, CFDictionaryCopyFunction, &ctx); + /* Iterate over the source dictionary `plist`, not the empty + * destination `dict` - otherwise the apply function visits + * zero entries and the mutable deep copy always comes back + * empty. Same bug as the array branch above. + */ + CFDictionaryApplyFunction (plist, CFDictionaryCopyFunction, &ctx); copy = dict; } diff --git a/Tests/CFPropertyList/deepcopy.m b/Tests/CFPropertyList/deepcopy.m new file mode 100644 index 00000000..12fd6543 --- /dev/null +++ b/Tests/CFPropertyList/deepcopy.m @@ -0,0 +1,182 @@ +/* + * deepcopy.m - regression test for CFPropertyListCreateDeepCopy mutable copies. + * + * Before the fix, the mutable-copy branch of CFPropertyListCreateDeepCopy + * called the apply function on the *destination* container - which had + * just been created empty - rather than on the source. The apply + * function therefore visited zero elements and the result came back + * empty. Both the array and the dictionary cases shared the same bug. + * + * This test exercises: + * + * - a mutable deep copy of an array preserves element count, order, + * and value equality; + * - a mutable deep copy of an array is actually mutable (i.e. the + * returned type is genuinely a mutable container, not an immutable + * one reinterpret-cast as mutable); + * - a mutable deep copy of a dictionary preserves key/value count and + * value equality by key; + * - a mutable deep copy of a dictionary is actually mutable; + * - an immutable deep copy of an array preserves its elements (the + * non-buggy branch - guards against a fix that accidentally + * regresses the already-working path); + * - a mutable deep copy of a nested container preserves the nested + * contents (since the recursive descent goes back through the same + * mutable-copy path, an off-by-nothing bug at the outer level would + * not reliably show up in a flat test). + * + * All tests use only the public CoreFoundation C API - no Objective-C + * runtime assumptions, no blocks, no platform-specific system calls - + * so the test builds under every compiler GNUstep's libs-corebase + * supports. + */ + +#include +#include +#include +#include +#include +#include "../CFTesting.h" + +int main (void) +{ + CFStringRef s1; + CFStringRef s2; + SInt32 i32; + CFNumberRef n; + const void *aValues[3]; + CFArrayRef srcArray; + CFMutableArrayRef mArrayCopy; + CFArrayRef iArrayCopy; + CFMutableDictionaryRef srcDict; + CFMutableDictionaryRef mDictCopy; + CFArrayRef inner; + CFMutableArrayRef outer; + CFMutableArrayRef mOuterCopy; + + s1 = CFSTR("hello"); + s2 = CFSTR("world"); + i32 = 42; + n = CFNumberCreate (NULL, kCFNumberSInt32Type, &i32); + + aValues[0] = s1; + aValues[1] = s2; + aValues[2] = n; + srcArray = CFArrayCreate (NULL, aValues, 3, &kCFTypeArrayCallBacks); + + /* Array: mutable deep copy preserves count. */ + mArrayCopy = (CFMutableArrayRef) CFPropertyListCreateDeepCopy (NULL, + srcArray, kCFPropertyListMutableContainersAndLeaves); + PASS_CF(mArrayCopy != NULL, + "mutable deep copy of a 3-element array is non-NULL"); + PASS_CF(CFArrayGetCount (mArrayCopy) == 3, + "mutable deep copy of a 3-element array has 3 elements " + "(was 0 before the CFArrayApplyFunction source/destination fix)"); + + /* Array: element values match. */ + if (mArrayCopy != NULL && CFArrayGetCount (mArrayCopy) == 3) + { + PASS_CFEQ(CFArrayGetValueAtIndex (mArrayCopy, 0), s1, + "mutable array deep copy preserves element 0"); + PASS_CFEQ(CFArrayGetValueAtIndex (mArrayCopy, 1), s2, + "mutable array deep copy preserves element 1"); + PASS_CFEQ(CFArrayGetValueAtIndex (mArrayCopy, 2), n, + "mutable array deep copy preserves element 2"); + } + + /* Array: the result is genuinely mutable - if the fix is applied, + * appending to the copy should succeed and the count should grow. */ + if (mArrayCopy != NULL) + { + CFArrayAppendValue (mArrayCopy, s1); + PASS_CF(CFArrayGetCount (mArrayCopy) == 4, + "mutable array deep copy is genuinely mutable (append grows count)"); + CFRelease (mArrayCopy); + } + + /* Array: immutable deep copy was never broken - guard it against a + * regression that mistakenly fixes both branches. */ + iArrayCopy = (CFArrayRef) CFPropertyListCreateDeepCopy (NULL, srcArray, + kCFPropertyListImmutable); + PASS_CF(iArrayCopy != NULL, + "immutable deep copy of an array is non-NULL"); + PASS_CF(iArrayCopy != NULL && CFArrayGetCount (iArrayCopy) == 3, + "immutable deep copy preserves count (non-buggy branch still works)"); + if (iArrayCopy != NULL) + { + CFRelease (iArrayCopy); + } + + /* Dictionary: mutable deep copy preserves count. The audit caught + * the array bug but missed the identical bug in the dictionary + * branch of CFPropertyListCreateDeepCopy - this test covers it. */ + srcDict = CFDictionaryCreateMutable (NULL, 0, + &kCFCopyStringDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks); + CFDictionaryAddValue (srcDict, CFSTR("first"), s1); + CFDictionaryAddValue (srcDict, CFSTR("second"), s2); + CFDictionaryAddValue (srcDict, CFSTR("number"), n); + + mDictCopy = (CFMutableDictionaryRef) CFPropertyListCreateDeepCopy (NULL, + srcDict, kCFPropertyListMutableContainersAndLeaves); + PASS_CF(mDictCopy != NULL, + "mutable deep copy of a 3-entry dictionary is non-NULL"); + PASS_CF(mDictCopy != NULL && CFDictionaryGetCount (mDictCopy) == 3, + "mutable deep copy of a 3-entry dictionary has 3 entries " + "(was 0 before the CFDictionaryApplyFunction source/destination fix)"); + + /* Dictionary: lookups by key return the right values. */ + if (mDictCopy != NULL && CFDictionaryGetCount (mDictCopy) == 3) + { + PASS_CFEQ(CFDictionaryGetValue (mDictCopy, CFSTR("first")), s1, + "mutable dict deep copy preserves value for \"first\""); + PASS_CFEQ(CFDictionaryGetValue (mDictCopy, CFSTR("second")), s2, + "mutable dict deep copy preserves value for \"second\""); + PASS_CFEQ(CFDictionaryGetValue (mDictCopy, CFSTR("number")), n, + "mutable dict deep copy preserves value for \"number\""); + } + + /* Dictionary: the result is genuinely mutable. */ + if (mDictCopy != NULL) + { + CFDictionaryAddValue (mDictCopy, CFSTR("extra"), s1); + PASS_CF(CFDictionaryGetCount (mDictCopy) == 4, + "mutable dict deep copy is genuinely mutable (add grows count)"); + CFRelease (mDictCopy); + } + + CFRelease (srcDict); + + /* Nested: a mutable deep copy of an array-containing-array must + * preserve both the outer and inner contents. Because + * CFPropertyListCreateDeepCopy recurses through itself, a nested + * test catches bugs where the fix only works at the top level. */ + inner = srcArray; + aValues[0] = inner; + aValues[1] = s1; + outer = CFArrayCreateMutable (NULL, 2, &kCFTypeArrayCallBacks); + CFArrayAppendValue (outer, inner); + CFArrayAppendValue (outer, s1); + + mOuterCopy = (CFMutableArrayRef) CFPropertyListCreateDeepCopy (NULL, outer, + kCFPropertyListMutableContainersAndLeaves); + PASS_CF(mOuterCopy != NULL, + "mutable deep copy of a nested array is non-NULL"); + PASS_CF(mOuterCopy != NULL && CFArrayGetCount (mOuterCopy) == 2, + "outer nested array deep copy preserves count"); + if (mOuterCopy != NULL && CFArrayGetCount (mOuterCopy) == 2) + { + CFArrayRef nestedCopy + = (CFArrayRef) CFArrayGetValueAtIndex (mOuterCopy, 0); + PASS_CF(nestedCopy != NULL && CFArrayGetCount (nestedCopy) == 3, + "inner nested array is also deep-copied with all elements"); + } + if (mOuterCopy != NULL) + { + CFRelease (mOuterCopy); + } + + CFRelease (outer); + CFRelease (srcArray); + CFRelease (n); + return 0; +}