bug#24118: 25.1; [PATCH] Fix a possible crash caused by mapcar1

bug#24118: 25.1; [PATCH] Fix a possible crash caused by mapcar1

[Chris Feng]

Processing a list with `mapcar' or `mapconcat' can be terminated early
when the list is tampered (as shown in the following example), and as a
result we'll be dealing with uninitialized memory which will likely
trigger a crash.

  (setq a (make-list 10 0))
  (mapcar (lambda (_)
            (setcdr a nil))
          a)

Chris

---

* src/fns.c (mapcar1): Check and reset uninitialized list elements.
---
 src/fns.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/fns.c b/src/fns.c
index d5a1f74..1804bce 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2524,6 +2524,10 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
 	    vals[i] = dummy;
 	  tail = XCDR (tail);
 	}
+
+      /* In case the list was tampered and the loop terminated early. */
+      if (i < leni)
+        memclear (vals + i, (leni - i) * word_size);
     }
 }
 
-- 
2.8.1

bug#24118: 25.1; [PATCH] Fix a possible crash caused by mapcar1

[Andreas Schwab]

Chris Feng <chris.w.feng@gmail.com> writes:

> diff --git a/src/fns.c b/src/fns.c
> index d5a1f74..1804bce 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -2524,6 +2524,10 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
>  	    vals[i] = dummy;
>  	  tail = XCDR (tail);
>  	}
> +
> +      /* In case the list was tampered and the loop terminated early. */
> +      if (i < leni)
> +        memclear (vals + i, (leni - i) * word_size);

That should not depend on the representation of Qnil.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#24118: 25.1; [PATCH] Fix a possible crash caused by mapcar1

[Chris Feng]

Andreas Schwab <schwab@linux-m68k.org> writes:

> That should not depend on the representation of Qnil.

I think the result is undefined.  I set it to Qnil because it was before
60d1b18.

Chris

bug#24118: 25.1; [PATCH] Fix a possible crash caused by mapcar1

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

Thanks for the bug report. I installed the attached more-adventurous patch, 
which truncates the result rather than extending it with nils. This seems a bit 
more appropriate anyway.

Although it no longer matters for this patch, memclear is specified to store nil 
values regardless of how nil is represented. Of course memclear's current 
implementation assumes Qnil is zero, and memclear can't be portably and easily 
implemented if we merely change Qnil to be nonzero, but that's a bridge we don't 
have to cross unless we change Qnil to be nonzero.

[-- Attachment #2: 0001-Fix-mapcar-F-S-crash-when-F-alters-S-s-length.txt --]
[-- Type: text/plain, Size: 4881 bytes --]

From ae77c07b399a194bdb7095823595086f8385f2a4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 2 Aug 2016 21:04:28 -0400
Subject: [PATCH] =?UTF-8?q?Fix=20(mapcar=20F=20S)=20crash=20when=20F=20alt?=
 =?UTF-8?q?ers=20S=E2=80=99s=20length?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/fns.c (mapcar1): Return number of elements computed,
which can be less than LENI if the function alters the list.
All callers changed.  (Bug#24118)
---
 src/fns.c | 77 +++++++++++++++++++++++++--------------------------------------
 1 file changed, 30 insertions(+), 47 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index c318608..3895ada 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2517,11 +2517,13 @@ usage: (nconc &rest LISTS)  */)
 }
 \f
 /* This is the guts of all mapping functions.
- Apply FN to each element of SEQ, one by one,
- storing the results into elements of VALS, a C vector of Lisp_Objects.
- LENI is the length of VALS, which should also be the length of SEQ.  */
+   Apply FN to each element of SEQ, one by one, storing the results
+   into elements of VALS, a C vector of Lisp_Objects.  LENI is the
+   length of VALS, which should also be the length of SEQ.  Return the
+   number of results; although this is normally LENI, it can be less
+   if SEQ is made shorter as a side effect of FN.  */
 
-static void
+static EMACS_INT
 mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
 {
   Lisp_Object tail, dummy;
@@ -2564,14 +2566,18 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
   else   /* Must be a list, since Flength did not get an error */
     {
       tail = seq;
-      for (i = 0; i < leni && CONSP (tail); i++)
+      for (i = 0; i < leni; i++)
 	{
+	  if (! CONSP (tail))
+	    return i;
 	  dummy = call1 (fn, XCAR (tail));
 	  if (vals)
 	    vals[i] = dummy;
 	  tail = XCDR (tail);
 	}
     }
+
+  return leni;
 }
 
 DEFUN ("mapconcat", Fmapconcat, Smapconcat, 3, 3, 0,
@@ -2581,34 +2587,26 @@ SEPARATOR results in spaces between the values returned by FUNCTION.
 SEQUENCE may be a list, a vector, a bool-vector, or a string.  */)
   (Lisp_Object function, Lisp_Object sequence, Lisp_Object separator)
 {
-  Lisp_Object len;
-  EMACS_INT leni;
-  EMACS_INT nargs;
-  ptrdiff_t i;
-  Lisp_Object *args;
-  Lisp_Object ret;
   USE_SAFE_ALLOCA;
-
-  len = Flength (sequence);
+  EMACS_INT leni = XFASTINT (Flength (sequence));
   if (CHAR_TABLE_P (sequence))
     wrong_type_argument (Qlistp, sequence);
-  leni = XINT (len);
-  nargs = leni + leni - 1;
-  if (nargs < 0) return empty_unibyte_string;
-
-  SAFE_ALLOCA_LISP (args, nargs);
-
-  mapcar1 (leni, args, function, sequence);
+  EMACS_INT args_alloc = 2 * leni - 1;
+  if (args_alloc < 0)
+    return empty_unibyte_string;
+  Lisp_Object *args;
+  SAFE_ALLOCA_LISP (args, args_alloc);
+  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
+  ptrdiff_t nargs = 2 * nmapped - 1;
 
-  for (i = leni - 1; i > 0; i--)
+  for (ptrdiff_t i = nmapped - 1; i > 0; i--)
     args[i + i] = args[i];
 
-  for (i = 1; i < nargs; i += 2)
+  for (ptrdiff_t i = 1; i < nargs; i += 2)
     args[i] = separator;
 
-  ret = Fconcat (nargs, args);
+  Lisp_Object ret = Fconcat (nargs, args);
   SAFE_FREE ();
-
   return ret;
 }
 
@@ -2618,24 +2616,15 @@ The result is a list just as long as SEQUENCE.
 SEQUENCE may be a list, a vector, a bool-vector, or a string.  */)
   (Lisp_Object function, Lisp_Object sequence)
 {
-  register Lisp_Object len;
-  register EMACS_INT leni;
-  register Lisp_Object *args;
-  Lisp_Object ret;
   USE_SAFE_ALLOCA;
-
-  len = Flength (sequence);
+  EMACS_INT leni = XFASTINT (Flength (sequence));
   if (CHAR_TABLE_P (sequence))
     wrong_type_argument (Qlistp, sequence);
-  leni = XFASTINT (len);
-
+  Lisp_Object *args;
   SAFE_ALLOCA_LISP (args, leni);
-
-  mapcar1 (leni, args, function, sequence);
-
-  ret = Flist (leni, args);
+  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
+  Lisp_Object ret = Flist (nmapped, args);
   SAFE_FREE ();
-
   return ret;
 }
 
@@ -2661,21 +2650,15 @@ the results by altering them (using `nconc').
 SEQUENCE may be a list, a vector, a bool-vector, or a string. */)
      (Lisp_Object function, Lisp_Object sequence)
 {
-  register EMACS_INT leni;
-  register Lisp_Object *args;
-  Lisp_Object ret;
   USE_SAFE_ALLOCA;
-
+  EMACS_INT leni = XFASTINT (Flength (sequence));
   if (CHAR_TABLE_P (sequence))
     wrong_type_argument (Qlistp, sequence);
-
-  leni = XFASTINT (Flength (sequence));
+  Lisp_Object *args;
   SAFE_ALLOCA_LISP (args, leni);
-  mapcar1 (leni, args, function, sequence);
-  ret = Fnconc (leni, args);
-
+  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
+  Lisp_Object ret = Fnconc (nmapped, args);
   SAFE_FREE ();
-
   return ret;
 }
 \f
-- 
2.5.5