bug#75322: SAFE_ALLOCA assumed to root Lisp_Objects/SSDATA(string)

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sat, 04 Jan 2025 21:15:50 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: gerd.moellmann@gmail.com, 75322@debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> >> > The safe thing is to re-initialize the pointer from the string
> >> > whenever we think GC could happen, and otherwise make sure GC cannot
> >> > happen.
> >>
> >> For the old GC, yes.  For the new GC, the safe thing would be to ensure
> >> MPS has removed all memory barriers, take the arena lock, then call the
> >> syscall and return.  Or use xstrdup.
> >
> > If this is indeed so (and I don't think it is), then we need to
> 
> Which part do you think is wrong?

All of what you describe.  We should be able to use without
restrictions local variables whose values are Lisp strings, and we
should be able to use without restrictions static variables whose
values are Lisp strings.  To use the 'char *' pointer to a Lisp
string's data we should make sure GC cannot happen between the time we
extract the pointer using SDATA or SSDATA and the time we end the use
of the pointer.

If you think anything in the above is not correct with MPS, please
tell.  Please only respond with real practical problems, to keep this
discussion concrete and on-topic.

> > discuss it very thoroughly, because it basically means we cannot do
> > anything with Lisp strings in C.  For example, the display iterator
> > has a member that is a Lisp string, which is used when we produce
> > glyphs for display from a string (such as a display property or an
> > overlay before/after string) -- what you say here basically means that
> > we cannot carry that string around, right?  If not, how is that
> > different?
> 
> Of course we can use Lisp strings.  As long as there's an automatic
> variable pointing to the string data, it'll stay there.  If there's a
> static variable pointing to the string data, it might move, but then the
> static variable will be updated.

Then I don't understand why you seem to be saying that our usual use
patterns of Lisp strings and their textual data is unsafe and needs to
be amended by using xstrdup etc.  We do it already as described above.

> >> >> If a pointer to "old" data is ever exposed to Emacs, we lose, because
> >> >> MPS will reuse the memory for new data, which might be behind a barrier.
> >> >>
> >> >> If we ever do:
> >> >>
> >> >>    static Lisp_Object unmarked;
> >>       ^^^^^^
> >> >>    unmarked = string;
> >> >>    ... trigger GC here ...
> >> >>    puts (SDATA (unmarked);
> >> >>
> >> >> the most likely outcome (thanks to Gerd for the hint) is that
> >> >> nonsensical data is printed
> >> >
> >> > Are you sure?
> >>
> >> With the static keyword, yes.  (Assuming we don't staticpro the static
> >> variable, of course).
> >
> > What does static have to do with this?
> 
> What matters is whether the value is visible to the garbage collector
> (in which case it remains a valid pointer) or isn't (in which case the
> memory it points to is used for something else).
> 
> Automatic variables, residing on the stack or residing in a register
> (which is then spilled to the stack) protect the memory they point to.
> Static variables don't unless we tell GC about them.
> 
> > What matters is the value, not the storage.
> 
> I have no idea what you mean by that.  The value of the variable is a
> tagged pointer.  It won't change during GC, because GC never alters
> automatic variables.  The question is whether this pointer still points
> to the right data area after GC.

The pointer taken before GC might not point to the string's data
after GC.  Which is why, if we know GC could happen between two points
in a program's code, we cannot use the same 'char *' pointer there;
instead, we must recompute the pointer (by using SDATA) after the
possible GC.

> > The value comes from 'string', a different variable.  It
> 
> Which might be in a register and not survive until GC is triggered.

A Lisp_Object variable will survive.  Its pointer will be updated if
needed to point to the new location of the data.  Thus, using the
Lisp_Object variable is always safe, but the pointer to its data must
be updated after a potential GC.

> >> > The below is indeed unsafe:
> >> >
> >> >   char *p = SDATA (unmarked);
> >> >   ... trigger GC here ...
> >> >   puts (p);
> >>
> >> Right now, that's safe for MPS, but not for the old GC, correct?
> >
> > If GC moves string data, then it is not safe, period.  Does MPS move
> > string data?
> 
> MPS does not move string data if there is a stack variable pointing to
> it.  It does in other cases.  This is why it's safe for MPS.  The old
> GC, IIUC, is less forgiving.

The conclusion is that the above is NOT safe, because in some cases
the data could move.  Which was what I said from the get-go.

> >> > Before we agree that this could be the reason, we'd need to find the
> >> > places where GC could reuse the memory of a live string, while that
> >> > string appears in some live data structure, and as long as no GC can
> >> > happen between the time we put the SDATA pointers into the environment
> >> > array and the time posix_spawn returns.
> >>
> >> Calling igc_collect () right before the spawn results in corrupt data:
> >
> > But the code doesn't call igc_collect, so how is this relevant?
> 
> This is all that is relevant to establishing the current code is not
> safe for the intended behavior of scratch/igc, which is to allow garbage
> collection at this point, equivalent to setting a breakpoint there and
> calling igc_collect().

Why would we allow GC at some point where the code clearly does NOT
expect GC to happen?

We are discussing whether some code in callproc.c needs to be changed
or is okay as it is.  If it is okay because GC cannot possibly happen
while some code fragment is being executed, then forcefully injecting
a call to GC there is pointless, because it changes the conditions
under which this code normally runs.  So please keep such arguments
out of this discussion, they just muddy the waters and are not useful.

> I honestly don't know whether hitting a memory barrier can result in
> other data being moved by MPS right now.  I'm assuming it can, and we
> should not assume string data is protected from being moved unless we
> root it explicitly.

We should not assume anything, we should have a clear understanding
what does and what doesn't move the data and the pointers.  We cannot
possibly analyze the code otherwise, or at least I cannot.

To keep this discussion focused, let's please return to looking at
some particular code in callproc.c where you think SAFE_ALLOCA is
unsafe.