GNU bug report logs - #62867
segfault in describe_vector

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 62867 in the body.
You can then email your comments to 62867 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 <chenxy <at> mit.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: segfault in describe_vector
Date: Sat, 15 Apr 2023 08:07:07 -0400

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

to reproduce:

(package-initialize)
(require 'evil)
(setq testmap (make-sparse-keymap))
(define-key testmap (kbd "SPC") #'test)
(evil-define-key 'motion global-map (kbd "SPC") testmap)
(evil-mode)
(describe-keymap global-map)

This appears to be caused by keymap.c line 3313
SYMBOL_NAME (shadowed_by)
where shadowed_by don't have to be a symbol (In this case its a keymap)

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Xinyang Chen <chenxy <at> mit.edu>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 62867 <at> debbugs.gnu.org
Subject: Re: bug#62867: segfault in describe_vector
Date: Sat, 15 Apr 2023 22:33:03 +0300

> From: Xinyang Chen <chenxy <at> mit.edu>
> Date: Sat, 15 Apr 2023 08:07:07 -0400
> 
> to reproduce:
> 
> (package-initialize)
> (require 'evil)
> (setq testmap (make-sparse-keymap))
> (define-key testmap (kbd "SPC") #'test)
> (evil-define-key 'motion global-map (kbd "SPC") testmap)
> (evil-mode)
> (describe-keymap global-map)
> 
> This appears to be caused by keymap.c line 3313
> SYMBOL_NAME (shadowed_by)
> where shadowed_by don't have to be a symbol (In this case its a keymap)

No good deed goes unpunished...

Stefan, can we do better than the below?

diff --git a/src/keymap.c b/src/keymap.c
index efac410..b9950b9 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -3308,13 +3308,18 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
       if (this_shadowed)
 	{
 	  SET_PT (PT - 1);
-	  static char const fmt[] = "  (currently shadowed by `%s')";
-	  USE_SAFE_ALLOCA;
-	  char *buffer = SAFE_ALLOCA (sizeof fmt +
-				      SBYTES (SYMBOL_NAME (shadowed_by)));
-	  esprintf (buffer, fmt, SDATA (SYMBOL_NAME (shadowed_by)));
-	  insert_string (buffer);
-	  SAFE_FREE();
+	  if (SYMBOLP (shadowed_by))
+	    {
+	      static char const fmt[] = "  (currently shadowed by `%s')";
+	      USE_SAFE_ALLOCA;
+	      char *buffer =
+		SAFE_ALLOCA (sizeof fmt + SBYTES (SYMBOL_NAME (shadowed_by)));
+	      esprintf (buffer, fmt, SDATA (SYMBOL_NAME (shadowed_by)));
+	      insert_string (buffer);
+	      SAFE_FREE();
+	    }
+	  else
+	    insert_string ("  (binding currently shadowed by a keymap)");
 	  SET_PT (PT + 1);
 	}
     }

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62867 <at> debbugs.gnu.org, Xinyang Chen <chenxy <at> mit.edu>
Subject: Re: bug#62867: segfault in describe_vector
Date: Sat, 15 Apr 2023 18:16:32 -0400

> Stefan, can we do better than the below?

The patch looks about right to me.

Only detail is that `shadowed_by` contains a binding, so it can be
a keymap, but it can also be a lambda expression, or a keyboard macro,
so we shouldn't say "shadowed by a keymap" but just "shadowed" (unless
we go through the extra trouble of checking if it's a keymap, or if it's
a ...).


        Stefan


> diff --git a/src/keymap.c b/src/keymap.c
> index efac410..b9950b9 100644
> --- a/src/keymap.c
> +++ b/src/keymap.c
> @@ -3308,13 +3308,18 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
>        if (this_shadowed)
>  	{
>  	  SET_PT (PT - 1);
> -	  static char const fmt[] = "  (currently shadowed by `%s')";
> -	  USE_SAFE_ALLOCA;
> -	  char *buffer = SAFE_ALLOCA (sizeof fmt +
> -				      SBYTES (SYMBOL_NAME (shadowed_by)));
> -	  esprintf (buffer, fmt, SDATA (SYMBOL_NAME (shadowed_by)));
> -	  insert_string (buffer);
> -	  SAFE_FREE();
> +	  if (SYMBOLP (shadowed_by))
> +	    {
> +	      static char const fmt[] = "  (currently shadowed by `%s')";
> +	      USE_SAFE_ALLOCA;
> +	      char *buffer =
> +		SAFE_ALLOCA (sizeof fmt + SBYTES (SYMBOL_NAME (shadowed_by)));
> +	      esprintf (buffer, fmt, SDATA (SYMBOL_NAME (shadowed_by)));
> +	      insert_string (buffer);
> +	      SAFE_FREE();
> +	    }
> +	  else
> +	    insert_string ("  (binding currently shadowed by a keymap)");
>  	  SET_PT (PT + 1);
>  	}
>      }

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 62867-done <at> debbugs.gnu.org, chenxy <at> mit.edu
Subject: Re: bug#62867: segfault in describe_vector
Date: Sun, 16 Apr 2023 08:53:05 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Xinyang Chen <chenxy <at> mit.edu>,  62867 <at> debbugs.gnu.org
> Date: Sat, 15 Apr 2023 18:16:32 -0400
> 
> > Stefan, can we do better than the below?
> 
> The patch looks about right to me.
> 
> Only detail is that `shadowed_by` contains a binding, so it can be
> a keymap, but it can also be a lambda expression, or a keyboard macro,
> so we shouldn't say "shadowed by a keymap" but just "shadowed" (unless
> we go through the extra trouble of checking if it's a keymap, or if it's
> a ...).

Thanks, installed on the emacs-29 branch with that change, and closing
the bug.

Previous Next

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