CFPropertyList: fix out-of-bounds reads and an invalid free

ChangeLog

@@ -1,3 +1,15 @@
+2026-06-28  Todd White  <todd.white@thalion.global>
+	* Source/CFPropertyList.c (CFPlistStringPeek): New helper returning 0
+	at or past the end of the buffer.
+	(CFPlistStringSkipWhitespace, CFOpenStepPlistParseString,
+	CFOpenStepPlistParseObject, CFOpenStepPlistParseData): Use it for the
+	unguarded character reads so the scans do not read past the
+	(non-NUL-terminated) buffer.
+	(CFPropertyListCreateWithData): Free the separately allocated buffer
+	on every path, and free start rather than the advanced tmp pointer.
+	* Tests/CFPropertyList/openstep.m: Regression tests for a large
+	string and a comment-only list.
+
 2021-09-30  Frederik Seiffert <frederik@algoriddim.com>
 	* Source/CFDate.c: Fix logic in CFGregorianDateIsValid()
 

Source/CFPropertyList.c

@@ -455,29 +455,39 @@ CFPlistCreateError (CFIndex code, CFStringRef message)
   return error;
 }
 
+/* Return the character at the cursor, or 0 if the cursor is at or past the
+   end of the buffer.  The parser scans for terminators, so treating the end
+   of the buffer as a NUL lets the existing logic stop without reading out of
+   bounds (the buffer itself is not NUL-terminated). */
+CF_INLINE UniChar
+CFPlistStringPeek (CFPlistString * string)
+{
+  return string->cursor < string->limit ? *string->cursor : 0;
+}
+
 static Boolean
 CFPlistStringSkipWhitespace (CFPlistString * string)
 {
   UniChar ch;
 
   while (string->cursor < string->limit)
     {
-      ch = *string->cursor;
-      while (string->cursor < string->limit && GSCharacterIsWhitespace (ch))
+      ch = CFPlistStringPeek (string);
+      while (GSCharacterIsWhitespace (ch))
         {
           string->cursor++;
-          ch = *string->cursor;
+          ch = CFPlistStringPeek (string);
         }
       if (ch == '/')
         {
           string->cursor++;
-          ch = *string->cursor;
+          ch = CFPlistStringPeek (string);
           if (ch == '/')
             {
               do
                 {
                   string->cursor++;
-                  ch = *string->cursor;
+                  ch = CFPlistStringPeek (string);
                   if (ch == '\n')
                     break;
                 }
@@ -488,11 +498,11 @@ CFPlistStringSkipWhitespace (CFPlistString * string)
               do
                 {
                   string->cursor++;
-                  ch = *string->cursor;
+                  ch = CFPlistStringPeek (string);
                   if (ch == '*')
                     {
                       string->cursor++;
-                      ch = *string->cursor;
+                      ch = CFPlistStringPeek (string);
                       if (ch == '/')
                         break;
                     }
@@ -564,7 +574,8 @@ CFOpenStepPlistParseData (CFAllocatorRef alloc, CFPlistString * string)
           && string->cursor < string->limit)
         break;
 
-      ch = *string->cursor++;
+      ch = CFPlistStringPeek (string);
+      string->cursor++;
       nibble1 = getNibble (ch);
     }
 
@@ -622,7 +633,8 @@ CFOpenStepPlistParseString (CFAllocatorRef alloc, CFPlistString * string)
 
               CFStringAppendCharacters (tmp, mark, string->cursor - mark);
 
-              ch = *string->cursor++;
+              ch = CFPlistStringPeek (string);
+              string->cursor++;
               /* FIXME */
               if (ch >= '0' && ch <= '9')
                 {
@@ -675,7 +687,7 @@ CFOpenStepPlistParseString (CFAllocatorRef alloc, CFPlistString * string)
           if (CFOpenStepPlistCharacterIsQuotable (ch))
             break;
           string->cursor++;
-          ch = *string->cursor;
+          ch = CFPlistStringPeek (string);
         }
 
       if (mark != string->cursor)
@@ -757,7 +769,7 @@ CFOpenStepPlistParseObject (CFAllocatorRef alloc, CFPlistString * string)
             }
         }
 
-      if (*string->cursor == '}')
+      if (CFPlistStringPeek (string) == '}')
         {
           string->cursor++;
           obj = dict;
@@ -792,7 +804,7 @@ CFOpenStepPlistParseObject (CFAllocatorRef alloc, CFPlistString * string)
             }
         }
 
-      if (*string->cursor == ')')
+      if (CFPlistStringPeek (string) == ')')
         {
           string->cursor++;
           obj = array;
@@ -1350,7 +1362,7 @@ CFPropertyListCreateWithData (CFAllocatorRef alloc, CFDataRef data,
                                  &bytes, bytes + dataLen, 0);
   if (count < 0)
     return NULL;
-  
+
   if (count > _kCFPlistBufferSize)
     {
       start = CFAllocatorAllocate (kCFAllocatorSystemDefault,
@@ -1375,21 +1387,22 @@ CFPropertyListCreateWithData (CFAllocatorRef alloc, CFDataRef data,
     {
       if (fmt)
         *fmt = kCFPropertyListXMLFormat_v1_0;
-      return result;
     }
-
-  result = CFOpenStepPlistParseObject (alloc, &string);
-  if (result)
+  else
     {
-      if (fmt)
+      result = CFOpenStepPlistParseObject (alloc, &string);
+      if (result && fmt)
         *fmt = kCFPropertyListOpenStepFormat;
-      return result;
     }
 
+  /* Free the heap buffer on every path.  Use start, not tmp: tmp was
+     advanced to the end of the buffer by GSUnicodeFromEncoding, so freeing
+     it would free an interior pointer.  The parser copies anything it
+     keeps, so the buffer can be released even on success. */
   if (start != buffer)
-    CFAllocatorDeallocate (kCFAllocatorSystemDefault, tmp);
+    CFAllocatorDeallocate (kCFAllocatorSystemDefault, start);
 
-  return NULL;
+  return result;
 }
 
 CFPropertyListRef

Tests/CFPropertyList/openstep.m

@@ -1,6 +1,8 @@
 #include "../CFTesting.h"
 #include "CoreFoundation/CFPropertyList.h"
 #include "CoreFoundation/CFDictionary.h"
+#include "CoreFoundation/CFString.h"
+#include <string.h>
 
 const UInt8 data[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
 
@@ -64,5 +66,47 @@ int main (void)
   CFRelease (readPlist);
   CFRelease (plist);
 
+  {
+    /* A string longer than the internal 1024-character buffer is parsed
+       from a separately allocated buffer; this must not read past the end
+       of it or leak it. */
+    UInt8 big[1200];
+    CFDataRef bigData;
+    CFPropertyListRef bigPlist;
+
+    memset (big, 'a', 1100);
+    bigData = CFDataCreate (NULL, big, 1100);
+    bigPlist = CFPropertyListCreateWithData (NULL, bigData,
+                                             kCFPropertyListImmutable, NULL,
+                                             NULL);
+    PASS_CF (bigPlist != NULL
+             && CFGetTypeID (bigPlist) == CFStringGetTypeID (),
+             "A string larger than the internal buffer parses.");
+    if (bigPlist)
+      CFRelease (bigPlist);
+    CFRelease (bigData);
+  }
+
+  {
+    /* A "//" comment with no trailing newline that fills the buffer must
+       be stopped at the end of the data, not read past it. */
+    UInt8 c[1200];
+    CFDataRef cData;
+    CFPropertyListRef cPlist;
+
+    c[0] = '/';
+    c[1] = '/';
+    memset (c + 2, ' ', 1100);
+    cData = CFDataCreate (NULL, c, 1102);
+    cPlist = CFPropertyListCreateWithData (NULL, cData,
+                                           kCFPropertyListImmutable, NULL,
+                                           NULL);
+    PASS_CF (cPlist == NULL,
+             "A comment-only property list returns NULL without overreading.");
+    if (cPlist)
+      CFRelease (cPlist);
+    CFRelease (cData);
+  }
+
   return 0;
 }