Subject: Re: bug#75768: Missing assertions to detect access to GC-freed strings

From: Eli Zaretskii <[email-masked]>
Date: Thu, 23 Jan 2025 20:16:46 +0200

> Date: Thu, 23 Jan 2025 17:42:16 +0000
> From:  Pip Cet via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <[email-masked]>
> 
> Pip Cet <[email-masked]> writes:
> 
> > Even with this patch, we would fail to recognize impossible strings
> > automatically if even a single bit in the data pointer is set.  I don't
> > have a better solution yet, though, but the patch (to follow as soon as
> > this has a bug number) would have detected this particular problem a
> > little sooner.
> 
> Patch follows, I've confirmed that it would have detected this problem
> very slightly sooner.

Thanks.

> +/* /\* Should always return true. */  */

I wonder why did you comment out the comment?

> +INLINE bool
> +LIVE_STRING_P (Lisp_Object string)
> +{
> +  return XSTRING (string)->u.s.data != NULL;

Can we please have a comment here saying that sweep_strings sets the
data pointer to NULL, and thus this function checks whether a string
was GC'ed?

>  INLINE ptrdiff_t
>  SCHARS (Lisp_Object string)
>  {
> +  eassert (LIVE_STRING_P (string));
>    ptrdiff_t nchars = XSTRING (string)->u.s.size;
>    eassume (0 <= nchars);
>    return nchars;
> @@ -1717,6 +1726,7 @@ SCHARS (Lisp_Object string)
>  INLINE ptrdiff_t
>  STRING_BYTES (struct Lisp_String *s)
>  {
> +  eassert (LIVE_STRING_P (make_lisp_ptr (s, Lisp_String)));

I understand why we want an assertion in SDATA and SSDATA, but do we
really want it in SCHARS and STRING_BYTES?  Those do not access the
string text, so how can they be harmful?