diff --git a/src/plist.c b/src/plist.c index 62b9ee1527..0073a85874 100644 --- a/src/plist.c +++ b/src/plist.c @@ -2628,19 +2628,22 @@ Obj FuncASS_PLIST_DEFAULT ( ** (or immutable, but MakeImmutable will have caught that case before we get here) */ -void MakeImmutablePlistInHom( Obj list ) +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); - } + // 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); + } } } diff --git a/src/precord.c b/src/precord.c index cc1b1cc76b..58ad17f1ac 100644 --- a/src/precord.c +++ b/src/precord.c @@ -240,19 +240,24 @@ void CleanPRecCopy ( *F MakeImmutablePRec( ) */ -void MakeImmutablePRec( Obj rec) +void MakeImmutablePRec(Obj rec) { - UInt len; - UInt i; - len = LEN_PREC( 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); - RetypeBag(rec, IMMUTABLE_TNUM(TNUM_OBJ(rec))); + // 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); } 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, ~ ]