>>>> "Hrv" == Hrvoje Niksic
<hniksic(a)iskon.hr> writes: 
Hrv> Martin Buchholz <martin(a)xemacs.org> writes:
> I noticed this code in mapcar1:
> 
> else if (STRINGP (seq))
> {
> Bufbyte *p = XSTRING_DATA (seq);
> for (i = 0; i < leni; i++)
> {
> args[1] = make_char (charptr_emchar (p));
> INC_CHARPTR (p);
> result = Ffuncall (2, args);
> if (vals) vals[gcpro1.nvars++] = result;
> }
> }
> 
> which, it seems to me, could crash if STRING_DATA gets relocated. 
Hrv> Congratulations!  I think you've just diagnosed a long-standing source
Hrv> of "mysterious" crashes in mapconcat.  Copying the string's data
out
Hrv> of harm's way is probably a good solution, although not one I'm
Hrv> entirely happy with.  (I sometimes wish XEmacs could do less copying.)
> else if (STRINGP (sequence))
> {
> /* SEQUENCE might be relocated during GC. */ 
Hrv> Not SEQUENCE, but SEQUENCE's STRING_DATA.  You should be very clear so
Hrv> that a later fiddler doesn't cry "no it can't!" and
"fix" the code.
> size_t string_length = XSTRING_LENGTH (sequence);
> Bufbyte *p = alloca_array (Bufbyte, string_length);
> memcpy (p, XSTRING_DATA (sequence), string_length); 
Hrv> This is probably philosophical, but shouldn't the result of
Hrv> XSTRING_LENGTH should be assigned to a Bytecount?
> for (i = 0; i < leni; i++)
> {
> args[1] = make_char (charptr_emchar (p));
> INC_CHARPTR (p);
> vals[gcpro1.nvars++] = Ffuncall (2, args);
> }
> } 
Hrv> or:
Hrv>   Bufbyte *end = p + string_length;
Hrv>   while (p < end)
Hrv>     {
Hrv>       ...
Hrv>     }
Here's a new patch, containing ONLY bug fixes, and incorporating
Hrvoje's suggestions:
ChangeLog:
1999-12-17  Martin Buchholz  <martin(a)xemacs.org>
	* fns.c (mapcar1): Fix ***THREE*** obscure crashes.
Index: fns.c
===================================================================
RCS file: /usr/CVSroot/XEmacs/xemacs/src/fns.c,v
retrieving revision 1.30.2.23
diff -u -w -r1.30.2.23 fns.c
--- fns.c	1999/10/24 03:48:39	1.30.2.23
+++ fns.c	1999/12/17 10:35:29
@@ -3068,14 +3068,50 @@
 
   if (LISTP (seq))
     {
+      /* A devious `fn' could either:
+	 - insert garbage into the list in front of us, causing XCDR to crash
+	 - break the list behind us using (setcdr), causing the remaining elts
+	   to lose their GCPRO status.
+
+	 We avoid these problems by making a copy of the list to be traversed.
+
+	 (We could also use EXTERNAL_LIST_LOOP and GCPRO the tail).
+
+	 If vals != 0, we can optimize this by fitting the elts to be
+	 traversed into the same space as the results we have computed. */
+
+      if (vals)
+	{
+	  Lisp_Object *val = vals;
+	  Lisp_Object elt;
+
+	  LIST_LOOP_2 (elt, seq)
+	      *val++ = elt;
+
+	  gcpro1.nvars = leni;
+
       for (i = 0; i < leni; i++)
 	{
-	  args[1] = XCAR (seq);
-	  seq = XCDR (seq);
-	  result = Ffuncall (2, args);
-	  if (vals) vals[gcpro1.nvars++] = result;
+	      args[1] = vals[i];
+	      vals[i] = Ffuncall (2, args);
+	    }
+	}
+      else
+	{
+	  Lisp_Object *seq_copy = alloca_array (Lisp_Object, leni);
+	  Lisp_Object *val = seq_copy;
+	  Lisp_Object elt;
+
+	  LIST_LOOP_2 (elt, seq)
+	    *val++ = elt;
+
+	  for (i = 0; i < leni; i++)
+	    {
+	      args[1] = seq_copy[i];
+	      Ffuncall (2, args);
 	}
     }
+    }
   else if (VECTORP (seq))
     {
       Lisp_Object *objs = XVECTOR_DATA (seq);
@@ -3088,8 +3124,14 @@
     }
   else if (STRINGP (seq))
     {
-      Bufbyte *p = XSTRING_DATA (seq);
-      for (i = 0; i < leni; i++)
+      /* The string data of `seq' might be relocated during GC. */
+      Bytecount slen = XSTRING_LENGTH (seq);
+      Bufbyte *p = alloca_array (Bufbyte, slen);
+      Bufbyte *end = p + slen;
+
+      memcpy (p, XSTRING_DATA (seq), slen);
+
+      while (p < end)
 	{
 	  args[1] = make_char (charptr_emchar (p));
 	  INC_CHARPTR (p);