[SV 67046] Don't modify the hash table when inside hash_map*()

It's illegal for the user-supplied function to modify the hash table
while executing hash_map*() methods.  snap_file() may add elements
due to EXTRA_PREREQS, so we can't call it via hash_map_args().

Diagnosis and test by Shim Manning <shimmanning@gmail.com>
Possible fix by Dmitry Goncharov <dgoncharov@users.sf.net>

* src/hash.h (hash_table): Remember when we're in hash_map*().
* src/hash.c (hash_init): Initialize the in-map value to false.
(hash_map): Set the in-map value to true when walking the map.
(hash_map_arg): Ditto.
(hash_insert_at): Assert we're not in hash_map*()
(hash_free_items): Ditto.
(hash_delete_items): Ditto.
(hash_free): Ditto.
* src/file.c (snap_deps): Use hash_dump() to create a copy of the
hash and walk that to call snap_file(), so we can add more files.
(snap_file): Since we're calling it directly, don't use void* args.

diff --git a/src/file.c b/src/file.c
index 65517d0714bbb5706256a5e8e235d66259a16a95..447662a7b6d5b355b11267c5f34958a0989de4bf 100644 (file)
--- a/src/file.c
+++ b/src/file.c
@@ -723,9 +723,8 @@ expand_extra_prereqs (const struct variable *extra)
 /* Perform per-file snap operations. */
 
 static void
-snap_file (const void *item, void *arg)
+snap_file (struct file *f, const struct dep *deps)
 {
-  struct file *f = (struct file*)item;
   struct dep *prereqs = NULL;
   struct dep *d;
 
@@ -759,7 +758,7 @@ snap_file (const void *item, void *arg)
           }
     }
   else if (f->is_target)
-    prereqs = copy_dep_chain (arg);
+    prereqs = copy_dep_chain (deps);
 
   if (prereqs)
     {
@@ -914,9 +913,16 @@ snap_deps (void)
   {
     struct dep *prereqs = expand_extra_prereqs (lookup_variable (STRING_SIZE_TUPLE(".EXTRA_PREREQS")));
 
-    /* Perform per-file snap operations.  */
-    hash_map_arg(&files, snap_file, prereqs);
+    /* Perform per-file snap operations.
+       We can't use hash_map*() here because snap_file may add new elements
+       into the files hash, which is not allowed in the map.  Instead make a
+       dump of the files and walk through that.  */
+    void** filedump = hash_dump (&files, NULL, 0);
 
+    for (void** filep = filedump; *filep; ++filep)
+      snap_file (*filep, prereqs);
+
+    free (filedump);
     free_dep_chain (prereqs);
   }
 

diff --git a/src/hash.c b/src/hash.c
index a118552a969d56d567e41885d271545de5ca9751..a0b370f6af96ab6a5bc4a45f89c4510944f5e5de 100644 (file)
--- a/src/hash.c
+++ b/src/hash.c
@@ -59,6 +59,7 @@ hash_init (struct hash_table *ht, unsigned long size,
   ht->ht_collisions = 0;
   ht->ht_lookups = 0;
   ht->ht_rehashes = 0;
+  ht->ht_in_map = 0;
   ht->ht_hash_1 = hash_1;
   ht->ht_hash_2 = hash_2;
   ht->ht_compare = hash_cmp;
@@ -138,6 +139,10 @@ void *
 hash_insert_at (struct hash_table *ht, const void *item, const void *slot)
 {
   const void *old_item = *(void **) slot;
+
+  /* It's illegal to insert while in hash_map*().  */
+  assert (! ht->ht_in_map);
+
   if (HASH_VACANT (old_item))
     {
       ht->ht_fill++;
@@ -181,6 +186,10 @@ hash_free_items (struct hash_table *ht)
 {
   void **vec = ht->ht_vec;
   void **end = &vec[ht->ht_size];
+
+  /* It's illegal to free items while in hash_map*().  */
+  assert (! ht->ht_in_map);
+
   for (; vec < end; vec++)
     {
       void *item = *vec;
@@ -197,6 +206,10 @@ hash_delete_items (struct hash_table *ht)
 {
   void **vec = ht->ht_vec;
   void **end = &vec[ht->ht_size];
+
+  /* It's illegal to delete all items while in hash_map*().  */
+  assert (! ht->ht_in_map);
+
   for (; vec < end; vec++)
     *vec = 0;
   ht->ht_fill = 0;
@@ -209,6 +222,9 @@ hash_delete_items (struct hash_table *ht)
 void
 hash_free (struct hash_table *ht, int free_items)
 {
+  /* It's illegal to free while in hash_map*().  */
+  assert (! ht->ht_in_map);
+
   if (free_items)
     hash_free_items (ht);
   else
@@ -227,11 +243,15 @@ hash_map (struct hash_table *ht, hash_map_func_t map)
   void **slot;
   void **end = &ht->ht_vec[ht->ht_size];
 
+  ht->ht_in_map = 1;
+
   for (slot = ht->ht_vec; slot < end; slot++)
     {
       if (!HASH_VACANT (*slot))
         (*map) (*slot);
     }
+
+  ht->ht_in_map = 0;
 }
 
 void
@@ -240,11 +260,15 @@ hash_map_arg (struct hash_table *ht, hash_map_arg_func_t map, void *arg)
   void **slot;
   void **end = &ht->ht_vec[ht->ht_size];
 
+  ht->ht_in_map = 1;
+
   for (slot = ht->ht_vec; slot < end; slot++)
     {
       if (!HASH_VACANT (*slot))
         (*map) (*slot, arg);
     }
+
+  ht->ht_in_map = 0;
 }
 
 /* Double the size of the hash table in the event of overflow... */

diff --git a/src/hash.h b/src/hash.h
index 1bbda0f7d9cf48caa0fce5bc0ebaacd107df2cee..27b202a4ac972e820d6c58b017aea39216ff41cb 100644 (file)
--- a/src/hash.h
+++ b/src/hash.h
@@ -51,6 +51,7 @@ struct hash_table
   unsigned long ht_collisions; /* # of failed calls to comparison function */
   unsigned long ht_lookups;    /* # of queries */
   unsigned int ht_rehashes;    /* # of times we've expanded table */
+  unsigned int ht_in_map:1;     /* 1 if we're inside a hash_map*() function */
 };
 
 typedef int (*qsort_cmp_t) __P((void const *, void const *));

diff --git a/tests/scripts/variables/EXTRA_PREREQS b/tests/scripts/variables/EXTRA_PREREQS
index 2f72be30898e74054a3340d013734fbfba8e78d2..a34abd93954e8f35a807a9ea4c47513febac87c4 100644 (file)
--- a/tests/scripts/variables/EXTRA_PREREQS
+++ b/tests/scripts/variables/EXTRA_PREREQS
@@ -178,7 +178,16 @@ hello.x world.x: .EXTRA_PREREQS=$$@.d
 ',
               '', "hello.x.d\nhello.x\nworld.x.d\nworld.x\n");
 
+# SV 67046. Rehashing of hash table files during iteration.
+my $files = "";
+for my $k (1 .. 1000) {
+    $files = "$files file$k";
+}
+run_make_test(qq!
+all: .EXTRA_PREREQS := $files
+all:;
+file%:;
+!,
+              '', "#MAKE#: 'all' is up to date.");
 
-
-# This tells the test driver that the perl test script executed properly.
 1;