From a937384ebf3e387c81897189e42c87a574b937fb Mon Sep 17 00:00:00 2001 From: Kaz Kylheku Date: Fri, 17 Dec 2021 19:26:54 -0800 Subject: 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. --- lib.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'lib.c') 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; } } -- cgit v1.2.3