bug#29066: 26.0.90; crash in gc involving buffer local symbols

[Valentin Gatien-Baron]

On Tue, Oct 31, 2017 at 2:32 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:

On Okt 31 2017, Eli Zaretskii <eliz@gnu.org> wrote:

>> I also checked the following works, and seems better to me (stop having dangling pointers, instead of being
>> careful with them):
>>
>> diff --git a/src/alloc.c b/src/alloc.c
>> index da0c3ad4b3..44dfa95cf5 100644
>> --- a/src/alloc.c
>> +++ b/src/alloc.c
>> @@ -7030,8 +7030,10 @@ sweep_symbols (void)
>>          {
>>            if (!sym->s.gcmarkbit)
>>              {
>> -              if (sym->s.redirect == SYMBOL_LOCALIZED)
>> +              if (sym->s.redirect == SYMBOL_LOCALIZED) {
>>                  xfree (SYMBOL_BLV (&sym->s));
>> +                sym->s.val.blv = NULL;
>> +              }
>
> That was my first attempt, but various macros like SYMBOL_BLV and
> SET_SYMBOL_BLV insist on val.blv being non-NULL.

SET_SYMBOL_BLV doesn't.  And calling SYMBOL_BLV with a freed symbol is a
bug anyway.

​SET_SYMBOL_BLV insists that the new value is not NULL, even if it asserts nothing about the current value.

We do call SYMBOL_BLV after freeing, when we re-sweep the symbol, which is fine because free does nothing when given NULL, but triggers the assertion​.

I would do this, to avoid the assertion failure:

diff --git a/src/alloc.c b/src/alloc.c

index da0c3ad4b3..72550e812b 100644

--- a/src/alloc.c

+++ b/src/alloc.c

@@ -7030,8 +7030,10 @@ sweep_symbols (void)

         {

           if (!sym->s.gcmarkbit)

             {

-              if (sym->s.redirect == SYMBOL_LOCALIZED)

+              if (sym->s.redirect == SYMBOL_LOCALIZED && sym->s.val.blv) {

                 xfree (SYMBOL_BLV (&sym->s));

+                sym->s.val.blv = NULL;

+              }

               sym->s.next = symbol_free_list;

               symbol_free_list = &sym->s;

               symbol_free_list->function = Vdead;

Or changing the redirect type:

diff --git a/src/alloc.c b/src/alloc.c

index da0c3ad4b3..6966d96c6d 100644

--- a/src/alloc.c

+++ b/src/alloc.c

@@ -7030,8 +7030,11 @@ sweep_symbols (void)

         {

           if (!sym->s.gcmarkbit)

             {

-              if (sym->s.redirect == SYMBOL_LOCALIZED)

+              if (sym->s.redirect == SYMBOL_LOCALIZED) {

                 xfree (SYMBOL_BLV (&sym->s));

+                sym->s.redirect = SYMBOL_PLAINVAL;

+                SET_SYMBOL_VAL (&sym->s, Qunbound);

+              }

               sym->s.next = symbol_free_list;

               symbol_free_list = &sym->s;

               symbol_free_list->function = Vdead;