GTree: Check for node counter overflow when adding new elements

Currently, when adding new elements to GTree we blindly increment the node
counter, which is only of guint size (so 32-bit even on 64-bit Unix
platforms).

This is even more problematic because the only way to check whether
particular GTree is empty is to check whether its node count is zero.
This will obviously give wrong answer if this counter overflows.

Let's fix this by adding an appropriate check when adding a new node.

For the recently added g_tree_{insert,replace}_node () API we can simply
return NULL in such case.

However, the older g_tree_{insert,replace} () API doesn't have any ability
to return an error so for them we follow the example of
g_ptr_array_extend () and g_ptr_array_set_size () by calling g_error ()
when this happens.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

glib/gtree.c

@@ -105,7 +105,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 +455,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 +495,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 +505,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 +524,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 +543,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 +553,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 +570,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 +597,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 +609,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 +658,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 +685,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;
             }

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);
     }