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

Previous Next

Full log

Message #17 received at 75845 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: mattiase <at> acm.org, monnier <at> iro.umontreal.ca, 75845 <at> debbugs.gnu.org
Subject: Re: bug#75845: mapconcat crashes for unreasonable self-modifications
Date: Sun, 26 Jan 2025 12:24:23 +0200

> Date: Sun, 26 Jan 2025 09:48:48 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, Mattias Engdegård <mattiase <at> acm.org>, 75845 <at> debbugs.gnu.org
> 
> I don't think this situation is expected to be common enough to warrant
> a more specific error message, TBH, particularly not if it then fails to
> apply to all possible cases.

I didn't want the message to be specific.  I wanted it to explain
which function signaled the error, so the bug could be easier to look
for and spot.

> My suggestion is to merge the two error messages to save some space,

I'm okay with using a single error message.  But let's make its text
provide more context than just saying that some list was modified by
some function.

> > Does it prevent us from crashing in some cases?
> 
> Yes.  Silly programmers like myself (see first suggested patch in
> another bug) may attempt to use out-of-bounds values in the array, which
> are uninitialized and would cause crashes.

No, I meant can we crash in the following code.  If the caller then
uses the value somewhere else, without validating it, it's a separate
problem that should be perhaps dealt with at that other place.

> >   sequence arg to `mapcar' or `mapconcat' modified by its mapping function arg
> 
> I like the idea of using the same message for both, reducing the memory
> requirement for the unused error string.

Then how about

  sequence arg to `mapcar' etc. modified by its mapping function arg

> (I may be unaware of a good
> error function to use in this case; I ended up not providing additional
> arguments because it seems unlikely to me that this problem, if really
> accidental, is something the programmer can fix without a Lisp
> backtrace, which indicates the situation more clearly).

The above just provides more context, to make it easier to look for
the likely culprits.  Arguments are not provided, just mentioned.

> >> -  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?
> 
> I see no reason to set this trap for the next person who modifies
> mapcar1.  If they see the return value is checked by some callers, they
> might miss the one caller which crashes.  If they remove the return
> value, they might miss that the eassert, too, needs to be removed,
> making their job harder.
> 
> If we want that eassert (why?), it belongs in mapcar1, not Fmapconcat.

That's what I meant: to make the test an assertion in mapcar1.

Previous Next

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