bug#61960: 30.0.50; Unexec build reliably crashes during loadup

[Konstantin Kharlamov]

On Sun, 2023-07-02 at 08:52 +0300, Eli Zaretskii wrote:
> > From: Konstantin Kharlamov <hi-angel@yandex.ru>
> > Date: Sun, 02 Jul 2023 04:50:26 +0300
> > 
> > I've found a diff that fixes the build, but whether it's okay is worth
> > discussion:
> > 
> >     diff --git a/src/gmalloc.c b/src/gmalloc.c
> >     index e655d69f660..f49bb01e08b 100644
> >     --- a/src/gmalloc.c
> >     +++ b/src/gmalloc.c
> >     @@ -1704,7 +1704,7 @@ allocated_via_gmalloc (void *ptr)
> >          return false;
> >        size_t block = BLOCK (ptr);
> >        size_t blockmax = _heaplimit - 1;
> >     -  return block <= blockmax && _heapinfo[block].busy.type != 0;
> >     +  return block <= blockmax;
> >      }
> > 
> >      /* See the comments near the beginning of this file for explanations
> > 
> > Here's what happens: Emacs uses internal stack-based allocator (apparently
> > allocating
> > with sbrk(), but I'm not sure) along with the system allocator. Whenever a
> > memory is
> > allocated from the internal allocator, you can't call `free()` on it.
> > 
> > When Emacs wants to free memory, it calls `hybrid_free_1()`, which
> > internally
> > determines whether the `ptr` passed belongs to system heap or to Emacs
> > stack. Determining in turn is done by `allocated_via_gmalloc()`.
> > 
> > Emacs also keeps the lowest and highest boundary of this stack in variables
> > `_heapbase` and `_heaplimit` accordingly (except the latter is measured in
> > "blocks"). The code in diff `block <= blockmax` simply makes sure that the
> > `ptr`
> > passed is within the stack-allocated memory, which implies it can't be
> > deallocated
> > with `free()`
> > 
> > There's a question though of the right-hand side that I remove, the
> > `_heapinfo[block].busy.type != 0;`. Apparently the `type` should keep some
> > memory
> > info, and apparently there's a bug somewhere that screws it up. It is a bug
> > worth
> > fixing, although for some reason `rr replay` doesn't work for me with
> > `temacs`
> > (probably a bug in rr), and without reverse-execution tracking that down
> > would be
> > very hard.
> > 
> > But I would argue that the right-hand side check has no value in this
> > function,
> > because to determine the source of allocation it's enough to just check
> > whether `ptr`
> > is in _heapbase .. _heaplimit range (barring the fact they're different
> > units).
> 
> Thanks, but how do you explain that this code works as-is when the
> BLOCK_ALIGN change is not used?

I don't know exactly. But I've read calculations in alloc.c where BLOCK_ALIGN is
used (directly and indirectly) and the code there is very convoluted. Apparently
an offset somewhere gets calculated incorrectly, so some
`_heapinfo[block].busy.type` fields end up having 0 despite being allocated with
malloc(). As I mentioned it is worth investigating, but without reverse-
execution (as `rr` for some reason doesn't work with `temacs` in this case) it
will be hard to pinpoint.

I am planning to report the `rr` problem to them, so hopefully it will be
possible to find the real culprit in the future.