GNU bug report logs - #75845
mapconcat crashes for unreasonable self-modifications

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>, Mattias Engdegård <mattiase <at> acm.org>
Cc: 75845 <at> debbugs.gnu.org
Subject: bug#75845: mapconcat crashes for unreasonable self-modifications
Date: Sun, 26 Jan 2025 08:56:03 +0200

> Date: Sun, 26 Jan 2025 02:37:37 +0000
> From:  Pip Cet via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> Pip Cet <pipcet <at> protonmail.com> writes:
> 
> > If the mapping function supplied to mapconcat modifies the sequence
> > being mapped over, Emacs can crash.
> >
> > This seems easily fixable, but I wanted to file a bug to make sure we
> > get it right: our current highly-optimized implementation of mapconcat
> > will fail in many cases in which a naive implementation of mapconcat (in
> > Lisp, say) would succeed.  Some of those limitations are documented (the
> > argument must be a sequence when the function is called), some aren't
> > (the sequence must not become shorter or longer).
> >
> > My suggestion is to make the ordinary no-modification case work as fast
> > as is possible under the constraint that extraordinary
> > self-modifications should not result in crashes.  They can and will
> > result in unpredictable behavior, though.
> >
> > These crash:
> >
> > (let ((l (make-list 1000 nil))) (mapconcat (lambda (x) (ntake 1 l)) l))
> >
> > (let ((s (string ?a 1000))) (mapconcat (lambda (x) (aset s 0 1000) (string x)) s))
> >
> > Patch will follow once this has a bug number.
> 
> This patch does the minimum work necessary; while the previous code
> attempted to return the "correct" length of a list which was modified,
> crashing in an eassert in mapconcat, the new code errors instead.  As
> error is marked with ATTRIBUTE_COLD, this should result in better code.
> 
> The string case is slightly trickier: we don't keep around an SDATA
> pointer, which is good, but we do remember byte and charpos index
> values, and those might get out of sync.  So verify we're looking at a
> CHAR_HEAD when we need to, which is for multibyte strings.
> 
> mapconcat itself is also fixed not to assert but to use whatever mapcar1
> returns.

Thanks, adding Stefan and Mattias to the discussion, in the hope that
they could review the patch and comment.

>  	  if (! CONSP (tail))
> -	    return i;
> +	    error ("list modified by mapping function");

I think this error message is not explicit enough: it doesn't tell
which list was modified by what function.  mapcar and mapconcat are
many times invoked by code that has little to do with the level which
could make sense to the user, so we should try to provide as much
contextual information as possible.  Perhaps something like

  sequence arg to `mapcar' or `mapconcat' modified by its mapping function arg

Better suggestions are welcome.

> +	  if (STRING_MULTIBYTE (seq) && !CHAR_HEAD_P (SREF (seq, i_byte)))
> +	    error ("string modified by mapping function");

AFAIU, this doesn't check whether the string was modified, it checks
for modifications that invalidate the relation between i and i_byte,
right?  So I would suggest

  string arg of mapcar or mapconcat clobbered by its mapping function arg

Because if we want to disallow _any_ modifications of the string, we'd
need to use a different test (e.g., unibyte ASCII strings could also
be modified by the mapping function).

> +      if (i < leni)
> +	error ("string modified by mapping function");

Why do we need this test?  Does it prevent us from crashing in some
cases?  If that replaces the below:

> @@ -3473,8 +3478,7 @@ DEFUN ("mapconcat", Fmapconcat, Smapconcat, 2, 3, 0,
>  	  goto concat;
>  	}
>      }
> -  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
> -  eassert (nmapped == leni);
> +  leni = mapcar1 (leni, args, function, sequence);

then can we have the test as eassert?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.