From e882dd6def125cabc6b1523520dbb35e85d6ab69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20Grubbstr=C3=B6m=20=28Grubba=29?=
 <grubba@grubba.org>
Date: Tue, 22 May 2018 14:49:06 +0200
Subject: [PATCH] Pike.identify_cycle: Fix various issues with LFUNs throwing
 errors.

LFUNs used by mapping operations may throw errors; this caused
identify_loop_visit_leave() to also throw errors. The rest of
the identify_cycle code was not happy about this and

  * Forgot to unlock the mc_mutex. This caused all following calls
    to Pike.identify_cycle() and Pike.count_memory() to hang.

  * Leaked memory.

The above issues are now avoided by instead using the addresses
of objects as indices in the affected mapping. It also fixes
the issue with hangs if there are objects with lfuns calling
Pike.identify_cycle() or Pike.count_memory().

Fixes [PIKE-106].
---
 src/gc.c         | 17 ++++++++++++++---
 src/testsuite.in | 27 +++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/src/gc.c b/src/gc.c
index 0526b40304..89c306edb0 100644
--- a/src/gc.c
+++ b/src/gc.c
@@ -6119,11 +6119,14 @@ void identify_loop_visit_ref(void *dst, int UNUSED(ref_type),
   ref_to = my_make_mc_marker(dst, visit_dst, extra);
 
   if (type != PIKE_T_UNKNOWN) {
+    /* NB: low_mapping_insert() for object indices may throw errors
+     *     if eg lfun::`==() throws an error. We therefore instead
+     *     use the raw pointers as indices instead.
+     */
     struct svalue s;
-    SET_SVAL(s, type, 0, refs, dst);
-    low_mapping_insert(identify_loop_reverse, &s, Pike_sp-1, 0);
-
+    SET_SVAL(s, PIKE_T_INT, NUMBER_NUMBER, integer, (INT_TYPE)(ptrdiff_t)dst);
     mc_wq_enqueue(ref_to);
+    low_mapping_insert(identify_loop_reverse, &s, Pike_sp-1, 0);
   } else {
     /* Not a valid svalue type.
      *
@@ -6258,14 +6261,22 @@ void f_identify_cycle(INT32 args)
   }
 #endif
 
+  /* NB: low_mapping_lookup() for object indices may throw errors
+   *     if eg lfun::`==() throws an error. We therefore instead
+   *     use the raw pointers as indices instead.
+   */
+  push_int((INT_TYPE)(ptrdiff_t)s->u.refs);
   while ((k = low_mapping_lookup(identify_loop_reverse, Pike_sp-1))) {
     /* NB: Since we entered this loop, we know that there's a
      *     reference loop involving s, as s otherwise wouldn't
      *     have been in the mapping.
      */
+    pop_stack();
     push_svalue(k);
+    push_int((INT_TYPE)(ptrdiff_t)k->u.refs);
     if (k->u.refs == s->u.refs) {
       /* Found! */
+      pop_stack();
       break;
     }
   }
diff --git a/src/testsuite.in b/src/testsuite.in
index 91dfd718ed..19cc1c78ae 100644
--- a/src/testsuite.in
+++ b/src/testsuite.in
@@ -7294,6 +7294,33 @@ test_program([[
   }
 ]])
 
+// Pike.identify_cycle
+test_any_equal([[
+  class Foo { Foo next; int counter; };
+  array(Foo) foos = allocate(10, Foo)();
+  for (int i = 0; i < 10; i++) {
+    foos[i]->next = foos[(i+1)%10];
+    foos[i]->counter = i;
+  }
+  return Pike.identify_cycle(foos[0])->counter;
+]], ({ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 }))
+test_any([[
+  // PIKE-106: Mutex not unlocked properly on error in identify_cycle().
+  class Foo {
+    int cnt;
+    int __hash() { return 0; }
+    int `==(mixed x) { if (!cnt++) error("Comparison failure.\n"); }
+  };
+  catch {
+    // This triggered an error.
+    Pike.identify_cycle(allocate(2, Foo)());
+  };
+  catch {
+    // This hanged due to the mc_mutex not having been released.
+    Pike.identify_cycle(allocate(2, Foo)());
+  };
+]], 0)
+
 // Numerical limits.
 test_true([[Int.NATIVE_MIN <= -2147483648]])
 test_true([[Int.NATIVE_MAX >= 2147483647]])
-- 
GitLab