Fix crash when incorrectly accessing a compressed vector v via v{...}[...] or v{...}{...} as if it was a matrix

src/lists.c

@@ -1270,6 +1270,7 @@ void            ElmListLevel (
     Obj                 row;
     Obj                 col;
 
+    RequirePlainList("List Elements", lists);
 
     /* if <level> is one, perform the replacements                         */
     if ( level == 1 ) {
@@ -1351,16 +1352,7 @@ void            ElmsListLevel (
     Obj                 elm;            /* selected elements from <list>   */
     Int                 i;              /* loop variable                   */
 
-    /* Workaround for issue #312: Accessing a two-level sublist
-       of a compressed FFE vector could lead to crashes because
-       FuncELMS_VEC8BIT and FuncELMS_GF2VEC may return lists which are
-       not plists. This boils down to a conflict between the documented
-       behavior and requirements of ElmsListLevel and ElmsListFuncs.
-       Resolving this properly requires some more discussion. But until
-       then, this change at least prevents hard crashes. */
-    if (!IS_PLIST(lists)) {
-        RequireArgument("List Elements", lists, "must be a list");
-    }
+    RequirePlainList("List Elements", lists);
 
     /* if <level> is one, perform the replacements                         */
     if ( level == 1 ) {
@@ -1433,7 +1425,7 @@ void            AssListLevel (
     Obj                 row;
     Obj                 col;
 
-    /* check <objs>                                                        */
+    RequirePlainList("List Assignments", lists);
     RequireDenseList("List Assignments", objs);
     RequireSameLength("List Assignments", objs, lists);
 
@@ -1516,7 +1508,7 @@ void            AsssListLevel (
     Obj                 obj;            /* one value from <objs>           */
     Int                 i;              /* loop variable                   */
 
-    /* check <objs>                                                        */
+    RequirePlainList("List Assignments", lists);
     RequireDenseList("List Assignments", objs);
     RequireSameLength("List Assignments", objs, lists);
 

tst/testbugfix/2013-03-12-t00285.tst

@@ -1,5 +1,58 @@
 # 2013/03/12 (MH)
-gap> v:=IdentityMat(28,GF(2))[1];
-<a GF2 vector of length 28>
-gap> v{[1..Length(v)]}{[1..5]};
-Error, List Elements: <lists> must be a list (not a data object)
+#
+# The following tests used to crash. This is because for compressed FFE
+# vectors, we install custom methods for ELMS_LIST (ELMS_VEC8BIT,
+# ELMS_GF2VEC...) which return compressed vectors again -- which are not plain
+# lists. But the code in ElmsListLevel etc. assumes that its first input is a
+# plain list. Indeed, the documentation for ElmsListFuncs states:
+#
+# > If the result is a list of lists, then it also *must* create a new list
+# > that has the same representation as a plain list.
+#
+# Now, ELMS_VEC8BIT etc. are not wrong: as they do not return a list of lists,
+# they are not required to return a plain list. But in the test cases below,
+# the code incorrectly is written as if that were the case; i.e., a vector is
+# accessed two levels deep, as if it was a matrix. This is wrong, and normally
+# (if the input is a regular uncompressed vector) fails because the code ends
+# up trying to access a coefficient as a list, which immediately throws an
+# error. But before it does that, it tries to recurse through the given list,
+# and for that it assumes it is a plain list.
+#
+# The fix is to add checks to the relevant functions that verify the input
+# is indeed a plain list.
+
+# test IsGF2VectorRep
+gap> v:=IdentityMat(12,GF(2))[1];
+<a GF2 vector of length 12>
+gap> v{[1..5]}{[1..5]};
+Error, List Elements: <lists> must be a plain list (not a data object)
+gap> v{[1..5]}[1];
+Error, List Elements: <lists> must be a plain list (not a data object)
+gap> v{[1..5]}[1] := 1;
+Error, List Assignments: <lists> must be a plain list (not a data object)
+gap> v{[1..5]}{[1..5]}:=ListWithIdenticalEntries(5,[1..5]);
+Error, List Assignments: <lists> must be a plain list (not a data object)
+
+# test Is8BitVectorRep
+gap> v:=IdentityMat(12,GF(3))[1];
+< mutable compressed vector length 12 over GF(3) >
+gap> v{[1..5]}{[1..5]};
+Error, List Elements: <lists> must be a plain list (not a data object)
+gap> v{[1..5]}[1];
+Error, List Elements: <lists> must be a plain list (not a data object)
+gap> v{[1..5]}[1] := 1;
+Error, List Assignments: <lists> must be a plain list (not a data object)
+gap> v{[1..5]}{[1..5]}:=ListWithIdenticalEntries(5,[1..5]);
+Error, List Assignments: <lists> must be a plain list (not a data object)
+
+# for reference, there is a similar error when using e.g. rational matrices
+gap> v:=IdentityMat(12,Rationals)[1];
+[ 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]
+gap> v{[1..5]}{[1..5]};
+Error, List Elements: <list> must be a list (not the integer 1)
+gap> v{[1..5]}[1];
+Error, List Element: <list> must be a list (not the integer 1)
+gap> v{[1..5]}[1] := 1;
+Error, List Assignments: <objs> must be a dense list (not the integer 1)
+gap> v{[1..5]}{[1..5]}:=ListWithIdenticalEntries(5,[1..5]);
+Error, List Assignments: <list> must be a list (not the integer 1)