iter-begin: gc problem.

Issue 1: the seq_iter_init_with_info function potentially allocates
an object via hash_begin or tree_begin and installs it into the
iterator. The problem is that under iter_begin, the iterator is
a heaped object; this extra allocation can trigger gc which pushes the
iterator into the mature generation; yet the assignment in
seq_iter_init_with_info is just a plain assignment without using the set
macro.

Issue 2: when gc is triggered in the above situations, it crashes
due to the struct seq_iter being incompletely initialized. The
mark function tries to dereference the si->ops pointer. Alas, this
is initialized in the wrong order inside seq_iter_init_with_info.
Concretely, tree_begin is called first, and then the
it->ops = &si_tree_ops assignment is performed, which means that if
the garbage collector runs under tree_begin, it sees a null it->ops
pointer. However, this issue cannot just be fixed here by rearranging
the code because that leaves Issue 1 unsolved. Also, this initialization
order is not an issue for stack-allocated struct seq_iters.

The fix for Issue 1 and Issue 2 is to reorder things in iter_begin.
Initialize the iterator structure first, and then create the iterator
cobj. Now, of course, that goes against the usual correct protocol for
object initialization. If we just do this re-ordering naively,
we have Issue 3: the familiar problem that the cobj() call triggers gc,
and the iterator object (e.g. from tree_iter) that has been stored into
the seq_iter structure is not visible ot the GC, and is reclaimed.

* lib.c (iter_begin): reorder the calls so that seq_iter_init_with_info
is called first, and then the cobj to create from it the heap-allocated
iterator, taking care of Issue 1 and Issue 2. To avoid Issue 3,
after initializing the structure, we pull out the vulnerable iterator
object into a local variable, and pass it to gc_hint(), to ensure that
the variable is spilled into the stack, thereby protecting it from
reclamation.
(seq_begin): This function has exactly the same issue, fixed in the
same way.

1 files changed, 8 insertions, 4 deletions

diff --git a/lib.c b/lib.c
index 9e20fb20..15909c6c 100644
--- a/lib.c
+++ b/lib.c
@@ -1157,10 +1157,12 @@ static struct cobj_ops seq_iter_ops = cobj_ops_init(eq,
 val seq_begin(val obj)
 {
   val self = lit("seq-begin");
-  val si_obj;
+  val si_obj, iter;
   struct seq_iter *si = coerce(struct seq_iter *, chk_calloc(1, sizeof *si));
-  si_obj = cobj(coerce(mem_t *, si), seq_iter_cls, &seq_iter_ops);
   seq_iter_init(self, si, obj);
+  iter = si->ui.iter;
+  si_obj = cobj(coerce(mem_t *, si), seq_iter_cls, &seq_iter_ops);
+  gc_hint(iter);
   return si_obj;
 }
 
@@ -1208,11 +1210,13 @@ val iter_begin(val obj)
       return sinf.obj;
     default:
       {
-        val si_obj;
+        val si_obj, iter;
         struct seq_iter *si = coerce(struct seq_iter *,
                                      chk_calloc(1, sizeof *si));
-        si_obj = cobj(coerce(mem_t *, si), seq_iter_cls, &seq_iter_ops);
         seq_iter_init_with_info(self, si, sinf, 0);
+        iter = si->ui.iter;
+        si_obj = cobj(coerce(mem_t *, si), seq_iter_cls, &seq_iter_ops);
+        gc_hint(iter);
         return si_obj;
       }
     }