Subject: bug#75845: mapconcat crashes for unreasonable self-modifications

From: Mattias Engdegård <[email-masked]>
Date: Thu, 20 Feb 2025 19:16:42 +0100

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

Finally I got some time to look at this, and the patch looks fine, largely.

Now that `mapcar1` always returns its first argument (leni), the return value can be eliminated.

The change in the list loop is cheap and fine.

The new tests and logic in the string loop are more expensive (5-10 % overhead, from a quick test) but given that strings are less common in calls to mapcar etc, it's probably not so bad. More significantly, these checks will go away entirely when we abolish size-changing string mutation -- mentioned elsewhere but there is no need for that issue to be resolved before this bug.

We should probably amend the documentation for mapcar & co with respect to the restrictions on mutating the input sequence. (Common Lisp has basically the same restrictions.) At least the manual would do well to include a short notice about this; we do mention it for `maphash`.

We could just contend ourselves with stating that the input sequence shouldn't be mutated and that's it, but if you really want to spell it out in detail (allowed to change the CARs but not the CDRs etc) then that's fine. Again, strings are messy here because it depends on internal representation, so maybe keep it simple.