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

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sat, 04 Jan 2025 15:26:01 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: gerd.moellmann@gmail.com, 75322@debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> > AFAICT, expand-file-name is called before we start using the SSDATA of
> > strings in the args[] array.  Or what did I miss?
> 
> You're right, thanks.  I got confused between args and new_argv.
> 
> The next thing I'd look at is the final call to ENCODE_FILE, called
> after new_argv is set up; this ends up in encode_coding_object, which
> calls safe_funcall in what seems to me to be an unlikely but possible
> code path.  I assume that's unsafe (the safe_ refers to redisplay, not
> GC, IIUC)?

ENCODE_FILE can indeed trigger GC in rare cases, but I see only one
such call:

  path = ENCODE_FILE (path);

We could simply move this to before the loop that sets up new_argv[].

> While maybe_quit is "safe" because it inhibits GC, I believe it can
> still call the debugger, which might require more memory than is
> available until GC is re-enabled.

maybe_quit is not the problem, the problem AFAIU is that any encoding
could have pre-write-conversion function written in Lisp.

> >> Yes, make_environment_block does say its callers can't run GC, but
> >> call_process doesn't indicate when and how it establishes a no-GC
> >> assumption.
> >
> > What would be needed to indicate that?
> 
> I'd prefer a pair of macros (no-ops in regular builds) to comments, but
> there is no obvious best solution here.

Sorry, I don't understand: why macros?  Do we use something like that
anywhere else in Emacs?

> My proposal would be to remove most (ideally, all, and then we're done)
> no-GC assumptions, and put the few ones that must remain into separate
> function bodies for the no-GC / possible-GC cases.  Then we can put the
> no-GC function bodies into a separate section and prohibit references
> from no-GC to possible-GC functions, and have the linker check that.

First, the techniques that rely on separate segments are non-portable,
so not all platforms will support them.  More importantly, I'm afraid
the amount of code where we currently don't expect GC is much more
than you seem to imagine, so I don't think the above is practical.

In any case, the starting point is to audit all the places where GC
can happen, and that needs to be done anyway if we want to do it
thoroughly and proactively (as opposed to only when someone reports a
bug).  We can delay the decision what to do with these places to
later, when we understand better the magnitude of the problem and its
various aspects (in terms of several different effects that GC can
have on various objects).

> >> As we agreed, code should be written to assume GC can strike at any
> >> time.
> >
> > I don't think we agreed to that.  At least I didn't, not in this
> > general form.  It would be a huge job to make all of our code comply
> > with this.
> 
> I said "That's what you should think: GC can strike at any time", and
> you said "The same is true with the old GC".  I misread that as
> agreement.

I only meant that MPS is not very different from the old GC in this
regard, that's all.

> >> IIUC, Gerd explained that the old GC can still move the string *data*
> >> used in that structure, even if the string metadata stays in place.
> >
> > If string data is moved, then accessing the old pointer would trigger
> > the barrier and MPS will do its thing, not?
> 
> Sorry, but I think I'm confused here.
> 
> IIUC, MPS doesn't currently use barriers on data that is moved (it
> could, so the data is copied lazily, but I don't think that's what you
> meant), it uses barriers on data that contains references that may not
> have been fixed.

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.

> 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?  AFAIU, referencing 'unmarked' is safe, even after GC.
The below is indeed unsafe:

  char *p = SDATA (unmarked);
  ... trigger GC here ...
  puts (p);

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