GNU bug report logs - #65796
dynamic module non_local_exit_get overwrites exit signals

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 65796 in the body.
You can then email your comments to 65796 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Xinyang Chen <chenxinyang99 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: dynamic module non_local_exit_get overwrites exit signals
Date: Wed, 6 Sep 2023 18:52:14 -0400

[Message part 1 (text/plain, inline)]

Currently `module_non_local_exit_get` returns pointers to fields
in emacs_env_private:
```
  if (p->pending_non_local_exit != emacs_funcall_exit_return)
    {
      *symbol = &p->non_local_exit_symbol;
      *data = &p->non_local_exit_data;
    }
```
this means that if one tries to:
```
funcall(...);
non_local_exit_get(&s1, &d1);
funcall(...);
non_local_exit_get(&s2, &d2);
non_local_exit_signal(s1, d1);
```
you would signal the second error, instead of the first error (I expected
this to happen).
It seems to me that `module_non_local_exit_get` should
`allocate_emacs_value` instead.

-Alan

[Message part 2 (text/html, inline)]

Message #8 received at 65796 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Xinyang Chen <chenxinyang99 <at> gmail.com>, Philipp Stephani <phst <at> google.com>,
 Daniel Colascione <dancol <at> dancol.org>
Cc: 65796 <at> debbugs.gnu.org
Subject: Re: bug#65796: dynamic module non_local_exit_get overwrites exit
 signals
Date: Thu, 07 Sep 2023 10:07:33 +0300

> From: Xinyang Chen <chenxinyang99 <at> gmail.com>
> Date: Wed, 6 Sep 2023 18:52:14 -0400
> 
> Currently `module_non_local_exit_get` returns pointers to fields
> in emacs_env_private:
> ```
>   if (p->pending_non_local_exit != emacs_funcall_exit_return)
>     {
>       *symbol = &p->non_local_exit_symbol;
>       *data = &p->non_local_exit_data;
>     }
> ```
> this means that if one tries to:
> ```
> funcall(...);
> non_local_exit_get(&s1, &d1);
> funcall(...);
> non_local_exit_get(&s2, &d2);
> non_local_exit_signal(s1, d1);
> ```
> you would signal the second error, instead of the first error (I expected
> this to happen).
> It seems to me that `module_non_local_exit_get` should
> `allocate_emacs_value` instead.

Philipp, Daniel: any comments?

Btw, the non_local_exit_get function is currently not documented in
the ELisp manual; should it be?

Message #11 received at 65796 <at> debbugs.gnu.org (full text, mbox):

From: Philipp Stephani <phst <at> google.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Xinyang Chen <chenxinyang99 <at> gmail.com>, p.stephani2 <at> gmail.com,
 Daniel Colascione <dancol <at> dancol.org>, 65796 <at> debbugs.gnu.org
Subject: Re: bug#65796: dynamic module non_local_exit_get overwrites exit
 signals
Date: Thu, 7 Sep 2023 12:24:03 +0200

On Thu, 7 Sept 2023 at 09:07, Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Xinyang Chen <chenxinyang99 <at> gmail.com>
> > Date: Wed, 6 Sep 2023 18:52:14 -0400
> >
> > Currently `module_non_local_exit_get` returns pointers to fields
> > in emacs_env_private:
> > ```
> >   if (p->pending_non_local_exit != emacs_funcall_exit_return)
> >     {
> >       *symbol = &p->non_local_exit_symbol;
> >       *data = &p->non_local_exit_data;
> >     }
> > ```
> > this means that if one tries to:
> > ```
> > funcall(...);
> > non_local_exit_get(&s1, &d1);
> > funcall(...);
> > non_local_exit_get(&s2, &d2);
> > non_local_exit_signal(s1, d1);
> > ```
> > you would signal the second error, instead of the first error (I expected
> > this to happen).
> > It seems to me that `module_non_local_exit_get` should
> > `allocate_emacs_value` instead.
>
> Philipp, Daniel: any comments?

Nice find!
We can't use allocate_emacs_value here because non_local_exit_get has
to work in OOM situations and can never fail. What we could do here is
e.g.:
- Document the current behavior, stating that the emacs_value objects
returned from non_local_exit_get are ephemeral. I'm not a huge fan of
this because it makes non_local_exit_get behave different from all
other functions.
- Provide an alternative non_local_exit_copy that copies the 2
Lisp_Objects into an opaque buffer supplied by the user (plus a way to
determine the buffer size). That way we shift the responsibility of
dealing with allocation failures to the user.
- Attempt to allocate a new emacs_value, fall back to the current
behavior if that fails. I don't really like that option either because
it doesn't solve the initial problem in all cases (so users still need
to deal with it), but makes both the interface and the implementation
more complex.
- Crash if we can't allocate memory. That has been rejected in other cases.

>
> Btw, the non_local_exit_get function is currently not documented in
> the ELisp manual; should it be?

At least in Emacs 29 I see it documented ("Module Nonlocal" node).


--
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise
erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes
weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich
bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

This e-mail is confidential. If you received this communication by
mistake, please don’t forward it to anyone else, please erase all
copies and attachments, and please let me know that it has gone to the
wrong person.

Message #14 received at 65796 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philipp Stephani <phst <at> google.com>
Cc: chenxinyang99 <at> gmail.com, p.stephani2 <at> gmail.com, dancol <at> dancol.org,
 65796 <at> debbugs.gnu.org
Subject: Re: bug#65796: dynamic module non_local_exit_get overwrites exit
 signals
Date: Thu, 07 Sep 2023 13:55:09 +0300

> From: Philipp Stephani <phst <at> google.com>
> Date: Thu, 7 Sep 2023 12:24:03 +0200
> Cc: Xinyang Chen <chenxinyang99 <at> gmail.com>, Daniel Colascione <dancol <at> dancol.org>, 65796 <at> debbugs.gnu.org, 
> 	p.stephani2 <at> gmail.com
> 
> Nice find!
> We can't use allocate_emacs_value here because non_local_exit_get has
> to work in OOM situations and can never fail. What we could do here is
> e.g.:
> - Document the current behavior, stating that the emacs_value objects
> returned from non_local_exit_get are ephemeral. I'm not a huge fan of
> this because it makes non_local_exit_get behave different from all
> other functions.
> - Provide an alternative non_local_exit_copy that copies the 2
> Lisp_Objects into an opaque buffer supplied by the user (plus a way to
> determine the buffer size). That way we shift the responsibility of
> dealing with allocation failures to the user.
> - Attempt to allocate a new emacs_value, fall back to the current
> behavior if that fails. I don't really like that option either because
> it doesn't solve the initial problem in all cases (so users still need
> to deal with it), but makes both the interface and the implementation
> more complex.
> - Crash if we can't allocate memory. That has been rejected in other cases.

I guess just one alternative is acceptable?

> > Btw, the non_local_exit_get function is currently not documented in
> > the ELisp manual; should it be?
> 
> At least in Emacs 29 I see it documented ("Module Nonlocal" node).

I need new glasses.

Message #17 received at 65796 <at> debbugs.gnu.org (full text, mbox):

From: Xinyang Chen <chenxinyang99 <at> gmail.com>
To: Philipp Stephani <phst <at> google.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Daniel Colascione <dancol <at> dancol.org>,
 65796 <at> debbugs.gnu.org, p.stephani2 <at> gmail.com
Subject: Re: bug#65796: dynamic module non_local_exit_get overwrites exit
 signals
Date: Wed, 29 Jan 2025 11:52:40 -0600

[Message part 1 (text/plain, inline)]

This is still a problem.

Using a user buffer will require gc-protecting it and thus a major
overhaul, so I think it's not a good idea.

IMO what we should do is: if we fail to allocate, we discard the original
signal and replace it with an OOM signal (pointing to constants so
requiring no allocation). Perhaps we should make a new field
in emacs_funcall_exit for OOM, or we can just use emacs_funcall_exit_signal.

Alternatively, make a copy_emacs_value function that allows the user to
copy the signal out, returning NULL to let the caller know that an
allocation failure occurred.



On Thu, Sep 7, 2023 at 5:24 AM Philipp Stephani <phst <at> google.com> wrote:

> On Thu, 7 Sept 2023 at 09:07, Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> > > From: Xinyang Chen <chenxinyang99 <at> gmail.com>
> > > Date: Wed, 6 Sep 2023 18:52:14 -0400
> > >
> > > Currently `module_non_local_exit_get` returns pointers to fields
> > > in emacs_env_private:
> > > ```
> > >   if (p->pending_non_local_exit != emacs_funcall_exit_return)
> > >     {
> > >       *symbol = &p->non_local_exit_symbol;
> > >       *data = &p->non_local_exit_data;
> > >     }
> > > ```
> > > this means that if one tries to:
> > > ```
> > > funcall(...);
> > > non_local_exit_get(&s1, &d1);
> > > funcall(...);
> > > non_local_exit_get(&s2, &d2);
> > > non_local_exit_signal(s1, d1);
> > > ```
> > > you would signal the second error, instead of the first error (I
> expected
> > > this to happen).
> > > It seems to me that `module_non_local_exit_get` should
> > > `allocate_emacs_value` instead.
> >
> > Philipp, Daniel: any comments?
>
> Nice find!
> We can't use allocate_emacs_value here because non_local_exit_get has
> to work in OOM situations and can never fail. What we could do here is
> e.g.:
> - Document the current behavior, stating that the emacs_value objects
> returned from non_local_exit_get are ephemeral. I'm not a huge fan of
> this because it makes non_local_exit_get behave different from all
> other functions.
> - Provide an alternative non_local_exit_copy that copies the 2
> Lisp_Objects into an opaque buffer supplied by the user (plus a way to
> determine the buffer size). That way we shift the responsibility of
> dealing with allocation failures to the user.
> - Attempt to allocate a new emacs_value, fall back to the current
> behavior if that fails. I don't really like that option either because
> it doesn't solve the initial problem in all cases (so users still need
> to deal with it), but makes both the interface and the implementation
> more complex.
> - Crash if we can't allocate memory. That has been rejected in other cases.
>
> >
> > Btw, the non_local_exit_get function is currently not documented in
> > the ELisp manual; should it be?
>
> At least in Emacs 29 I see it documented ("Module Nonlocal" node).
>
>
> --
> Google Germany GmbH
> Erika-Mann-Straße 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Liana Sebastian
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
>
> Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise
> erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes
> weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich
> bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.
>
> This e-mail is confidential. If you received this communication by
> mistake, please don’t forward it to anyone else, please erase all
> copies and attachments, and please let me know that it has gone to the
> wrong person.
>

[Message part 2 (text/html, inline)]

Message #20 received at 65796 <at> debbugs.gnu.org (full text, mbox):

From: Philipp Stephani <p.stephani2 <at> gmail.com>
To: Xinyang Chen <chenxinyang99 <at> gmail.com>
Cc: Philipp Stephani <phst <at> google.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Daniel Colascione <dancol <at> dancol.org>, 65796 <at> debbugs.gnu.org
Subject: Re: bug#65796: dynamic module non_local_exit_get overwrites exit
 signals
Date: Tue, 25 Feb 2025 15:43:13 +0100

[Message part 1 (text/plain, inline)]

> Am 29.01.2025 um 18:52 schrieb Xinyang Chen <chenxinyang99 <at> gmail.com>:
> 
> This is still a problem.
> 
> Using a user buffer will require gc-protecting it and thus a major overhaul, so I think it's not a good idea.

Yeah, I figured out that approach is a dead end meanwhile.

> 
> IMO what we should do is: if we fail to allocate, we discard the original signal and replace it with an OOM signal (pointing to constants so requiring no allocation).

Yeah, that's a good idea, thanks for bringing it up.  I've attached a patch to that effect.

> Perhaps we should make a new field in emacs_funcall_exit for OOM, or we can just use emacs_funcall_exit_signal.

My patch does the latter: Adding a new enum value risks UB if callers don't have a default case in their switch statements, behavior in OOM situations is best-effort anyway, and very careful callers can still compare the returned error symbol against the (documented) OOM symbol.

> 
> Alternatively, make a copy_emacs_value function that allows the user to copy the signal out, returning NULL to let the caller know that an allocation failure occurred.

I also considered that, but it puts too much onus on the module authors to deal with a situation that effectively never happens.

> 
> 
> 
> On Thu, Sep 7, 2023 at 5:24 AM Philipp Stephani <phst <at> google.com> wrote:
> On Thu, 7 Sept 2023 at 09:07, Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> > > From: Xinyang Chen <chenxinyang99 <at> gmail.com>
> > > Date: Wed, 6 Sep 2023 18:52:14 -0400
> > >
> > > Currently `module_non_local_exit_get` returns pointers to fields
> > > in emacs_env_private:
> > > ```
> > >   if (p->pending_non_local_exit != emacs_funcall_exit_return)
> > >     {
> > >       *symbol = &p->non_local_exit_symbol;
> > >       *data = &p->non_local_exit_data;
> > >     }
> > > ```
> > > this means that if one tries to:
> > > ```
> > > funcall(...);
> > > non_local_exit_get(&s1, &d1);
> > > funcall(...);
> > > non_local_exit_get(&s2, &d2);
> > > non_local_exit_signal(s1, d1);
> > > ```
> > > you would signal the second error, instead of the first error (I expected
> > > this to happen).
> > > It seems to me that `module_non_local_exit_get` should
> > > `allocate_emacs_value` instead.
> >
> > Philipp, Daniel: any comments?
> 
> Nice find!
> We can't use allocate_emacs_value here because non_local_exit_get has
> to work in OOM situations and can never fail. What we could do here is
> e.g.:
> - Document the current behavior, stating that the emacs_value objects
> returned from non_local_exit_get are ephemeral. I'm not a huge fan of
> this because it makes non_local_exit_get behave different from all
> other functions.
> - Provide an alternative non_local_exit_copy that copies the 2
> Lisp_Objects into an opaque buffer supplied by the user (plus a way to
> determine the buffer size). That way we shift the responsibility of
> dealing with allocation failures to the user.
> - Attempt to allocate a new emacs_value, fall back to the current
> behavior if that fails. I don't really like that option either because
> it doesn't solve the initial problem in all cases (so users still need
> to deal with it), but makes both the interface and the implementation
> more complex.
> - Crash if we can't allocate memory. That has been rejected in other cases.
> 
> >
> > Btw, the non_local_exit_get function is currently not documented in
> > the ELisp manual; should it be?
> 
> At least in Emacs 29 I see it documented ("Module Nonlocal" node).

[0001-Don-t-overwrite-non-local-exit-symbol-and-data-Bug-6.patch (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

Message #27 received at 65796-done <at> debbugs.gnu.org (full text, mbox):

From: Philipp Stephani <p.stephani2 <at> gmail.com>
To: Xinyang Chen <chenxinyang99 <at> gmail.com>
Cc: Philipp Stephani <phst <at> google.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Daniel Colascione <dancol <at> dancol.org>, 65796-done <at> debbugs.gnu.org
Subject: Re: bug#65796: dynamic module non_local_exit_get overwrites exit
 signals
Date: Fri, 28 Feb 2025 02:15:06 +0100

> Am 25.02.2025 um 15:43 schrieb Philipp Stephani <p.stephani2 <at> gmail.com>:
> 
>> 
>> IMO what we should do is: if we fail to allocate, we discard the original signal and replace it with an OOM signal (pointing to constants so requiring no allocation).
> 
> Yeah, that's a good idea, thanks for bringing it up.  I've attached a patch to that effect.

Pushed as commit 32da093e524d5e28945557701f7c50d7c4a898cd.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.