From 7d39a545f7804fad5c876af7ba7ebf7ca87be1a4 Mon Sep 17 00:00:00 2001
From: Chris Jefferson <caj21@st-andrews.ac.uk>
Date: Tue, 12 Nov 2019 12:08:15 +0000
Subject: [PATCH] Fix overflow bug in CYCLE_STRUCT_PERM

CycleStructPerm overflowed a UInt2 when given a cycle of length
exactly 2^16, so store the size-1 (which is what we output anyway)
---
 src/permutat.cc                     |  24 +++---
 tst/testinstall/kernel/permutat.tst | 113 ++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+), 15 deletions(-)
 create mode 100644 tst/testinstall/kernel/permutat.tst

diff --git a/src/permutat.cc b/src/permutat.cc
index dfa28005e6..4777aa5c85 100644
--- a/src/permutat.cc
+++ b/src/permutat.cc
@@ -1340,14 +1340,14 @@ static inline Obj CYCLE_STRUCT_PERM(Obj perm)
     max = 0;
     for (pnt = 0; pnt < deg; pnt++) {
         if (clr[pnt] == 0) {
-            len = 1;
+            len = 0;
             clr[pnt] = 1;
             for (p = ptPerm[pnt]; p != pnt; p = ptPerm[p]) {
                 clr[p] = 1;
                 len++;
             }
 
-            if (len > 1) {
+            if (len > 0) {
                 offset[cnt] = (T)len;
                 cnt++;
                 if (len > max) {
@@ -1360,27 +1360,21 @@ static inline Obj CYCLE_STRUCT_PERM(Obj perm)
     ende = cnt;
 
     /* create the list */
-    list = NEW_PLIST(T_PLIST, max - 1);
-    SET_LEN_PLIST(list, max - 1);
+    list = NEW_PLIST(T_PLIST, max);
+    SET_LEN_PLIST(list, max);
     ptList = ADDR_OBJ(list);
 
     /* Recalculate after possible GC */
     scratch = ADDR_TMP_PERM<T>();
     offset = (T *)((UInt)scratch + (bytes));
 
-    for (pnt = 1; pnt < max; pnt++) {
-        ptList[pnt] = 0;
-    } /* clean out */
-
     for (cnt = 0; cnt < ende; cnt++) {
         pnt = (UInt)offset[cnt];
-        pnt--;
-        ptList[pnt] = (Obj)((UInt)ptList[pnt] + 1);
-    }
-
-    for (pnt = 1; pnt < max; pnt++) {
-        if (ptList[pnt] != 0) {
-            ptList[pnt] = INTOBJ_INT((UInt)ptList[pnt]);
+        if (ptList[pnt] == 0) {
+            ptList[pnt] = INTOBJ_INT(1);
+        }
+        else {
+            ptList[pnt] = INTOBJ_INT(INT_INTOBJ(ptList[pnt]) + 1);
         }
     }
 
diff --git a/tst/testinstall/kernel/permutat.tst b/tst/testinstall/kernel/permutat.tst
new file mode 100644
index 0000000000..6022fe6455
--- /dev/null
+++ b/tst/testinstall/kernel/permutat.tst
@@ -0,0 +1,113 @@
+#
+# Tests for functions defined in src/permutat.cc
+#
+gap> START_TEST("kernel/permutat.tst");
+
+# Reduce the amount printed by CYCLE_STRUCT_PERM by skipping unbound values
+gap> CycleStructPermShort := function(p)
+>  local ret, i, l;
+>  l := CYCLE_STRUCT_PERM(p);
+>  ret := [];
+>  for i in [1..Length(l)] do
+>    if IsBound(l[i]) then
+>      Add(ret, [i,l[i]]);
+>    fi;
+> od;
+> return ret;
+> end;;
+gap> permprops := function(p)
+>  local name;
+>  for name in ["LARGEST_MOVED_POINT_PERM",
+>               "ORDER_PERM", "SIGN_PERM"] do
+>      PrintFormatted("{}:{}\n", name, ValueGlobal(name)(p));
+>  od;
+>  PrintFormatted("CYCLE_SHORT_PERM (short output): {}\n", CycleStructPermShort(p));
+> end;;
+gap> permprops(());
+LARGEST_MOVED_POINT_PERM:0
+ORDER_PERM:1
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [  ]
+gap> permprops((1,2));
+LARGEST_MOVED_POINT_PERM:2
+ORDER_PERM:2
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 1, 1 ] ]
+gap> permprops((1,2)(3,4));
+LARGEST_MOVED_POINT_PERM:4
+ORDER_PERM:2
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [ [ 1, 2 ] ]
+gap> permprops((1,5,4,3,2));
+LARGEST_MOVED_POINT_PERM:5
+ORDER_PERM:5
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [ [ 4, 1 ] ]
+gap> permprops((1,2)(3,4)(5,6));
+LARGEST_MOVED_POINT_PERM:6
+ORDER_PERM:2
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 1, 3 ] ]
+gap> permprops((1,2^20));
+LARGEST_MOVED_POINT_PERM:1048576
+ORDER_PERM:2
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 1, 1 ] ]
+gap> permprops((1,2,3,4,5)^5);
+LARGEST_MOVED_POINT_PERM:0
+ORDER_PERM:1
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [  ]
+gap> permprops(PermList(Concatenation([2^17], [1..2^17-1])));
+LARGEST_MOVED_POINT_PERM:131072
+ORDER_PERM:131072
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 131071, 1 ] ]
+
+# Test the boundary between PERM2 and PERM4
+gap> permprops((2^16-1, 2^16));
+LARGEST_MOVED_POINT_PERM:65536
+ORDER_PERM:2
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 1, 1 ] ]
+gap> permprops((2^16-2, 2^16-1));
+LARGEST_MOVED_POINT_PERM:65535
+ORDER_PERM:2
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 1, 1 ] ]
+gap> permprops((2^16-2, 2^16-3));
+LARGEST_MOVED_POINT_PERM:65534
+ORDER_PERM:2
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 1, 1 ] ]
+gap> permprops(PermList(Concatenation([2^16+1], [1..2^16])));
+LARGEST_MOVED_POINT_PERM:65537
+ORDER_PERM:65537
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [ [ 65536, 1 ] ]
+gap> permprops(PermList(Concatenation([2^16], [1..2^16-1])));
+LARGEST_MOVED_POINT_PERM:65536
+ORDER_PERM:65536
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 65535, 1 ] ]
+gap> permprops(PermList(Concatenation([2^16-1], [1..2^16-2])));
+LARGEST_MOVED_POINT_PERM:65535
+ORDER_PERM:65535
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [ [ 65534, 1 ] ]
+gap> permprops(PermList(Concatenation([2^16-2], [1..2^16-3])));
+LARGEST_MOVED_POINT_PERM:65534
+ORDER_PERM:65534
+SIGN_PERM:-1
+CYCLE_SHORT_PERM (short output): [ [ 65533, 1 ] ]
+gap> permprops(PermList(Concatenation([2,1,2^16], [3..2^16-1])));
+LARGEST_MOVED_POINT_PERM:65536
+ORDER_PERM:65534
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [ [ 1, 1 ], [ 65533, 1 ] ]
+gap> permprops(PermList(Concatenation(List([1,3..2^16-1], x -> [x+1,x]))));
+LARGEST_MOVED_POINT_PERM:65536
+ORDER_PERM:2
+SIGN_PERM:1
+CYCLE_SHORT_PERM (short output): [ [ 1, 32768 ] ]
+gap> STOP_TEST("kernel/permutat.tst", 1);