bug#31238: Dynamic Module Crash

[Philipp Stephani]

Am Sa., 22. Sept. 2018 um 00:48 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> >>> Emacs 25.3.1 crashes with a segfault when trying to print a value
> >>> constructed (in certain ways) by a dynamic module.
> >>> Reproduction: https://github.com/ubolonton/emacs-module-crash
> >>> Context: https://github.com/ubolonton/emacs-module-rs/issues/2
> >> This might be caused by commit 3eb93c07f7a60ac9ce8a16f10c3afd5a3a31243a,
>
> IIUC that's just a wild guess, right?

Let's rather say that it's the obvious suspect. I've now also
confirmed that I can consistently reproduce this bug with commit
3eb93c07f7a60ac9ce8a16f10c3afd5a3a31243a, and that it's consistently
fixed after reverting 3eb93c07f7a60ac9ce8a16f10c3afd5a3a31243a. Since
there was also no consensus to install commit
3eb93c07f7a60ac9ce8a16f10c3afd5a3a31243a in the first place (see
https://lists.gnu.org/archive/html/emacs-devel/2016-01/msg00150.html),
I went ahead and reverted commit
3eb93c07f7a60ac9ce8a16f10c3afd5a3a31243a on master.

>
> >> which removed GC protection for module-allocated values.
>
> Not really, no.  It just relied on another pre-existing mechanism.

A mechanism that can't work. Lisp_Object values in Emacs's source code
might be known to be on the stack, but that's not the case for
modules.

>
> > Unless anybody complains, I'll revert that commit.
>
> Do we even know that reverting it circumvents this crash?

Yes, I've tested this now extensively.

>
> And IIRC this patch is not just an implementation detail, it changes the
> module's API and ABI, so reverting it shouldn't be done lightly.

Commit 3eb93c07f7a60ac9ce8a16f10c3afd5a3a31243a did introduce a
breaking change in the module API in the sense that it removed NULL as
a sentinel value. Reverting that commit restores the original behavior
(which is itself not a breaking change). I don't see any ABI changes
with 3eb93c07f7a60ac9ce8a16f10c3afd5a3a31243a since it didn't change
emacs-module.h (meaningfully).

>
> > Modules can store/use emacs_value objects in arbitrary ways;
> > there's no way we can rely on stack scanning to reliably find them.
>
> We don't blindly rely on stack scanning: there are functions to
> (un)register other values stored in the (non-Lisp) heap.

Do you mean the "global value" function? These are only for values
that have to survive longer than their environment. All other values
live as long as their environment.

>
> The previous code wasn't magical either.

No, and I certainly don't claim it's perfect, but at least it didn't
exhibit this bug. I'm happy to discuss alternatives as long as we (at
least Daniel and I) agree with them and they don't reintroduce this
bug.

>
> I don't know nearly enough about Rust's implementation strategy and the
> `emacs` crate used to guess how its memory management could/should
> interact with Emacs's.

I don't think Rust is the issue here. As with any language exposing a
C interface, it has to play by C's rules for the scope of that
interface.