Slightly extending commit 16b0520a9

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Slightly extending commit 16b0520a9

Alex-2
Commit 16b0520a9 changed various Fcar/Fcdr calls into XCAR/XCDR, but I
believe I see two more instances where this change can also be applied.
In both cases, XCDR is already being applied on XCDR (args), so XCAR
should also be fine.

diff --git a/src/eval.c b/src/eval.c
index e5900382de..d132959f0c 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -396,7 +396,7 @@ usage: (if COND THEN ELSE...)  */)
   cond = eval_sub (XCAR (args));
 
   if (!NILP (cond))
-    return eval_sub (Fcar (XCDR (args)));
+    return eval_sub (XCAR (XCDR (args)));
   return Fprogn (XCDR (XCDR (args)));
 }
 
@@ -806,7 +806,7 @@ usage: (defconst SYMBOL INITVALUE [DOCSTRING])  */)
   if (CONSP (Fcdr (XCDR (XCDR (args)))))
     error ("Too many arguments");
 
-  tem = eval_sub (Fcar (XCDR (args)));
+  tem = eval_sub (XCAR (XCDR (args)));
   if (!NILP (Vpurify_flag))
     tem = Fpurecopy (tem);
   Fset_default (sym, tem);

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Slightly extending commit 16b0520a9

Stefan Monnier
>    cond = eval_sub (XCAR (args));
>
>    if (!NILP (cond))
> -    return eval_sub (Fcar (XCDR (args)));
> +    return eval_sub (XCAR (XCDR (args)));

I don't see anything in the preceding code that guarantees that `XCDR (args)`
holds a cons, so I think XCAR here is unsafe.

> @@ -806,7 +806,7 @@ usage: (defconst SYMBOL INITVALUE [DOCSTRING])  */)
>    if (CONSP (Fcdr (XCDR (XCDR (args)))))
>      error ("Too many arguments");
>
> -  tem = eval_sub (Fcar (XCDR (args)));
> +  tem = eval_sub (XCAR (XCDR (args)));

This one looks right, yes,


        Stefan


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Slightly extending commit 16b0520a9

Alex-2
Stefan Monnier <[hidden email]> writes:

>>    cond = eval_sub (XCAR (args));
>>
>>    if (!NILP (cond))
>> -    return eval_sub (Fcar (XCDR (args)));
>> +    return eval_sub (XCAR (XCDR (args)));
>
> I don't see anything in the preceding code that guarantees that `XCDR (args)`
> holds a cons, so I think XCAR here is unsafe.

The following line includes "XCDR (XCDR (args))", and the value of cond
should not affect the type of XCDR (args). If XCDR is allowed in this
case, then IIUC XCAR should be allowed as well.

I believe the reason why we can assume that XCDR (args) is a cons cell
is that `if' requires at least 2 (unevalled) arguments, so args must be
a list of at least length 2.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Slightly extending commit 16b0520a9

Stefan Monnier
>>> cond = eval_sub (XCAR (args));
>>> if (!NILP (cond))
>>> -    return eval_sub (Fcar (XCDR (args)));
>>> +    return eval_sub (XCAR (XCDR (args)));
>> I don't see anything in the preceding code that guarantees that `XCDR (args)`
>> holds a cons, so I think XCAR here is unsafe.
> The following line includes "XCDR (XCDR (args))",

Indeed, that looks like a bug.

> I believe the reason why we can assume that XCDR (args) is a cons cell
> is that `if' requires at least 2 (unevalled) arguments, so args must be
> a list of at least length 2.

Try (eval '(if nil . "hello"))

[ ... trying it himself ... ]

Hmm... it turns out that indeed it seems that XCDR and XCAR here are
safe because before calling those functions, eval_sub happens to call
Flength on the args, and that triggers an error if the form is not
a proper list, so `XCDR (args)` will indeed be a cons once we get
to Fif.

Arguably Fif could be called from elsewhere than eval_sub, and arguably
eval_sub's implementation could be changed in such a way that it doesn't
catch this error, so the safety of using XCDR is debatable.

The important thing to remember, tho, is that Fif should not be
performance sensitive: code whose performance matters should be
byte-compiled in which case it doesn't call Fif (as is the case for all
other special forms).


        Stefan


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Slightly extending commit 16b0520a9

Paul Eggert
Stefan Monnier wrote:
> it seems that XCDR and XCAR here are
> safe because before calling those functions, eval_sub happens to call
> Flength on the args, and that triggers an error if the form is not
> a proper list, so `XCDR (args)` will indeed be a cons once we get
> to Fif.

That's true only if the S-expression does not mutate as it is being evaluated.
If the S-expression modifies itself after Flength and before XCDR, Emacs can
crash. So Alex's first patch is the opposite of what it should be: instead of
changing the Fcar to an XCAR, we should change an XCDR to an Fcdr in the next
line. I looked for nearby instances of this problem, and fixed them by
installing the attached patch. This should address both problems that Alex
mentioned.

> Arguably Fif could be called from elsewhere than eval_sub, and arguably
> eval_sub's implementation could be changed in such a way that it doesn't
> catch this error, so the safety of using XCDR is debatable.

Even before the attached patch was installed, several UNEVALLED functions
assumed that their arguments were proper lists. So, when writing the
abovementioned patch, I found it simpler to make this assumption everywhere. The
argument lists might become improper if they are mutated, so UNEVALLED functions
that can invoke arbitrary lisp code still must check the lists as they go, though.

> Fif should not be performance sensitive

Yes, in this part of eval.c using XCDR instead of Fcdr is helpful mostly as a
hint to the reader that the object is a cons; it's not a significant performance
improvement.

0001-Fix-some-crashes-on-self-modifying-Elisp-code.patch (11K) Download Attachment
Loading...