bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists

[Philipp Stephani <p.stephani2@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 455 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 9. Nov. 2016 um
22:19 Uhr:

>
> For example:
>
> (funcall (lambda (&optional &rest &rest &optional x) (list x)) 'a)
> => ((a))
>
> Obviously here the &rest keyword "wins", but I think that's overly
> confusing.  Such an argument list is most likely a programmer mistake,
> and should signal an error to make the programmer aware of the mistake.
>
>
Here's a patch that detects such argument lists.

[-- Attachment #1.2: Type: text/html, Size: 990 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Prevent-dubious-argument-lists.txt --]
[-- Type: text/plain; charset=US-ASCII; name="0001-Prevent-dubious-argument-lists.txt", Size: 3951 bytes --]

From 185586a3377e166a5123407799ab7741d4627c52 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Wed, 9 Nov 2016 23:13:52 +0100
Subject: [PATCH] Prevent dubious argument lists

See Bug#24912 and Bug#24913.

* src/eval.c (funcall_lambda): Detect more dubious argument lists.
* lisp/emacs-lisp/bytecomp.el (byte-compile-check-lambda-list): Detect
more dubious argument lists.
* test/src/eval-tests.el (eval-tests--bugs-24912-and-24913): Add unit
test.
---
 lisp/emacs-lisp/bytecomp.el |  7 +++++--
 src/eval.c                  | 18 +++++++++++++++---
 test/src/eval-tests.el      | 15 +++++++++++++++
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 428e21c..85daa43 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2672,8 +2672,11 @@ byte-compile-check-lambda-list
 	       (when (cddr list)
 		 (error "Garbage following &rest VAR in lambda-list")))
 	      ((eq arg '&optional)
-	       (unless (cdr list)
-		 (error "Variable name missing after &optional")))
+	       (when (or (null (cdr list))
+                         (memq (cadr list) '(&optional &rest)))
+		 (error "Variable name missing after &optional"))
+               (when (memq '&optional (cddr list))
+                 (error "Duplicate &optional")))
 	      ((memq arg vars)
 	       (byte-compile-warn "repeated variable %s in lambda-list" arg))
 	      (t
diff --git a/src/eval.c b/src/eval.c
index caeb791..884e1eb 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2888,6 +2888,7 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs,
     emacs_abort ();
 
   i = optional = rest = 0;
+  bool previous_optional_or_rest = false;
   for (; CONSP (syms_left); syms_left = XCDR (syms_left))
     {
       QUIT;
@@ -2897,9 +2898,19 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs,
 	xsignal1 (Qinvalid_function, fun);
 
       if (EQ (next, Qand_rest))
-	rest = 1;
+        {
+          if (rest || previous_optional_or_rest)
+            xsignal1 (Qinvalid_function, fun);
+          rest = 1;
+          previous_optional_or_rest = true;
+        }
       else if (EQ (next, Qand_optional))
-	optional = 1;
+        {
+          if (optional || rest || previous_optional_or_rest)
+            xsignal1 (Qinvalid_function, fun);
+          optional = 1;
+          previous_optional_or_rest = true;
+        }
       else
 	{
 	  Lisp_Object arg;
@@ -2922,10 +2933,11 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs,
 	  else
 	    /* Dynamically bind NEXT.  */
 	    specbind (next, arg);
+          previous_optional_or_rest = false;
 	}
     }
 
-  if (!NILP (syms_left))
+  if (!NILP (syms_left) || previous_optional_or_rest)
     xsignal1 (Qinvalid_function, fun);
   else if (i < nargs)
     xsignal2 (Qwrong_number_of_arguments, fun, make_number (nargs));
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index 75999e1..fe08506 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -32,4 +32,19 @@
   ;; This should not crash.
   (should-error (funcall '(closure)) :type 'invalid-function))
 
+(ert-deftest eval-tests--bugs-24912-and-24913 ()
+  "Checks that Emacs doesn’t accept weird argument lists.
+Bug#24912 and Bug#24913."
+  (dolist (args '((&optional) (&rest) (&optional &rest) (&rest &optional)
+                  (&optional &rest a) (&optional a &rest)
+                  (&rest a &optional) (&rest &optional a)
+                  (&optional &optional) (&optional &optional a)
+                  (&optional a &optional b)
+                  (&rest &rest) (&rest &rest a)
+                  (&rest a &rest b)))
+    (should-error (eval `(funcall (lambda ,args)) t) :type 'invalid-function)
+    (should-error (byte-compile-check-lambda-list args))
+    (let ((byte-compile-debug t))
+      (should-error (eval `(byte-compile (lambda ,args)) t)))))
+
 ;;; eval-tests.el ends here
-- 
2.8.0.rc3.226.g39d4020