Merge branch 'fix-string-replace-heap-buffer-overflow' into 'main'

gstring: Fix a heap buffer overflow in the new g_string_replace() code

See merge request GNOME/glib!4332

glib/gstring.c

@@ -1041,10 +1041,53 @@ g_string_replace (GString     *string,
   f_len = strlen (find);
   r_len = strlen (replace);
 
+  /* It removes a lot of branches and possibility for infinite loops if we
+   * handle the case of an empty @find string separately. */
+  if (G_UNLIKELY (f_len == 0))
+    {
+      if (limit == 0 || limit > string->len)
+        {
+          if (string->len > G_MAXSIZE - 1)
+            g_error ("inserting in every position in string would overflow");
+
+          limit = string->len + 1;
+        }
+
+      if (r_len > 0 &&
+          (limit > G_MAXSIZE / r_len ||
+           limit * r_len > G_MAXSIZE - string->len))
+        g_error ("inserting in every position in string would overflow");
+
+      new_len = string->len + limit * r_len;
+      new_string = g_string_sized_new (new_len);
+      for (size_t i = 0; i < limit; i++)
+        {
+          g_string_append_len (new_string, replace, r_len);
+          if (i < string->len)
+            g_string_append_c (new_string, string->str[i]);
+        }
+      if (limit < string->len)
+        g_string_append_len (new_string, string->str + limit, string->len - limit);
+
+      g_free (string->str);
+      string->allocated_len = new_string->allocated_len;
+      string->len = new_string->len;
+      string->str = g_string_free_and_steal (g_steal_pointer (&new_string));
+
+      return limit;
+    }
+
   /* Potentially do two passes: the first to calculate the length of the new string,
    * new_len, if it’s going to be longer than the original string; and the second to
    * do the replacements. The first pass is skipped if the new string is going to be
-   * no longer than the original. */
+   * no longer than the original.
+   *
+   * The second pass calls various g_string_insert_len() (and similar) methods
+   * which would normally potentially reallocate string->str, and hence
+   * invalidate the cur/next/first/dst pointers. Because we’ve pre-calculated
+   * the new_len and do all the string manipulations on new_string, that
+   * shouldn’t happen. This means we scan `string` while modifying
+   * `new_string`. */
   do
     {
       dst = first;
@@ -1053,48 +1096,32 @@ g_string_replace (GString     *string,
       while ((next = strstr (cur, find)) != NULL)
         {
           n++;
-          if G_UNLIKELY (f_len == 0)
+
+          if (r_len <= f_len)
             {
-              g_string_insert_len (string, next - string->str, replace, r_len);
+              memmove (dst, cur, next - cur);
-              cur = next + r_len;
+              dst += next - cur;
-              /* Only match the empty string once at any given position, to
+              memcpy (dst, replace, r_len);
-               * avoid infinite loops */
+              dst += r_len;
-              if (cur[0] == '\0')
-                break;
-              else
-                cur++;
             }
           else
             {
-              if (r_len <= f_len)
+              if (new_string == NULL)
                 {
-                  memmove (dst, cur, next - cur);
+                  new_len += r_len - f_len;
-                  dst += next - cur;
-                  memcpy (dst, replace, r_len);
-                  dst += r_len;
                 }
               else
                 {
-                  if (new_string == NULL)
+                  g_string_append_len (new_string, cur, next - cur);
-                    {
+                  g_string_append_len (new_string, replace, r_len);
-                      new_len += r_len - f_len;
-                    }
-                  else
-                    {
-                      g_string_append_len (new_string, cur, next - cur);
-                      g_string_append_len (new_string, replace, r_len);
-                    }
                 }
-              cur = next + f_len;
             }
+          cur = next + f_len;
+
           if (n == limit)
             break;
         }
 
-      /* @find is an empty string. */
-      if (f_len == 0)
-        break;
-
       /* Append the trailing characters from after the final instance of @find
        * in the input string. */
       if (r_len <= f_len)

glib/tests/string.c

@@ -664,10 +664,25 @@ test_string_replace (void)
       "bba", 1 },
     { "foo", "", "bar", 0,
       "barfbarobarobar", 4 },
+    { "foo", "", "bar", 1,
+      "barfoo", 1 },
+    { "foo", "", "bar", 2,
+      "barfbaroo", 2 },
+    { "foo", "", "bar", 3,
+      "barfbarobaro", 3 },
+    { "foo", "", "bar", 4,
+      "barfbarobarobar", 4 },
+    { "foo", "", "bar", 5,
+      "barfbarobarobar", 4 },
     { "", "", "x", 0,
       "x", 1 },
     { "", "", "", 0,
       "", 1 },
+    /* use find and replace strings long enough to trigger a reallocation in
+     * the result string */
+    { "bbbbbbbbb", "", "aaaaaaaaaaaa", 0,
+      "aaaaaaaaaaaabaaaaaaaaaaaabaaaaaaaaaaaabaaaaaaaaaaaabaaaaaaaaaaaab"
+      "aaaaaaaaaaaabaaaaaaaaaaaabaaaaaaaaaaaabaaaaaaaaaaaabaaaaaaaaaaaa", 10 },
   };
   gsize i;