Bug in TADS3 Vector.appendAll(), interpreter independent

Attempting to use appendAll() on an empty Vector with an argument that’s not a List or Vector causes a segfault. Appears to be interpreter-independent—tested on FrobTads, QTads, and emglken.

Example of problem. This will cause a segfault:

local v = new Vector();
v.appendAll(1);

This will not:

local v = new Vector();
v.append(1);
v.appendAll(1);

…and neither will…

local v = new Vector();
v.appendAll([ 1 ]);

Unfortunately it isn’t possible to modify Vector.appendAll() in TADS3 code, although you can implement a shim like:

modify Vector
        safeAppendAll(v) {
                if((v == nil) || !v.ofKind(Collection))
                        v = [ v ];
                appendAll(v);
        }
;

…and then just re-write code to use Vector.safeAppendAll() instead of appendAll().

5 Likes

Huh. I’ll be honest, I never would have considered myVector.appendAll(1) to be safe under any circumstances, and would have just checked what was being appended under all cases.

1 Like

The documentation sure makes it sound like the behavior should work:

This works like append(val), except that if val is a List or Vector, each element of val is individually appended to the target Vector. This method works like the + operator, except that this method modifies the Vector, rather than creating a new Vector to store the result. Returns self.

“If the val is a List or Vector” is a weird way to put it if the arg must be a List or Vector or the interpreter will segfault. As in not even just throwing an exception, which is what i.e. if(v > 1) {} will do if v is a Vector.

It also compares the operation of appendAll() to append() and the + operator, but both v.append(1) and v += 1 work fine.

2 Likes

Looks like this function, which does sound like it should accept non-lists:

But unlike the other functions in the file, it doesn’t call is_listlike, so it just assumes the value is a list. Adding in a check and a branch for a scalar wouldn’t be too tricky I expect.

5 Likes

Inside the logic of CVmObjVector::getp_append_all() this appears to work:

diff --git a/tads3/vmvec.cpp b/tads3/vmvec.cpp
index f57ee0d..75762a1 100644
--- a/tads3/vmvec.cpp
+++ b/tads3/vmvec.cpp
@@ -2792,24 +2792,29 @@ int CVmObjVector::getp_append_all(VMG_ vm_obj_id_t self, vm_val_t *retval,
     /* get the number of items to add */
     int add_cnt = valp->ll_length(vmg0_);
 
-    /* expand myself to make room for the new elements */
-    expand_by(vmg_ self, add_cnt);
-
-    /* add the new elements */
-    for (int i = 1 ; i <= add_cnt ; ++i)
-    {
-        /* get the next new element */
-        vm_val_t ele;
-        valp->ll_index(vmg_ &ele, i);
+    if(add_cnt < 0) {
+       expand_by(vmg_ self, 1);
+       set_element_undo(vmg_ self, cnt, valp);
+    } else {
+        /* expand myself to make room for the new elements */
+        expand_by(vmg_ self, add_cnt);
+
+        /* add the new elements */
+        for (int i = 1 ; i <= add_cnt ; ++i)
+        {
+            /* get the next new element */
+            vm_val_t ele;
+            valp->ll_index(vmg_ &ele, i);
         
-        /* 
-         *   add the new element - note that we don't need to keep undo
-         *   because we created this new element in this same operation, and
-         *   hence undoing the operation will truncate the vector to exclude
-         *   the element, and hence we don't need an old value for the
-         *   element 
-         */
-        set_element(cnt + i - 1, &ele);
+            /* 
+             *   add the new element - note that we don't need to keep undo
+             *   because we created this new element in this same operation, and
+             *   hence undoing the operation will truncate the vector to exclude
+             *   the element, and hence we don't need an old value for the
+             *   element 
+             */
+            set_element(cnt + i - 1, &ele);
+        }
     }
 
     /* discard the argument and gc protection */

…because the method is already calling ll_length() which will return -1 if the value isn’t a list.

But I’m really not familiar with the VM’s internals so I have no idea if there’s a better solution.

3 Likes

Ah, if ll_length can return -1 that probably explains the segfaults.

2 Likes

Yeah. And looking closer at it, my “fix” is probably not a fix, because ll_length() returns -1 not if the arg isn’t an array type but rather if it doesn’t have a length property. So this will not behave nicely:

local v = new Vector(); 
v.appendAll(object { length = 10 }); 

Although that will “just” throw a runtime error (instead of crashing the terp) and so I guess you could kinda squint at it and say that’s correct behavior (that is, objects declared that way shouldn’t use appendAll()).

But actually I think your first impression is right and getp_append_all() should just use is_listlike()):

diff --git a/tads3/vmvec.cpp b/tads3/vmvec.cpp
index f57ee0d..70247b8 100644
--- a/tads3/vmvec.cpp
+++ b/tads3/vmvec.cpp
@@ -2792,24 +2792,29 @@ int CVmObjVector::getp_append_all(VMG_ vm_obj_id_t self, vm_val_t *retval,
     /* get the number of items to add */
     int add_cnt = valp->ll_length(vmg0_);
 
-    /* expand myself to make room for the new elements */
-    expand_by(vmg_ self, add_cnt);
-
-    /* add the new elements */
-    for (int i = 1 ; i <= add_cnt ; ++i)
-    {
-        /* get the next new element */
-        vm_val_t ele;
-        valp->ll_index(vmg_ &ele, i);
+    if(!valp->is_listlike(vmg0_)) {
+       expand_by(vmg_ self, 1);
+       set_element_undo(vmg_ self, cnt, valp);
+    } else {
+        /* expand myself to make room for the new elements */
+        expand_by(vmg_ self, add_cnt);
+
+        /* add the new elements */
+        for (int i = 1 ; i <= add_cnt ; ++i)
+        {
+            /* get the next new element */
+            vm_val_t ele;
+            valp->ll_index(vmg_ &ele, i);
         
-        /* 
-         *   add the new element - note that we don't need to keep undo
-         *   because we created this new element in this same operation, and
-         *   hence undoing the operation will truncate the vector to exclude
-         *   the element, and hence we don't need an old value for the
-         *   element 
-         */
-        set_element(cnt + i - 1, &ele);
+            /* 
+             *   add the new element - note that we don't need to keep undo
+             *   because we created this new element in this same operation, and
+             *   hence undoing the operation will truncate the vector to exclude
+             *   the element, and hence we don't need an old value for the
+             *   element 
+             */
+            set_element(cnt + i - 1, &ele);
+        }
     }
 
     /* discard the argument and gc protection */

Which will handle v.appendAll(object { length = 10 }) “correctly” (that is, it will add a single element to v containing the silly anonymous object).

But, like I said, I really don’t know much about the VM internals and so I don’t know if there’s any hidden gotchas here. In particular I don’t grok the undo juggling that most of the vector manipulations do.

1 Like

Ahh. If TADS uses duck typing then it’s really the author’s responsibility not to pass in malformed structures. Runtime errors are totally appropriate.

Right. That’s why I can see the case for the ll_length() fix versus the is_listlike() fix.

But because the unpatched getp_append_all() actually segfaults and crashes the interpreter (instead of just throwing an exception) it’s probably a bug that should be patched instead of just a usage wart.

I made an issue so that this won’t be forgotten. Vector.appendAll() doesn't support scalars · Issue #36 · tads-intfic/tads-runner · GitHub

1 Like