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

[Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@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?

> 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.

>> >> 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.  Unless there happens to be another
ambiguous reference to the string data (which means MPS cannot move the
string data, because it cannot alter ambiguous references), an
unprotected static Lisp_Object will most likely point to invalid data
after MPS GC runs.

> The value comes from 'string', a different variable.  It

Which might be in a register and not survive until GC is triggered.  Or
it might be a global/static variable which is in an exact root, which
means the data can be moved, 'string' will be updated, 'unmarked' won't.

> points to string data, and it's that string data that is of interest
> to us in the above snippet.

>> > 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.

>> >> > To clarify, I was trying to understand whether the error message
>> >> > reported by Ihor in another thread could have happened because of GC
>> >> > in this are of the code.
>> >>
>> >> I currently think that Ihor's test case calls execve () with nonsensical
>> >> "environment" strings a lot, and once in a while they'll even be behind
>> >> the barrier which causes an EFAULT.
>> >
>> > 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().

That we have a specific code path which ends up in MPS code is a bonus.

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.

(Just answering the GC questions for now, sorry).

Pip