Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Eli Zaretskii]

> Date: Tue, 02 Mar 2021 16:14:18 +0900 (JST)
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> From: Naoya Yamashita <conao3@gmail.com>
> 
> I compared the two and found that the problem of crashing on
> (if . "abc") crash problem was fixed in the former.
> 
> However
> 
>     (let ((vars (list 'v))))
>       (setcdr vars vars)
>       (eval (list let vars))))
> 
> and
> 
>     (let ((clauses (list '((progn (setcdr clauses "ouch")) nil)))))
>       (eval (cons 'cond clauses))))
> 
> did not crash.  The circular list example became an infinite loop
> in an older version of b3a3ed5 (even older changes to eval.c).
> My change removed the list length checker, which is vulnerable to
> list changes.  Furthermore, Emacs with my patch passes these test
> cases.
> 
> My patched Emacs does not segfault the Pip example either.
> 
>     (let ((cons-cell '((a 2) (b 3))))
>       (eval `(let ((x (setcdr ',cons-cell nil))
>                . ,cons-cell)
>            (message "foo"))))
>     ;;=> foo
> 
> Please let me know if there are any test cases I'm missing.

I think the problem is a general one, and it's impossible to prove it
doesn't exist by providing a small number of examples.  This code was
explicitly added to make the code safer in the face of self-modifying
expressions.

So I think removing it is not TRT.

> > P.S. When I see code in Emacs that looks wrong/redundant/unclear, I
> > find it useful to look at the VC history of that code.  This is how I
> > discovered where this particular "redundancy" came from, just 3.5
> > years ago.
> 
> Thanks, I find `M-x vc-history-region` and this is very useful!
> I should use before write a patch.

Thanks.