bug#7496: 23.2; copy recursive keymap cause crash

bug#7496: 23.2; copy recursive keymap cause crash

[ARISAWA Akihiro]

In GNU Emacs 23.2.1 (i486-pc-linux-gnu, GTK+ Version 2.12.12)
 of 2010-05-15 on nagi, modified by Debian
configured using `configure  '--build' 'i486-linux-gnu' '--build' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.2/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.2/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Hi,

When I call `copy-keymap' with a keymap which contains recursive binding,
the emacs crashed.
I can reproduce it by following sexp.

(let ((map (make-sparse-keymap)))
  (define-key map " " map)
  (copy-keymap map))

Regards,
ARISAWA

bug#7496: 23.2; copy recursive keymap cause crash

[Stefan Monnier]

> When I call `copy-keymap' with a keymap which contains recursive binding,
> the emacs crashed.
> I can reproduce it by following sexp.

> (let ((map (make-sparse-keymap)))
>   (define-key map " " map)
>   (copy-keymap map))

I'm not surprised.  There are many ways to address it:
- try and make sure we better handle the "using up all memory" case
  rather than crashing.  This is very difficult.  We already try to do
  it, but clearly it's not working that well.
- try and detect such cycles and either signal an error or reproduce the
  same cycle in the copy.  We have added such things in several other
  cases, so we should probably do that.
- don't use cyclic keymaps and especially don't copy them.
- don't use copy-keymap, instead: inherit.


        Stefan

bug#7496: 23.2; copy recursive keymap cause crash

[ARISAWA Akihiro]

Stefan Monnier writes:

>> When I call `copy-keymap' with a keymap which contains recursive binding,
>> the emacs crashed.
> - don't use cyclic keymaps and especially don't copy them.
> - don't use copy-keymap, instead: inherit.

OK.
I use cycle binding in global-map, and the evernote-mode.el calls
(copy-keymap global-map).
http://code.google.com/p/emacs-evernote-mode/

So, I will suggest to the author that evernote-mode.el uses
set-keymap-parent instead of copy-keymap.

Regards,
ARISAWA

bug#7496: 23.2; copy recursive keymap cause crash

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> When I call `copy-keymap' with a keymap which contains recursive binding,
>> the emacs crashed.
>> I can reproduce it by following sexp.
>
>> (let ((map (make-sparse-keymap)))
>>   (define-key map " " map)
>>   (copy-keymap map))
>
> I'm not surprised.  There are many ways to address it:
> - try and make sure we better handle the "using up all memory" case
>   rather than crashing.  This is very difficult.  We already try to do
>   it, but clearly it's not working that well.
> - try and detect such cycles and either signal an error or reproduce the
>   same cycle in the copy.  We have added such things in several other
>   cases, so we should probably do that.

While this is a pretty obscure, Emacs shouldn't crash on stuff like
this.  I first considered whether just to check for EQ in Fcopy_keymap,
but it's possible to have nested keymaps that are mutually recursive,
so that won't work.

So I just added a recursion counter and refuse to copy when we've
reached level 100.

It does not protect against the case where the keymap is a char table
where one of the entries is the same keymap, but I don't know whether
that's a thing.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#7496: 23.2; copy recursive keymap cause crash

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 13 Oct 2019 03:15:17 +0200
> Cc: ARISAWA Akihiro <ari@mbf.ocn.ne.jp>, 7496@debbugs.gnu.org
> 
> >> (let ((map (make-sparse-keymap)))
> >>   (define-key map " " map)
> >>   (copy-keymap map))
> >
> > I'm not surprised.  There are many ways to address it:
> > - try and make sure we better handle the "using up all memory" case
> >   rather than crashing.  This is very difficult.  We already try to do
> >   it, but clearly it's not working that well.
> > - try and detect such cycles and either signal an error or reproduce the
> >   same cycle in the copy.  We have added such things in several other
> >   cases, so we should probably do that.
> 
> While this is a pretty obscure, Emacs shouldn't crash on stuff like
> this.  I first considered whether just to check for EQ in Fcopy_keymap,
> but it's possible to have nested keymaps that are mutually recursive,
> so that won't work.
> 
> So I just added a recursion counter and refuse to copy when we've
> reached level 100.

What happens in the current master without that limitation?  We have
since added stack overflow protection -- doesn't it kick in in this
case?

bug#7496: 23.2; copy recursive keymap cause crash

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> What happens in the current master without that limitation?  We have
> since added stack overflow protection -- doesn't it kick in in this
> case?

No, Emacs just crashes hard.  I haven't examined why, though -- I didn't
know about the new stack overflow protection.

It's easy to reproduce, though: Just eval this form and Emacs will
segfault (if it's older than yesterday's):

(let ((map (make-sparse-keymap)))
  (define-key map " " map)
  (copy-keymap map))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#7496: 23.2; copy recursive keymap cause crash

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: monnier@iro.umontreal.ca,  ari@mbf.ocn.ne.jp,  7496@debbugs.gnu.org
> Date: Sun, 13 Oct 2019 19:56:47 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What happens in the current master without that limitation?  We have
> > since added stack overflow protection -- doesn't it kick in in this
> > case?
> 
> No, Emacs just crashes hard.  I haven't examined why, though -- I didn't
> know about the new stack overflow protection.

Maybe because the stack overflow happens during GC.

bug#7496: 23.2; copy recursive keymap cause crash

[Stefan Monnier]

> (let ((map (make-sparse-keymap)))
>   (define-key map " " map)
>   (copy-keymap map))

BTW, copy-keymap should pretty much never be used.
And as a consequence its performance is largely irrelevant, so it could
be re-implemented in Elisp ;-)


        Stefan

bug#7496: 23.2; copy recursive keymap cause crash

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> BTW, copy-keymap should pretty much never be used.
> And as a consequence its performance is largely irrelevant, so it could
> be re-implemented in Elisp ;-)

Yes, I wondered whether I should just do that -- almost everything it
does is available from Lisp land, I think -- except map_char_table,
which I didn't investigate closely.

(copy-keymap is used 76 times in the Emacs tree.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#7496: 23.2; copy recursive keymap cause crash

[Stefan Monnier]

> (copy-keymap is used 76 times in the Emacs tree.)

Yes, I know.  I've had it in my TODO to get this number down, but never
got around to it.


        Stefan

bug#7496: 23.2; copy recursive keymap cause crash

[Drew Adams]

> It's easy to reproduce, though: Just eval this form and Emacs will
> segfault (if it's older than yesterday's):
> 
> (let ((map (make-sparse-keymap)))
>   (define-key map " " map)
>   (copy-keymap map))

FWIW, on MS Windows, Emacs 26.3, emacs -Q hangs if I
do that, after showing this message in the echo area:

  Re-entering top level after C stack overflow.

Have to kill the process with the Task Manager.

bug#7496: 23.2; copy recursive keymap cause crash

[Eli Zaretskii]

> Date: Sun, 13 Oct 2019 21:38:06 +0000 (UTC)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: ari@mbf.ocn.ne.jp, monnier@iro.umontreal.ca, 7496@debbugs.gnu.org
> 
> > It's easy to reproduce, though: Just eval this form and Emacs will
> > segfault (if it's older than yesterday's):
> > 
> > (let ((map (make-sparse-keymap)))
> >   (define-key map " " map)
> >   (copy-keymap map))
> 
> FWIW, on MS Windows, Emacs 26.3, emacs -Q hangs if I
> do that, after showing this message in the echo area:
> 
>   Re-entering top level after C stack overflow.
> 
> Have to kill the process with the Task Manager.

Thanks for testing.  So the stack overflow protection does kick in.