From 8cd28d8a161a18523044cd9e84521d04c439d456 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Mon, 2 Jul 2018 17:51:19 +0200 Subject: [PATCH 1/3] kernel: fix crash in MakeImmutable(rec(x:=~)); Fixes #1912 --- src/precord.c | 6 +++++- tst/testbugfix/2018-07-02-MakeImmutablePRec.tst | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 tst/testbugfix/2018-07-02-MakeImmutablePRec.tst diff --git a/src/precord.c b/src/precord.c index cc1b1cc76b..9c3106b0c6 100644 --- a/src/precord.c +++ b/src/precord.c @@ -245,6 +245,11 @@ void MakeImmutablePRec( Obj rec) UInt len; UInt i; len = LEN_PREC( rec ); + + // change the tnum first, to avoid infinite recursion for objects that + // contain themselves + RetypeBag(rec, IMMUTABLE_TNUM(TNUM_OBJ(rec))); + for ( i = 1; i <= len; i++ ) MakeImmutable(GET_ELM_PREC(rec,i)); @@ -252,7 +257,6 @@ void MakeImmutablePRec( Obj rec) This can never hurt, unless the record will never be accessed again anyway for HPCGAP it's essential so that immutable records are actually binary unchanging */ SortPRecRNam(rec, 1); - RetypeBag(rec, IMMUTABLE_TNUM(TNUM_OBJ(rec))); } diff --git a/tst/testbugfix/2018-07-02-MakeImmutablePRec.tst b/tst/testbugfix/2018-07-02-MakeImmutablePRec.tst new file mode 100644 index 0000000000..75ac0d1063 --- /dev/null +++ b/tst/testbugfix/2018-07-02-MakeImmutablePRec.tst @@ -0,0 +1,7 @@ +# the following used to crash GAP due to infinite recursion +gap> MakeImmutable(rec(x:=~)); +rec( x := ~ ) + +# just to be complete, also test this for plists +gap> MakeImmutable([1,~]); +[ 1, ~ ] From d3c47ecd112482f5976ad15f1d8f4e8de8e4ddf9 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Mon, 2 Jul 2018 17:56:29 +0200 Subject: [PATCH 2/3] kernel: cleanup MakeImmutable{PRec,PlistInHom} --- src/plist.c | 30 +++++++++++++++++------------- src/precord.c | 33 +++++++++++++++++---------------- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/src/plist.c b/src/plist.c index 62b9ee1527..5998070562 100644 --- a/src/plist.c +++ b/src/plist.c @@ -2628,19 +2628,23 @@ Obj FuncASS_PLIST_DEFAULT ( ** (or immutable, but MakeImmutable will have caught that case before we get here) */ -void MakeImmutablePlistInHom( Obj list ) -{ - UInt i; - Obj elm; - RetypeBag( list, IMMUTABLE_TNUM(TNUM_OBJ(list))); - for (i = 1; i <= LEN_PLIST(list); i++) - { - elm = ELM_PLIST( list, i); - if (elm != 0) - { - MakeImmutable( elm ); - CHANGED_BAG(list); - } +void MakeImmutablePlistInHom(Obj list) +{ + // change the tnum first, to avoid infinite recursion for objects that + // contain themselves + RetypeBag(list, IMMUTABLE_TNUM(TNUM_OBJ(list))); + + // FIXME HPC-GAP: there is a potential race here: becomes public + // the moment we change its type, but it's not ready for public access + // until the following code completed. + + UInt len = LEN_PLIST(list); + for (UInt i = 1; i <= len; i++) { + Obj elm = ELM_PLIST(list, i); + if (elm != 0) { + MakeImmutable(elm); + CHANGED_BAG(list); + } } } diff --git a/src/precord.c b/src/precord.c index 9c3106b0c6..58ad17f1ac 100644 --- a/src/precord.c +++ b/src/precord.c @@ -240,23 +240,24 @@ void CleanPRecCopy ( *F MakeImmutablePRec( ) */ -void MakeImmutablePRec( Obj rec) +void MakeImmutablePRec(Obj rec) { - UInt len; - UInt i; - len = LEN_PREC( rec ); - - // change the tnum first, to avoid infinite recursion for objects that - // contain themselves - RetypeBag(rec, IMMUTABLE_TNUM(TNUM_OBJ(rec))); - - for ( i = 1; i <= len; i++ ) - MakeImmutable(GET_ELM_PREC(rec,i)); - - /* Sort the record at this point. - This can never hurt, unless the record will never be accessed again anyway - for HPCGAP it's essential so that immutable records are actually binary unchanging */ - SortPRecRNam(rec, 1); + // change the tnum first, to avoid infinite recursion for objects that + // contain themselves + RetypeBag(rec, IMMUTABLE_TNUM(TNUM_OBJ(rec))); + + // FIXME HPC-GAP: there is a potential race here: becomes public + // the moment we change its type, but it's not ready for public access + // until the following code completed. + + UInt len = LEN_PREC(rec); + for (UInt i = 1; i <= len; i++) + MakeImmutable(GET_ELM_PREC(rec, i)); + + // Sort the record at this point. This can never hurt, unless the record + // will never be accessed again anyway. But for HPC-GAP it is essential so + // that immutable records are actually binary unchanging. + SortPRecRNam(rec, 1); } From 5dcde4ae318e29eb1bf7f46474fa3de2a4bfe3bc Mon Sep 17 00:00:00 2001 From: Max Horn Date: Mon, 2 Jul 2018 17:58:22 +0200 Subject: [PATCH 3/3] kernel: remove unnecessary CHANGED_BAG --- src/plist.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/plist.c b/src/plist.c index 5998070562..0073a85874 100644 --- a/src/plist.c +++ b/src/plist.c @@ -2643,7 +2643,6 @@ void MakeImmutablePlistInHom(Obj list) Obj elm = ELM_PLIST(list, i); if (elm != 0) { MakeImmutable(elm); - CHANGED_BAG(list); } } }