GNU bug report logs - #75754
styled_format stack usage/GC protection

Previous Next

Full log

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

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: eggert <at> cs.ucla.edu, 75754 <at> debbugs.gnu.org
Subject: Re: bug#75754: styled_format stack usage/GC protection
Date: Thu, 23 Jan 2025 23:58:41 +0000

Pip Cet <pipcet <at> protonmail.com> writes:

> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
>> What did I miss?
>
> Can we make this a new bug?

Whether this is the same bug or a different one depends on how you look
at it: both can be interpreted as a violated no-GC assumption, or the
first one can be interpreted as a result of the excessive stack usage
while the second one is due to SDATA relocation.

There are three ways to fix the SDATA relocation issue:

1. Reload format_start and format (and end, and format0) after every call which might have
GC'd.  If you think we should do this, please tell me whether
lisp_string_width can GC.  More importantly, assuming it doesn't,
document this in every function in the call tree starting at
lisp_string_width so we don't accidentally change it.

2. memcpy the format string.  Two-liner, more likely to fix the bug for
good than (1), wastes more memory (since sa_avail has been negative
since we entered the function, this is xmalloc'd memory).

3. replace format by a ptrdiff_t and all instances of *format by
SREF (args[0], index).  Faster than 2, but many changes hurting
readability.

> diff --git a/src/editfns.c b/src/editfns.c
> index 4ba356d627c..23a5f9aeac6 100644
> --- a/src/editfns.c
> +++ b/src/editfns.c
> @@ -3491,7 +3491,7 @@ styled_format (ptrdiff_t nargs, Lisp_Object *args, bool message)
>    /* If we start out planning a unibyte result,
>       then discover it has to be multibyte, we jump back to retry.  */
>   retry:
> -
> +  format_start = SSDATA (args[0]);
>    p = buf;
>    nchars = 0;
>  
>
> should fix it.

Just be clear here: that fixes the specific test.  It does not fix the
bug.

Here's (2), which seems most doable and, I think, may fix the bug,
unless there's a subtlety I missed.

diff --git a/src/editfns.c b/src/editfns.c
index 4ba356d627c..72cbbb51c40 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -3442,9 +3442,10 @@ styled_format (ptrdiff_t nargs, Lisp_Object *args, bool message)
   } *info;
 
   CHECK_STRING (args[0]);
-  char *format_start = SSDATA (args[0]);
   bool multibyte_format = STRING_MULTIBYTE (args[0]);
   ptrdiff_t formatlen = SBYTES (args[0]);
+  char *format_start = SAFE_ALLOCA (formatlen);
+  memcpy (format_start, SSDATA (args[0]), formatlen);
   bool fmt_props = !!string_intervals (args[0]);
 
   /* Upper bound on number of format specs.  Each uses at least 2 chars.  */

Note that (3) technically introduces another bug: we call out to Lisp,
which can modify args[0], causing it to be resized if it's multibyte.
We'd use the old bytecount, and might read past the end of the string.

feature/igc avoids all of these problems by keeping SDATA pointers
valid, even when the string is resized.

Pip

Previous Next

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