GNU bug report logs - #79156
igc: igc_xpalloc_ambig SEGV

Previous Next

Full log

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: Helmut Eller <eller.helmut <at> gmail.com>, 79156 <at> debbugs.gnu.org
Subject: Re: igc: igc_xpalloc_ambig SEGV
Date: Sat, 02 Aug 2025 16:00:23 +0000

Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:

> We have
>
> igc.c:
>  3465 void *
>  3466 igc_xpalloc_ambig (void *old_pa, ptrdiff_t *nitems, ptrdiff_t nitems_incr_min,
>  3467                    ptrdiff_t nitems_max, ptrdiff_t item_size)
>  3468 {
>  3469   ptrdiff_t old_nitems = *nitems;
>
> and igc_xpalloc_ambig is called here with a NULL old_pa, and non-zero
> new_size:
>
> charset.c:
>  1135           struct charset *new_table =
>  1136 #ifdef HAVE_MPS
>  1137             igc_xpalloc_ambig
>  1138 #else
>  1139             xpalloc
>  1140 #endif
>  1141             (0, &new_size, 1,
>  1142              min (INT_MAX, MOST_POSITIVE_FIXNUM),
>  1143              sizeof *charset_table);
>
> In this case, this loop deferences a null pointer.
>
> igc.c:
>  3477   mps_word_t *new_word = new_pa;
>  3478   for (ptrdiff_t i = 0; i < (old_nitems * item_size) / sizeof (mps_word_t); i++)
>  3479     new_word[i] = old_word[i];
>
> This apparently currently never happens in feature/igc, but it could,
> and it does when using igc with emacs-mac's mac-win.el which defines
> charsets.

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.

(Of course, the reason for the static charset_table_init variable was
unexec, and now that reason is gone we should just remove it and use
plain xpalloc)

> Quick workaround is something like
>
> modified   src/igc.c
> @@ -3466,7 +3466,7 @@ igc_xfree (void *p)
>  igc_xpalloc_ambig (void *old_pa, ptrdiff_t *nitems, ptrdiff_t nitems_incr_min,
>  		   ptrdiff_t nitems_max, ptrdiff_t item_size)
>  {
> -  ptrdiff_t old_nitems = *nitems;
> +  ptrdiff_t old_nitems = old_pa == NULL ? 0 : *nitems;
>    ptrdiff_t new_nitems = *nitems;
>    ptrdiff_t nbytes = xpalloc_nbytes (old_pa, &new_nitems, nitems_incr_min,
>  				     nitems_max, item_size);

LGTM.

> Likewise in other igc_xpalloc variants, and one could exploit old_pa ==
> NULL, and so on.

Do we need it for those?

> If someone else has the time to fix this, that would be nice because I'm
> doing something else ATM.

I'll push this if there are no objections.

commit 962e9be111402e1c83c418bea9e75f23a423ccc6 (HEAD)
Author: Pip Cet <pipcet <at> protonmail.com>
Date:   Sat Aug 2 15:51:13 2025 +0000

    [MPS] Avoid segfaults for some inconsistent arguments in igc_xpalloc
    
    Fdefine_charset_internal calls igc_xpalloc with a NULL pointer as
    OLD_PA and a non-zero *NITEMS value.
    
    * src/igc.c (igc_xpalloc_ambig):
    (igc_xpalloc_exact):
    (igc_xpalloc_lisp_objs_exact): Handle OLD_PA == NULL, *NITEMS != 0.

diff --git a/src/igc.c b/src/igc.c
index f3c98fbe49c..4eb57a3072b 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -3473,7 +3473,7 @@ igc_xfree (void *p)
 igc_xpalloc_ambig (void *old_pa, ptrdiff_t *nitems, ptrdiff_t nitems_incr_min,
 		   ptrdiff_t nitems_max, ptrdiff_t item_size)
 {
-  ptrdiff_t old_nitems = *nitems;
+  ptrdiff_t old_nitems = old_pa ? *nitems : 0;
   ptrdiff_t new_nitems = *nitems;
   ptrdiff_t nbytes = xpalloc_nbytes (old_pa, &new_nitems, nitems_incr_min,
 				     nitems_max, item_size);
@@ -3496,7 +3496,7 @@ igc_xpalloc_exact (void **pa_cell, ptrdiff_t *nitems,
 		   void *closure)
 {
   void *old_pa = *pa_cell;
-  ptrdiff_t old_nitems = *nitems;
+  ptrdiff_t old_nitems = old_pa ? *nitems : 0;
   ptrdiff_t new_nitems = *nitems;
   ptrdiff_t nbytes = xpalloc_nbytes (old_pa, &new_nitems, nitems_incr_min,
 				     nitems_max, item_size);
@@ -3556,7 +3556,7 @@ igc_xpalloc_lisp_objs_exact (Lisp_Object *pa, ptrdiff_t *nitems,
 			     ptrdiff_t nitems_incr_min, ptrdiff_t nitems_max,
 			     const char *label)
 {
-  ptrdiff_t nitems_old = *nitems;
+  ptrdiff_t nitems_old = pa ? *nitems : 0;
   ptrdiff_t nitems_new = nitems_old;
   ptrdiff_t nbytes
     = xpalloc_nbytes (pa, &nitems_new, nitems_incr_min, nitems_max, word_size);

Pip

Previous Next

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