Merge branch 'gtree-overflow' into 'main'

GTree: Handle node counter overflow and return it as an unsigned value

See merge request GNOME/glib!3521

glib/gtree.c

@@ -69,6 +69,8 @@
  **/
 
 #define MAX_GTREE_HEIGHT 40
+/* G_MAXUINT nodes will be covered by tree height of log2(G_MAXUINT) + 2. */
+G_STATIC_ASSERT ((G_GUINT64_CONSTANT (1) << (MAX_GTREE_HEIGHT - 2)) >= G_MAXUINT);
 
 /**
  * GTree:
@@ -105,7 +107,8 @@ static GTreeNode* g_tree_node_new                   (gpointer       key,
 static GTreeNode *g_tree_insert_internal (GTree *tree,
                                           gpointer key,
                                           gpointer value,
-                                          gboolean replace);
+                                          gboolean replace,
+                                          gboolean null_ret_ok);
 static gboolean   g_tree_remove_internal            (GTree         *tree,
                                                      gconstpointer  key,
                                                      gboolean       steal);
@@ -454,6 +457,26 @@ g_tree_destroy (GTree *tree)
   g_tree_unref (tree);
 }
 
+static GTreeNode *
+g_tree_insert_replace_node_internal (GTree *tree,
+                                     gpointer key,
+                                     gpointer value,
+                                     gboolean replace,
+                                     gboolean null_ret_ok)
+{
+  GTreeNode *node;
+
+  g_return_val_if_fail (tree != NULL, NULL);
+
+  node = g_tree_insert_internal (tree, key, value, replace, null_ret_ok);
+
+#ifdef G_TREE_DEBUG
+  g_tree_node_check (tree->root);
+#endif
+
+  return node;
+}
+
 /**
  * g_tree_insert_node:
  * @tree: a #GTree
@@ -474,7 +497,8 @@ g_tree_destroy (GTree *tree)
  * result in a O(n log(n)) operation where most of the other operations
  * are O(log(n)).
  *
- * Returns: (transfer none): the inserted (or set) node.
+ * Returns: (transfer none) (nullable): the inserted (or set) node or %NULL
+ * if insertion would overflow the tree node counter.
  *
  * Since: 2.68
  */
@@ -483,17 +507,7 @@ g_tree_insert_node (GTree    *tree,
                     gpointer  key,
                     gpointer  value)
 {
-  GTreeNode *node;
+  return g_tree_insert_replace_node_internal (tree, key, value, FALSE, TRUE);
-
-  g_return_val_if_fail (tree != NULL, NULL);
-
-  node = g_tree_insert_internal (tree, key, value, FALSE);
-
-#ifdef G_TREE_DEBUG
-  g_tree_node_check (tree->root);
-#endif
-
-  return node;
 }
 
 /**
@@ -512,7 +526,7 @@ g_tree_insert (GTree    *tree,
                gpointer  key,
                gpointer  value)
 {
-  g_tree_insert_node (tree, key, value);
+  g_tree_insert_replace_node_internal (tree, key, value, FALSE, FALSE);
 }
 
 /**
@@ -531,7 +545,8 @@ g_tree_insert (GTree    *tree,
  * The tree is automatically 'balanced' as new key/value pairs are added,
  * so that the distance from the root to every leaf is as small as possible.
  *
- * Returns: (transfer none): the inserted (or set) node.
+ * Returns: (transfer none) (nullable): the inserted (or set) node or %NULL
+ * if insertion would overflow the tree node counter.
  *
  * Since: 2.68
  */
@@ -540,17 +555,7 @@ g_tree_replace_node (GTree    *tree,
                      gpointer  key,
                      gpointer  value)
 {
-  GTreeNode *node;
+  return g_tree_insert_replace_node_internal (tree, key, value, TRUE, TRUE);
-
-  g_return_val_if_fail (tree != NULL, NULL);
-
-  node = g_tree_insert_internal (tree, key, value, TRUE);
-
-#ifdef G_TREE_DEBUG
-  g_tree_node_check (tree->root);
-#endif
-
-  return node;
 }
 
 /**
@@ -567,7 +572,26 @@ g_tree_replace (GTree    *tree,
                 gpointer  key,
                 gpointer  value)
 {
-  g_tree_replace_node (tree, key, value);
+  g_tree_insert_replace_node_internal (tree, key, value, TRUE, FALSE);
+}
+
+/* internal checked nnodes increment routine */
+static gboolean
+g_tree_nnodes_inc_checked (GTree *tree, gboolean overflow_fatal)
+{
+  if (G_UNLIKELY (tree->nnodes == G_MAXUINT))
+    {
+      if (overflow_fatal)
+        {
+          g_error ("Incrementing GTree nnodes counter would overflow");
+        }
+
+      return FALSE;
+    }
+
+  tree->nnodes++;
+
+  return TRUE;
 }
 
 /* internal insert routine */
@@ -575,7 +599,8 @@ static GTreeNode *
 g_tree_insert_internal (GTree    *tree,
                         gpointer  key,
                         gpointer  value,
-                        gboolean  replace)
+                        gboolean  replace,
+                        gboolean  null_ret_ok)
 {
   GTreeNode *node, *retnode;
   GTreeNode *path[MAX_GTREE_HEIGHT];
@@ -586,7 +611,12 @@ g_tree_insert_internal (GTree    *tree,
   if (!tree->root)
     {
       tree->root = g_tree_node_new (key, value);
+
+#ifdef G_TREE_DEBUG
+      g_assert (tree->nnodes == 0);
+#endif
       tree->nnodes++;
+
       return tree->root;
     }
 
@@ -630,16 +660,20 @@ g_tree_insert_internal (GTree    *tree,
             }
           else
             {
-              GTreeNode *child = g_tree_node_new (key, value);
+              GTreeNode *child;
 
+              if (!g_tree_nnodes_inc_checked (tree, !null_ret_ok))
+                {
+                  return NULL;
+                }
+
+              child = g_tree_node_new (key, value);
               child->left = node->left;
               child->right = node;
               node->left = child;
               node->left_child = TRUE;
               node->balance -= 1;
 
-              tree->nnodes++;
-
               retnode = child;
               break;
             }
@@ -653,16 +687,20 @@ g_tree_insert_internal (GTree    *tree,
             }
           else
             {
-              GTreeNode *child = g_tree_node_new (key, value);
+              GTreeNode *child;
 
+              if (!g_tree_nnodes_inc_checked (tree, !null_ret_ok))
+                {
+                  return NULL;
+                }
+
+              child = g_tree_node_new (key, value);
               child->right = node->right;
               child->left = node;
               node->right = child;
               node->right_child = TRUE;
               node->balance += 1;
 
-              tree->nnodes++;
-
               retnode = child;
               break;
             }
@@ -1454,6 +1492,11 @@ g_tree_height (GTree *tree)
  * Gets the number of nodes in a #GTree.
  * 
  * Returns: the number of nodes in @tree
+ *
+ * The node counter value type is really a #guint,
+ * but it is returned as a #gint due to backward
+ * compatibility issues (can be cast back to #guint to
+ * support its full range of values).
  */
 gint
 g_tree_nnodes (GTree *tree)

glib/tests/tree.c

@@ -237,9 +237,10 @@ static void
 test_tree_remove (void)
 {
   GTree *tree;
-  char c, d;
+  char c, d, e, f;
   gint i;
   gboolean removed;
+  GTreeNode *node;
   gchar *remove;
 
   tree = g_tree_new_full ((GCompareDataFunc)my_compare, NULL,
@@ -263,6 +264,12 @@ test_tree_remove (void)
   destroyed_key = NULL;
   destroyed_value = NULL;
 
+  e = '\xff';
+  node = g_tree_insert_node (tree, &e, &e);
+  g_assert (node);
+  g_assert (destroyed_key == NULL);
+  g_assert (destroyed_value == NULL);
+
   c = '2';
   removed = g_tree_remove (tree, &c);
   g_assert (removed);
@@ -277,6 +284,14 @@ test_tree_remove (void)
   g_assert (destroyed_key == NULL);
   g_assert (destroyed_value == NULL);
 
+  f = '4';
+  node = g_tree_replace_node (tree, &f, &f);
+  g_assert (node);
+  g_assert (destroyed_key == &chars[4]);
+  g_assert (destroyed_value == &chars[4]);
+  destroyed_key = NULL;
+  destroyed_value = NULL;
+
   remove = "omkjigfedba";
   for (i = 0; remove[i]; i++)
     {
@@ -655,6 +670,7 @@ test_tree_bounds (void)
         g_test_message ("%c ", *(char *) elem);
 
       node = g_tree_insert_node (tree, elem, elem);
+      g_assert (node);
       g_assert (g_tree_node_key (node) == elem);
       g_assert (g_tree_node_value (node) == elem);
     }