GNU bug report logs - #79156
igc: igc_xpalloc_ambig SEGV

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: gerd.moellmann <at> gmail.com, eller.helmut <at> gmail.com, 79156 <at> debbugs.gnu.org
Subject: bug#79156: igc: igc_xpalloc_ambig SEGV
Date: Sat, 02 Aug 2025 17:24:35 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Sat, 02 Aug 2025 17:07:35 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, eller.helmut <at> gmail.com, 79156 <at> debbugs.gnu.org
>>
>> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>>
>> > Eli Zaretskii <eliz <at> gnu.org> writes:
>> >
>> >>> Ouch. That seems to me to be a bug in how charset.c calls xpalloc, but
>> >>> I'm not sure whether there are other callers that rely on this behavior,
>> >>> so it's safest to work around it.
>> >>
>> >> xpalloc handles this case:
>> >>
>> >>   if (! pa)
>> >>     *nitems = 0;
>> >
>> > Yeah, I'd rather check the other igc variants of xpalloc, to make sure
>> > they are compatible with the original, even if no one else uses that
>> > particular feature.
>>
>> That's what I did, I think?
>>
>> The code above doesn't have any effect unless we run out of memory (even
>> then, it won't have an effect if NITEMS is a stack variable in a frame
>> that's unwound by memory_full).
>
> The code above in xpalloc doesn't have anything to do with running out
> of memory, because it tests the value of the argument passed to
> xpalloc before calling xrealloc.

Here's the code:

  if (! pa)
    *nitems = 0;
  if (n - n0 < nitems_incr_min
      && (ckd_add (&n, n0, nitems_incr_min)
	  || (0 <= nitems_max && nitems_max < n)
	  || ckd_mul (&nbytes, n, item_size)))
    memory_full (SIZE_MAX);
  pa = xrealloc (pa, nbytes);
  *nitems = n;

Since the final line sets *NITEMS unconditionally, the first two lines
have no effect unless we either call memory_full or xrealloc exits
nonlocally, which it does only if we run out of memory.

So to say the code you quoted "doesn't have anything to do with running
out of memory" seems incorrect to me; it has an effect only if we run
out of memory.

>> The FIXME comment in charset.c should be amended to point out that
>> charset_table usually lives in the pdump, and xpalloc does not like
>> pdumper object pointers, so we'd have to check that before freeing
>> charset_table.
>
> If this is the reason, then there's no reason to call xpalloc in the
> first place, right?  We could call xmalloc instead, since we already
> memcpy the old table to the newly-allocated memory.

I think the point is only to reuse the larger-buffer-size calculation
xpalloc performs. There are probably cleaner ways of doing that.

Pip

Previous Next

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