Subject: Re: bug#75754: styled_format stack usage/GC protection

From: Pip Cet <[email-masked]>
Date: Thu, 23 Jan 2025 23:58:41 +0000

Pip Cet <[email-masked]> writes:

> "Eli Zaretskii" <[email-masked]> 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