GNU bug report logs - #75593
31.0.50; Faulty macro kills Emacs

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahprae <at> protonmail.com, mattiase <at> acm.org, monnier <at> iro.umontreal.ca, 75593 <at> debbugs.gnu.org
Subject: bug#75593: 31.0.50; Faulty macro kills Emacs
Date: Thu, 16 Jan 2025 10:38:21 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Wed, 15 Jan 2025 22:20:42 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, Alexander Prähauser <ahprae <at> protonmail.com>, 75593 <at> debbugs.gnu.org
>>
>> --- a/src/eval.c
>> +++ b/src/eval.c
>> @@ -2446,8 +2446,11 @@ DEFUN ("eval", Feval, Seval, 1, 2, 0,
>>    (Lisp_Object form, Lisp_Object lexical)
>>  {
>>    specpdl_ref count = SPECPDL_INDEX ();
>> -  specbind (Qinternal_interpreter_environment,
>> -	    CONSP (lexical) || NILP (lexical) ? lexical : list_of_t);
>> +  if (CONSP (lexical) || NILP (lexical))
>> +    Flength (lexical);
>> +  else
>> +    lexical = list_of_t;
>> +  specbind (Qinternal_interpreter_environment, lexical);
>>    return unbind_to (count, eval_sub (form));
>>  }
>
> Isn't there a cheaper way of verifying LEXICAL is a list, not a cons
> cell?

I don't think there is, no.  As I explained, it's unlikely to be a
significant expense: lexical environments are not very large, usually,
and all other users of Vinternal_interpreter_environment are optimized
for small lists which are scanned until we find the right entry.  If we
wanted to optimize for large lexical environments, we might be able to,
but we've decided not to do that.

> How about calling Flength only "#ifdef ENABLE_CHECKING", and otherwise
> just verifying that LEXICAL is a list?

ENABLE_CHECKING shouldn't affect code behavior in such ways, no.  I
don't think a crash is acceptable here, but if people end up disabling
ENABLE_CHECKING and using XCAR on a non-cons, a crash is the *best*
possible outcome: more likely, we'd just be creating nonsensical
Lisp_Objects which may cause memory corruption.

Making behavior depend on ENABLE_CHECKING in such ways will cause bugs
in !ENABLE_CHECKING builds that cannot be debugged because the
corresponding ENABLE_CHECKING build will never reach the problematic
code.  IOW, it'd defeat the purpose of ENABLE_CHECKING.

> After all, passing a circular
> list to this function means some Lisp program shoots itself in the
> foot, so we could tell "don't do that", like in other cases when Emacs
> gives Lisp programmers enough rope to hang themselves.

Flength does precisely that.  If you think a crash, unquittable infloop,
or memory corruption is the right way to tell people "don't do that", I
disagree completely.  It's not what the rest of Emacs does, it's not the
right thing to do, and a performance concern that is entirely
hypothetical at this point is not a good reason to start doing it.

(Also, this is not primarily about circular lists, it's about non-lists
in general).

> IOW, can we prevent crashes in a way that is cheap in production
> builds, since 'eval' is quite a hotspot?

If there's any evidence that lexenvs are large enough that scanning them
linearly in this one place is a significant expense, we might want to
rewrite the entire lexenv code: all of it is O(N) in the size of the
environment: it assumes lexenvs are small enough to scan linearly, and
so does the fixed Feval.

(I think it would be possible to allow a hash table as the non-list
final CDR of Feval's lexenv argument, but we'd still end up overriding
bindings with an alist, so it'd be a bit of a Frankenstein affair).

Pip

Previous Next

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