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

[Valentin Gatien-Baron <vgatien-baron@janestreet.com>]

[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]

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;

[-- Attachment #2: Type: text/html, Size: 5006 bytes --]