bug#62867: segfault in describe_vector

bug#62867: segfault in describe_vector

[Xinyang Chen]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

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)

[-- Attachment #2: Type: text/html, Size: 551 bytes --]

bug#62867: segfault in describe_vector

[Eli Zaretskii]

> From: Xinyang Chen <chenxy@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);
 	}
     }

bug#62867: segfault in describe_vector

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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);
>  	}
>      }

bug#62867: segfault in describe_vector

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Xinyang Chen <chenxy@mit.edu>,  62867@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.