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

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75784 <at> debbugs.gnu.org
Subject: 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

Previous Next

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