GNU bug report logs - #29066
26.0.90; crash in gc involving buffer local symbols

Previous Next

Full log

View this message in rfc822 format

From: Valentin Gatien-Baron <vgatien-baron <at> janestreet.com>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 29066 <at> debbugs.gnu.org, Mark Shinwell <mshinwell <at> janestreet.com>
Subject: bug#29066: 26.0.90; crash in gc involving buffer local symbols
Date: Tue, 31 Oct 2017 10:52:44 -0400

[Message part 1 (text/plain, inline)]

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

> On Okt 31 2017, Eli Zaretskii <eliz <at> 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;

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.