>>>> "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);