GNU bug report logs - #75784
Typo "unreadeable" in print.c; was exposed to Lisp

Previous Next

To reply to this bug, email your comments to 75784 AT debbugs.gnu.org.

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

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

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Thu, 23 Jan 2025 12:21:32 +0000

print.c contains the following code:

static void
print_create_variable_mapping (void)
{
  Lisp_Object total[] = {
    list3 (intern ("length"), intern ("print-length"), Qnil),
    list3 (intern ("level"), intern ("print-level"), Qnil),
    list3 (intern ("circle"), intern ("print-circle"), Qnil),
    list3 (intern ("quoted"), intern ("print-quoted"), Qt),
    list3 (intern ("escape-newlines"), intern ("print-escape-newlines"), Qnil),
    list3 (intern ("escape-control-characters"),
	   intern ("print-escape-control-characters"), Qnil),
    list3 (intern ("escape-nonascii"), intern ("print-escape-nonascii"), Qnil),
    list3 (intern ("escape-multibyte"),
	   intern ("print-escape-multibyte"), Qnil),
    list3 (intern ("charset-text-property"),
	   intern ("print-charset-text-property"), Qnil),
    list3 (intern ("unreadeable-function"),
	   intern ("print-unreadable-function"), Qnil),
    list3 (intern ("gensym"), intern ("print-gensym"), Qnil),
    list3 (intern ("continuous-numbering"),
	   intern ("print-continuous-numbering"), Qnil),
    list3 (intern ("number-table"), intern ("print-number-table"), Qnil),
    list3 (intern ("float-format"), intern ("float-output-format"), Qnil),
    list3 (intern ("integers-as-characters"),
	   intern ("print-integers-as-characters"), Qnil),
  };

  Vprint_variable_mapping = CALLMANY (Flist, total);
}

Unfortunately, "unreadable" is spelled in two different ways: the first
instance is "unreadeable-function", with an "e", the second is
"print-unreadable-function", without the "e".

I think this was a simple typo at one point, but it was exposed as part
of the Lisp API and people may have used it.  OTOH, it wasn't introduced
too long ago, so we might want to fix it and see whether anyone
complains.

The documentation also needs to be updated, either to change it to the
correct spelling or to mention the typo so Lisp hackers know this is
another kdb-macro-redisplay.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>, 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 06:47:40 -0600

Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs <at> gnu.org> writes:

> print.c contains the following code:
>
> static void
> print_create_variable_mapping (void)
> {
>   Lisp_Object total[] = {
>     list3 (intern ("length"), intern ("print-length"), Qnil),
>     list3 (intern ("level"), intern ("print-level"), Qnil),
>     list3 (intern ("circle"), intern ("print-circle"), Qnil),
>     list3 (intern ("quoted"), intern ("print-quoted"), Qt),
>     list3 (intern ("escape-newlines"), intern ("print-escape-newlines"), Qnil),
>     list3 (intern ("escape-control-characters"),
> 	   intern ("print-escape-control-characters"), Qnil),
>     list3 (intern ("escape-nonascii"), intern ("print-escape-nonascii"), Qnil),
>     list3 (intern ("escape-multibyte"),
> 	   intern ("print-escape-multibyte"), Qnil),
>     list3 (intern ("charset-text-property"),
> 	   intern ("print-charset-text-property"), Qnil),
>     list3 (intern ("unreadeable-function"),
> 	   intern ("print-unreadable-function"), Qnil),
>     list3 (intern ("gensym"), intern ("print-gensym"), Qnil),
>     list3 (intern ("continuous-numbering"),
> 	   intern ("print-continuous-numbering"), Qnil),
>     list3 (intern ("number-table"), intern ("print-number-table"), Qnil),
>     list3 (intern ("float-format"), intern ("float-output-format"), Qnil),
>     list3 (intern ("integers-as-characters"),
> 	   intern ("print-integers-as-characters"), Qnil),
>   };
>
>   Vprint_variable_mapping = CALLMANY (Flist, total);
> }
>
> Unfortunately, "unreadable" is spelled in two different ways: the first
> instance is "unreadeable-function", with an "e", the second is
> "print-unreadable-function", without the "e".
>
> I think this was a simple typo at one point, but it was exposed as part
> of the Lisp API and people may have used it.  OTOH, it wasn't introduced
> too long ago, so we might want to fix it and see whether anyone
> complains.
>
> The documentation also needs to be updated, either to change it to the
> correct spelling or to mention the typo so Lisp hackers know this is
> another kdb-macro-redisplay.

Thanks for catching this typo.

I think we will have to support both spellings, at least for a while.
Do we have a way to add an obsoletion warning to both names?

Would you like to suggest a patch?

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 13:50:20 +0000

"Stefan Kangas" <stefankangas <at> gmail.com> writes:

> Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> print.c contains the following code:
>>
>> static void
>> print_create_variable_mapping (void)
>> {
>>   Lisp_Object total[] = {
>>     list3 (intern ("length"), intern ("print-length"), Qnil),
>>     list3 (intern ("level"), intern ("print-level"), Qnil),
>>     list3 (intern ("circle"), intern ("print-circle"), Qnil),
>>     list3 (intern ("quoted"), intern ("print-quoted"), Qt),
>>     list3 (intern ("escape-newlines"), intern ("print-escape-newlines"), Qnil),
>>     list3 (intern ("escape-control-characters"),
>> 	   intern ("print-escape-control-characters"), Qnil),
>>     list3 (intern ("escape-nonascii"), intern ("print-escape-nonascii"), Qnil),
>>     list3 (intern ("escape-multibyte"),
>> 	   intern ("print-escape-multibyte"), Qnil),
>>     list3 (intern ("charset-text-property"),
>> 	   intern ("print-charset-text-property"), Qnil),
>>     list3 (intern ("unreadeable-function"),
>> 	   intern ("print-unreadable-function"), Qnil),
>>     list3 (intern ("gensym"), intern ("print-gensym"), Qnil),
>>     list3 (intern ("continuous-numbering"),
>> 	   intern ("print-continuous-numbering"), Qnil),
>>     list3 (intern ("number-table"), intern ("print-number-table"), Qnil),
>>     list3 (intern ("float-format"), intern ("float-output-format"), Qnil),
>>     list3 (intern ("integers-as-characters"),
>> 	   intern ("print-integers-as-characters"), Qnil),
>>   };
>>
>>   Vprint_variable_mapping = CALLMANY (Flist, total);
>> }
>>
>> Unfortunately, "unreadable" is spelled in two different ways: the first
>> instance is "unreadeable-function", with an "e", the second is
>> "print-unreadable-function", without the "e".
>>
>> I think this was a simple typo at one point, but it was exposed as part
>> of the Lisp API and people may have used it.  OTOH, it wasn't introduced
>> too long ago, so we might want to fix it and see whether anyone
>> complains.
>>
>> The documentation also needs to be updated, either to change it to the
>> correct spelling or to mention the typo so Lisp hackers know this is
>> another kdb-macro-redisplay.
>
> Thanks for catching this typo.

Thanks for suggesting I report it.

> I think we will have to support both spellings, at least for a while.

Okay.

> Do we have a way to add an obsoletion warning to both names?

I'm sorry: why both names?  The new name should be permanent, unless we
decide to move to a different name entirely to get rid of the typo (this
is what happened with kdb-macro-redisplay, IIRC).

While we could check for an obsoletion property on the symbol in
question in print_bind_overrides, it's very likely that if anyone
actually uses this code, they use it in an automated loop, so the
warning would need rate-limiting as well.  My suggestion is to print the
warning just once, increasing the risk it'll be missed but saving us
having to write a rate-limiting facility for Emacs which, if I
understand the context directly, cannot use Lisp.

> Would you like to suggest a patch?

diff --git a/src/print.c b/src/print.c
index 8c10ffa883e..918cef107b7 100644
--- a/src/print.c
+++ b/src/print.c
@@ -677,6 +677,8 @@ print_create_variable_mapping (void)
 	   intern ("print-escape-multibyte"), Qnil),
     list3 (intern ("charset-text-property"),
 	   intern ("print-charset-text-property"), Qnil),
+    list3 (intern ("unreadable-function"),
+	   intern ("print-unreadable-function"), Qnil),
     list3 (intern ("unreadeable-function"),
 	   intern ("print-unreadable-function"), Qnil),
     list3 (intern ("gensym"), intern ("print-gensym"), Qnil),

should allow using the correct spelling, but not add the obsoletion
warning.

I do not know how to make a symbol that is unbound but used to indicate
a keyword (I don't understand why no ":" is present, or how this would
change the situation) obsolete.

How would you like to proceed?

As is, unfortunately, normal for loops that attempt to avoid
FOR_EACH_TAIL, print_bind_overrides misses a corner case and infloops
unquittably for:

(prin1 "foo" nil (quote #1=(t . #1#)))

Maybe we should build on the useful parts of this first attempt and
review it carefully for a second version.

Pip

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 14:15:06 +0000

Pip Cet <pipcet <at> protonmail.com> writes:

> "Stefan Kangas" <stefankangas <at> gmail.com> writes:
>
> I'm sorry: why both names?  The new name should be permanent, unless we
> decide to move to a different name entirely to get rid of the typo (this
> is what happened with kdb-macro-redisplay, IIRC).
>
>> Would you like to suggest a patch?
>
> diff --git a/src/print.c b/src/print.c

Here's a better patch.  While this goes overboard and fixes the case
that Vprint_variable_mapping has become invalid, I'm not sure this can
happen: Fassq can quit, but the Lisp backtrace won't expose its
arguments to the debugger, it seems.  I'd like to err on the side of
caution and double-check that this mapping is still valid in case
someone somehow exposes it to Lisp.

Note that specbind currently insists on a BARE_SYMBOL.  We'll have to
check all callers to make sure they never pass a symbol-with-position!

Tested only very briefly.

From 879714122b1de7852be898a7940ed85aefde6e2e Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH] Fix typo (and bugs) in prin1's overrides code (bug#75754)

Vprint_variable_mapping may have been exposed to Lisp via the
backtrace if there was a previous quit in the Fassq call, requiring us
to check it still has the intended format.

* src/print.c (print_create_variable_mapping): Add correct spelling
"unreadable-function" as an alternative to "unreadeable-function".
(print_bind_overrides): Preserve original argument for error case. Use
FOR_EACH_TAIL, fixing infloop.  Warn about obsolete symbol.  Guard
against invalid Vprint_variable_mapping.
---
 src/print.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/src/print.c b/src/print.c
index 43698f309b1..0da2baa6e09 100644
--- a/src/print.c
+++ b/src/print.c
@@ -676,6 +676,8 @@ print_create_variable_mapping (void)
 	   intern ("print-escape-multibyte"), Qnil),
     list3 (intern ("charset-text-property"),
 	   intern ("print-charset-text-property"), Qnil),
+    list3 (intern ("unreadable-function"),
+	   intern ("print-unreadable-function"), Qnil),
     list3 (intern ("unreadeable-function"),
 	   intern ("print-unreadable-function"), Qnil),
     list3 (intern ("gensym"), intern ("print-gensym"), Qnil),
@@ -698,11 +700,10 @@ print_bind_overrides (Lisp_Object overrides)
 
   if (EQ (overrides, Qt))
     print_bind_all_defaults ();
-  else if (!CONSP (overrides))
-    xsignal (Qwrong_type_argument, Qconsp);
   else
     {
-      while (!NILP (overrides))
+      Lisp_Object original_overrides = overrides;
+      FOR_EACH_TAIL (overrides)
 	{
 	  Lisp_Object setting = XCAR (overrides);
 	  if (EQ (setting, Qt))
@@ -714,15 +715,23 @@ print_bind_overrides (Lisp_Object overrides)
 	      Lisp_Object key = XCAR (setting),
 		value = XCDR (setting);
 	      Lisp_Object map = Fassq (key, Vprint_variable_mapping);
+	      static bool warned;
+	      if (!warned && SYMBOLP (key) &&
+		  strcmp (SSDATA (SYMBOL_NAME (key)),
+			  "unreadeable-function") == 0)
+		{
+		  warned = true;
+		  add_to_log ("Obsolete symbol `unreadeable-function' used.  Use `unreadable-function' instead.");
+		}
 	      if (NILP (map))
 		xsignal2 (Qwrong_type_argument, Qsymbolp, map);
-	      specbind (XCAR (XCDR (map)), value);
+	      Lisp_Object bare_symbol =
+		maybe_remove_pos_from_symbol (Fcar (XCDR (map)));
+	      CHECK_SYMBOL (bare_symbol);
+	      specbind (bare_symbol, value);
 	    }
-
-	  if (!NILP (XCDR (overrides)) && !CONSP (XCDR (overrides)))
-	    xsignal (Qwrong_type_argument, Qconsp);
-	  overrides = XCDR (overrides);
 	}
+      CHECK_LIST_END (overrides, original_overrides);
     }
 }
 
-- 
2.47.1

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 14:27:04 +0000

"Pip Cet" <pipcet <at> protonmail.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> "Stefan Kangas" <stefankangas <at> gmail.com> writes:
>>
>> I'm sorry: why both names?  The new name should be permanent, unless we
>> decide to move to a different name entirely to get rid of the typo (this
>> is what happened with kdb-macro-redisplay, IIRC).
>>
>>> Would you like to suggest a patch?
>>
>> diff --git a/src/print.c b/src/print.c
>
> Here's a better patch.  While this goes overboard and fixes the case
> that Vprint_variable_mapping has become invalid, I'm not sure this can
> happen: Fassq can quit, but the Lisp backtrace won't expose its
> arguments to the debugger, it seems.  I'd like to err on the side of
> caution and double-check that this mapping is still valid in case
> someone somehow exposes it to Lisp.
>
> Note that specbind currently insists on a BARE_SYMBOL.  We'll have to
> check all callers to make sure they never pass a symbol-with-position!

Now done.  All but one call safe.

Proposed patch to fix what is very likely a nasty crashable bug in the
byte compiler:

From 9c70bd663a2045db92ed01ffedca23748f492d50 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH] DO NOT COMMIT: Fix likely (but unproven) specbind bug in
 bytecode runner (bug#75784)

* src/bytecode.c (exec_byte_code): Ensure position is removed from
symbol taken from constant vector.  Check it is a symbol if
BYTE_CODE_SAFE.
---
 src/bytecode.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/bytecode.c b/src/bytecode.c
index d62d7d067b1..7516431cb96 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -734,8 +734,15 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
 	CASE (Bvarbind5):
 	  op -= Bvarbind;
 	varbind:
-	  /* Specbind can signal and thus GC.  */
-	  specbind (vectorp[op], POP);
+	  {
+	    Lisp_Object symbol = vectorp[op];
+	    symbol = maybe_remove_pos_from_symbol (symbol);
+#if BYTE_CODE_SAFE
+	    CHECK_SYMBOL (symbol);
+#endif
+	    /* Specbind can signal and thus GC.  */
+	    specbind (symbol, POP);
+	  }
 	  NEXT;
 
 	CASE (Bcall6):
-- 
2.47.1

Marked as DO NOT COMMIT because this needs more thought.  Might have to
be applied with -n as this makes the first line "too long".

Will confirm and file separate bug report if crashable.

Pip

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 10:52:10 -0600

Pip Cet <pipcet <at> protonmail.com> writes:

> "Stefan Kangas" <stefankangas <at> gmail.com> writes:
>
>> Do we have a way to add an obsoletion warning to both names?
>
> I'm sorry: why both names?  The new name should be permanent, unless we
> decide to move to a different name entirely to get rid of the typo (this
> is what happened with kdb-macro-redisplay, IIRC).

Sorry, that was a typo.  I meant the old name only.

> While we could check for an obsoletion property on the symbol in
> question in print_bind_overrides, it's very likely that if anyone
> actually uses this code, they use it in an automated loop, so the
> warning would need rate-limiting as well.  My suggestion is to print the
> warning just once, increasing the risk it'll be missed but saving us
> having to write a rate-limiting facility for Emacs which, if I
> understand the context directly, cannot use Lisp.

Printing it once is what we usually do in similar circumstances, sounds
good.

Pip Cet <pipcet <at> protonmail.com> writes:

> Here's a better patch.  While this goes overboard and fixes the case
> that Vprint_variable_mapping has become invalid, I'm not sure this can
> happen: Fassq can quit, but the Lisp backtrace won't expose its
> arguments to the debugger, it seems.  I'd like to err on the side of
> caution and double-check that this mapping is still valid in case
> someone somehow exposes it to Lisp.
>
> Note that specbind currently insists on a BARE_SYMBOL.  We'll have to
> check all callers to make sure they never pass a symbol-with-position!

I guess we could also patch specbind.  Not very pretty, overall.

> Tested only very briefly.
>
> From 879714122b1de7852be898a7940ed85aefde6e2e Mon Sep 17 00:00:00 2001
> From: Pip Cet <pipcet <at> protonmail.com>
> Subject: [PATCH] Fix typo (and bugs) in prin1's overrides code (bug#75754)
>
> Vprint_variable_mapping may have been exposed to Lisp via the
> backtrace if there was a previous quit in the Fassq call, requiring us
> to check it still has the intended format.
>
> * src/print.c (print_create_variable_mapping): Add correct spelling
> "unreadable-function" as an alternative to "unreadeable-function".
> (print_bind_overrides): Preserve original argument for error case. Use
> FOR_EACH_TAIL, fixing infloop.  Warn about obsolete symbol.  Guard
> against invalid Vprint_variable_mapping.
> ---
>  src/print.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/src/print.c b/src/print.c
> index 43698f309b1..0da2baa6e09 100644
> --- a/src/print.c
> +++ b/src/print.c
> @@ -676,6 +676,8 @@ print_create_variable_mapping (void)
>  	   intern ("print-escape-multibyte"), Qnil),
>      list3 (intern ("charset-text-property"),
>  	   intern ("print-charset-text-property"), Qnil),
> +    list3 (intern ("unreadable-function"),
> +	   intern ("print-unreadable-function"), Qnil),
>      list3 (intern ("unreadeable-function"),
>  	   intern ("print-unreadable-function"), Qnil),

Please add a comment saying why we are keeping the typo, and that
it should be deleted at some point.

>      list3 (intern ("gensym"), intern ("print-gensym"), Qnil),
> @@ -698,11 +700,10 @@ print_bind_overrides (Lisp_Object overrides)
>
>    if (EQ (overrides, Qt))
>      print_bind_all_defaults ();
> -  else if (!CONSP (overrides))
> -    xsignal (Qwrong_type_argument, Qconsp);

FOR_EACH_TAIL won't raise a signal, AFAICT.  Do we really want to stop
signaling for the wrong argument?  And similarly below.

>    else
>      {
> -      while (!NILP (overrides))
> +      Lisp_Object original_overrides = overrides;
> +      FOR_EACH_TAIL (overrides)
>  	{
>  	  Lisp_Object setting = XCAR (overrides);
>  	  if (EQ (setting, Qt))
> @@ -714,15 +715,23 @@ print_bind_overrides (Lisp_Object overrides)
>  	      Lisp_Object key = XCAR (setting),
>  		value = XCDR (setting);
>  	      Lisp_Object map = Fassq (key, Vprint_variable_mapping);
> +	      static bool warned;
> +	      if (!warned && SYMBOLP (key) &&
> +		  strcmp (SSDATA (SYMBOL_NAME (key)),
> +			  "unreadeable-function") == 0)
> +		{
> +		  warned = true;
> +		  add_to_log ("Obsolete symbol `unreadeable-function' used.  Use `unreadable-function' instead.");
> +		}

Nice. I don't think we want to end the message with a period though,
according to our conventions, do we?

I'd suggest:

		  add_to_log ("\
Obsolete symbol `unreadeable-function'; use `unreadable-function' instead");

Using "\ trades one kind of ugly for another, so it's up to you which
you find less grating.  I have no real opinion.

>  	      if (NILP (map))
>  		xsignal2 (Qwrong_type_argument, Qsymbolp, map);
> -	      specbind (XCAR (XCDR (map)), value);
> +	      Lisp_Object bare_symbol =
> +		maybe_remove_pos_from_symbol (Fcar (XCDR (map)));
> +	      CHECK_SYMBOL (bare_symbol);
> +	      specbind (bare_symbol, value);
>  	    }
> -
> -	  if (!NILP (XCDR (overrides)) && !CONSP (XCDR (overrides)))
> -	    xsignal (Qwrong_type_argument, Qconsp);
> -	  overrides = XCDR (overrides);
>  	}
> +      CHECK_LIST_END (overrides, original_overrides);
>      }
>  }
>
> --
> 2.47.1

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 17:37:24 +0000

"Stefan Kangas" <stefankangas <at> gmail.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> "Stefan Kangas" <stefankangas <at> gmail.com> writes:
>>
>> Here's a better patch.  While this goes overboard and fixes the case
>> that Vprint_variable_mapping has become invalid, I'm not sure this can
>> happen: Fassq can quit, but the Lisp backtrace won't expose its
>> arguments to the debugger, it seems.  I'd like to err on the side of
>> caution and double-check that this mapping is still valid in case
>> someone somehow exposes it to Lisp.
>>
>> Note that specbind currently insists on a BARE_SYMBOL.  We'll have to
>> check all callers to make sure they never pass a symbol-with-position!
>
> I guess we could also patch specbind.  Not very pretty, overall.

I tried crashing this, but haven't been successful so far: I don't see
how byte-compile sanitizes its constants so they're no longer
symbols-with-positions, but it appears to do so.

(Maybe by accident: whenever we reach code touched by certain
contributors, all symbols with positions are normalized :-) )

>> Tested only very briefly.
>>
>> From 879714122b1de7852be898a7940ed85aefde6e2e Mon Sep 17 00:00:00 2001
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Subject: [PATCH] Fix typo (and bugs) in prin1's overrides code (bug#75754)
>>
>> Vprint_variable_mapping may have been exposed to Lisp via the
>> backtrace if there was a previous quit in the Fassq call, requiring us
>> to check it still has the intended format.
>>
>> * src/print.c (print_create_variable_mapping): Add correct spelling
>> "unreadable-function" as an alternative to "unreadeable-function".
>> (print_bind_overrides): Preserve original argument for error case. Use
>> FOR_EACH_TAIL, fixing infloop.  Warn about obsolete symbol.  Guard
>> against invalid Vprint_variable_mapping.
>> ---
>>  src/print.c | 25 +++++++++++++++++--------
>>  1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/print.c b/src/print.c
>> index 43698f309b1..0da2baa6e09 100644
>> --- a/src/print.c
>> +++ b/src/print.c
>> @@ -676,6 +676,8 @@ print_create_variable_mapping (void)
>>  	   intern ("print-escape-multibyte"), Qnil),
>>      list3 (intern ("charset-text-property"),
>>  	   intern ("print-charset-text-property"), Qnil),
>> +    list3 (intern ("unreadable-function"),
>> +	   intern ("print-unreadable-function"), Qnil),
>>      list3 (intern ("unreadeable-function"),
>>  	   intern ("print-unreadable-function"), Qnil),
>
> Please add a comment saying why we are keeping the typo, and that
> it should be deleted at some point.

Sorry. I meant to mention that I ran out of time for adding comments,
then I ran out of time for mentioning I'd run out of time.

>>      list3 (intern ("gensym"), intern ("print-gensym"), Qnil),
>> @@ -698,11 +700,10 @@ print_bind_overrides (Lisp_Object overrides)
>>
>>    if (EQ (overrides, Qt))
>>      print_bind_all_defaults ();
>> -  else if (!CONSP (overrides))
>> -    xsignal (Qwrong_type_argument, Qconsp);
>
> FOR_EACH_TAIL won't raise a signal, AFAICT.  Do we really want to stop

FOR_EACH_TAIL signals for a circular list, leaves its argument a cons
for a real list, and leaves its argument as a non-cons for a dotted
list.

I hope.

While there are things that are subject to change about its behavior,
this much we can rely on.  We shouldn't rely on circular_list being
called with the beginning of a cycle, or as soon as possible; it'll get
called eventually.

> signaling for the wrong argument?  And similarly below.
>
>>    else
>>      {
>> -      while (!NILP (overrides))
>> +      Lisp_Object original_overrides = overrides;
>> +      FOR_EACH_TAIL (overrides)
>>  	{
>>  	  Lisp_Object setting = XCAR (overrides);
>>  	  if (EQ (setting, Qt))
>> @@ -714,15 +715,23 @@ print_bind_overrides (Lisp_Object overrides)
>>  	      Lisp_Object key = XCAR (setting),
>>  		value = XCDR (setting);
>>  	      Lisp_Object map = Fassq (key, Vprint_variable_mapping);
>> +	      static bool warned;
>> +	      if (!warned && SYMBOLP (key) &&
>> +		  strcmp (SSDATA (SYMBOL_NAME (key)),
>> +			  "unreadeable-function") == 0)
>> +		{
>> +		  warned = true;
>> +		  add_to_log ("Obsolete symbol `unreadeable-function' used.  Use `unreadable-function' instead.");
>> +		}

BTW: add-to-log here, or message?  While my reasons for using the former
were that I was confused, I think this is most likely going to happen in
a noninteractive program, where messages might be used to generate its
output.

> Nice. I don't think we want to end the message with a period though,
> according to our conventions, do we?
>
> I'd suggest:
>
> 		  add_to_log ("\
> Obsolete symbol `unreadeable-function'; use `unreadable-function' instead");
>
> Using "\ trades one kind of ugly for another, so it's up to you which
> you find less grating.  I have no real opinion.

Thanks! I agree completely, and thank you for watching out for details
like this; details are important.

>> +      CHECK_LIST_END (overrides, original_overrides);

This should signal if 'overrides' isn't a proper, finite, non-dotted list.

(Lisp doesn't have "infinite" (non-circular) lists, but I tend to use
that term for a list that is modified from a Lisp callback while we walk
it, and keeps adding new cons cells so we never see the end of it.

I've yet to find a way this causes a bug, but it's a scenario that would
give me great personal satisfaction to fix it.  However, it'd be a
strange bug, because calling Lisp means we should have a way to quit.)

Pip

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 15:59:32 -0600

Pip Cet <pipcet <at> protonmail.com> writes:

> Sorry. I meant to mention that I ran out of time for adding comments,
> then I ran out of time for mentioning I'd run out of time.

:-)

>>> +		  add_to_log ("Obsolete symbol `unreadeable-function' used.  Use `unreadable-function' instead.");
>
> BTW: add-to-log here, or message?  While my reasons for using the former
> were that I was confused, I think this is most likely going to happen in
> a noninteractive program, where messages might be used to generate its
> output.

While add_to_log is probably enough to notify users, maybe message is
indeed better because it's harder to miss.  So I'd probably lean towards
the latter, but I have no strong opinion.

>>> +      CHECK_LIST_END (overrides, original_overrides);
>
> This should signal if 'overrides' isn't a proper, finite, non-dotted list.
>
> (Lisp doesn't have "infinite" (non-circular) lists, but I tend to use
> that term for a list that is modified from a Lisp callback while we walk
> it, and keeps adding new cons cells so we never see the end of it.
>
> I've yet to find a way this causes a bug, but it's a scenario that would
> give me great personal satisfaction to fix it.  However, it'd be a
> strange bug, because calling Lisp means we should have a way to quit.)

No objection to fixing this, assuming that we can do it cleanly.

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Fri, 24 Jan 2025 23:38:55 +0000

"Stefan Kangas" <stefankangas <at> gmail.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> Sorry. I meant to mention that I ran out of time for adding comments,
>> then I ran out of time for mentioning I'd run out of time.
>
> :-)
>
>>>> +		  add_to_log ("Obsolete symbol `unreadeable-function' used.  Use `unreadable-function' instead.");
>>
>> BTW: add-to-log here, or message?  While my reasons for using the former
>> were that I was confused, I think this is most likely going to happen in
>> a noninteractive program, where messages might be used to generate its
>> output.
>
> While add_to_log is probably enough to notify users, maybe message is
> indeed better because it's harder to miss.  So I'd probably lean towards
> the latter, but I have no strong opinion.
>
>>>> +      CHECK_LIST_END (overrides, original_overrides);
>>
>> This should signal if 'overrides' isn't a proper, finite, non-dotted list.
>>
>> (Lisp doesn't have "infinite" (non-circular) lists, but I tend to use
>> that term for a list that is modified from a Lisp callback while we walk
>> it, and keeps adding new cons cells so we never see the end of it.
>>
>> I've yet to find a way this causes a bug, but it's a scenario that would
>> give me great personal satisfaction to fix it.  However, it'd be a
>> strange bug, because calling Lisp means we should have a way to quit.)
>
> No objection to fixing this, assuming that we can do it cleanly.

Oh, sorry, I wasn't clear: usually, growing a list from Lisp while it's
being scanned doesn't cause bugs: an infloop, maybe, but a quittable one
because it's Lisp.  A bug would be something like C code checking a
list's length, then calling a predicate (such as a sort predicate) which
then extends the list, and assuming that the list length was limited so
overrunning its buffer.

Found one!  Well, two:

(let ((l (make-list 1000 nil))) (mapconcat (lambda (x) (ntake 1 l)) l))

(let ((s (string ?a 1000))) (mapconcat (lambda (x) (aset s 0 1000) (string x)) s))

My first impulse is to change as little as possible to prevent crashes.
A trivial mapconcat can handle many cases in which our mapconcat fails
or produces unexpected results.  That's okay.  Don't modify the sequence
and you'll be fine.

I still have to check that the old, more forgiving behavior for lists
isn't relied upon anywhere.

From 1eb35efa41da5dd00f1c824c6b396c89bf4f0ca8 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH] Fix crashes in mapconcat etc. (bug#TBD)

This doesn't attempt to catch all cases in which the mapping function
modifies the sequence, only those which would cause crashes.

* src/fns.c (mapcar1): Error if we detect seq to have been modified by
fn.
---
 src/fns.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 081ed2b9f51..ce09e8c086c 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -3385,7 +3385,7 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
       for (ptrdiff_t i = 0; i < leni; i++)
 	{
 	  if (! CONSP (tail))
-	    return i;
+	    error ("list modified by mapping function");
 	  Lisp_Object dummy = calln (fn, XCAR (tail));
 	  if (vals)
 	    vals[i] = dummy;
@@ -3403,11 +3403,14 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
     }
   else if (STRINGP (seq))
     {
+      ptrdiff_t i = 0;
       ptrdiff_t i_byte = 0;
 
-      for (ptrdiff_t i = 0; i < leni;)
+      while (i_byte < SBYTES (seq))
 	{
 	  ptrdiff_t i_before = i;
+	  if (! CHAR_HEAD_P (SREF (seq, i_byte)))
+	    error ("string modified by mapping function");
 	  int c = fetch_string_char_advance (seq, &i, &i_byte);
 	  Lisp_Object dummy = calln (fn, make_fixnum (c));
 	  if (vals)
-- 
2.47.1

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp
Date: Sat, 25 Jan 2025 10:51:01 +0000

Pip Cet <pipcet <at> protonmail.com> writes:

> From 1eb35efa41da5dd00f1c824c6b396c89bf4f0ca8 Mon Sep 17 00:00:00 2001
> From: Pip Cet <pipcet <at> protonmail.com>
> Subject: [PATCH] Fix crashes in mapconcat etc. (bug#TBD)
>
> This doesn't attempt to catch all cases in which the mapping function
> modifies the sequence, only those which would cause crashes.
>
> * src/fns.c (mapcar1): Error if we detect seq to have been modified by
> fn.

Additional change required:

diff --git a/src/fns.c b/src/fns.c
index ce09e8c086c..8bfb0cc44d3 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -3416,6 +3416,8 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
 	  if (vals)
 	    vals[i_before] = dummy;
 	}
+      if (i < leni)
+	error ("string modified by mapping function");
     }
   else
     {

This change ensures that vals aren't used uninitialized and bogus
pointers exposed to Lisp.  Usually the memory will be zeroed, I think,
but not always.

Additional change suggested:

@@ -3476,8 +3478,7 @@ DEFUN ("mapconcat", Fmapconcat, Smapconcat, 2, 3, 0,
 	  goto concat;
 	}
     }
-  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
-  eassert (nmapped == leni);
+  leni = mapcar1 (leni, args, function, sequence);
 
  concat: ;
   ptrdiff_t nargs = args_alloc;

The second hunk isn't necessary because mapcar1 will signal an error
rather than returning a smaller value if the list has been truncated,
and will return leni (and leave vals invalid).

This is a special case where removing an eassert is the right thing to
do, IMHO.

If any further complications arise, I'll report a new bug, but so far it
still looks like a reasonably obvious pushable fix to me.  Comment needs
expanding to list the crashable code, and it needs to become an Emacs
crash test when we have those (there's no time pressure there).

Pip

Previous Next

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