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

From: Eli Zaretskii <[email-masked]>
Date: Sat, 25 Jan 2025 15:23:56 +0200

> Date: Sat, 25 Jan 2025 09:45:19 +0000
> From: Pip Cet <[email-masked]>
> Cc: [email-masked], [email-masked], [email-masked]
> 
> "Eli Zaretskii" <[email-masked]> writes:
> 
> >> Date: Fri, 24 Jan 2025 22:35:50 +0000
> >> From: Pip Cet <[email-masked]>
> 
> >> >> >> > Even if the code modifies the string after GC?
> >> >> >>
> >> >> >> If the GC free'd the string, but not the SDATA structure, we cannot
> >> >> >> regenerate the string to call resize_string_data.  So, no, this cannot
> >> >> >> happen.
> >> >> >>
> >> >> >> Note that resizing a live string while you have a live SDATA pointer to
> >> >> >> it might result in the SDATA pointer referring to the pre-modification
> >> >> >> SDATA, not the current one.
> >> >> >
> >> >> > Who said anything about resizing?
> >> >>
> >> >> Non-resizing Faset is boring, works as expected.
> >> >
> >> > But it will modify the string whose SDATA pointer is different, no?
> >>
> >> Different how?
> >
> > Different as in: "pointing to a location different from the data of
> > the string that was moved by GC".
> 
> I'm sorry, this may sound like you as though I'm being pedantic, but I
> need to be sure that we're talking about the same thing here:
> 
> The scenario you describe involves a single string object, but two sdata
> areas.  The single string points to the first sdata area, but then is
> modified to point to the second.  Modifying the first sdata area after
> the second one has been created will have no effect whatsoever on the
> string.
> 
> >> There's only one string in this example, and after the
> >> string's SDATA changes, you can access the old SDATA, but it doesn't
> >> belong to a string anymore.
> >
> > That's what I thought.  So code that expects the old SDATA pointer to
> > be pointing to the same string after GC will have a bug on the igc
> 
> feature/igc: GC doesn't invalidate SDATA pointers.  Resizing string
> modification does (the other way is to call pin_string).
> 
> > branch.  It's a different bug than under the old GC, but still a bug.
> > Right?
> 
> If there's modification involved, it's a bug which will cause apparently
> random behavior but no crashes.  If there's no modification, it's a bug
> only because the code might be used with alloc.c GC.
> 
> >> >   Lisp_Object my_string;
> >> >   char *p = SDATA (my_string);
> >> >   /* trigger GC */
> >> >   Faset (my_string, make_fixnum (0), make_fixnum (97));
> >> >   /* code which expects p[0] to be 'a' */
> >>
> >> If that is all the code, this is safe with feature/igc.
> >
> > You say "safe", but it sounds like we have different definitions of
> > "safe".  Because later you say:
> 
> I meant "won't crash, but most likely useless".
> 
> >> MPS GC will never make pointers that live on the stack invalid.  It's
> >> transparent to correct client programs.  So, yes, if you're asking about
> >> MPS GC, the sequence is safe and doesn't crash, but may surprise you by
> >> reproducing the old string data.
> >
> > Which seems to mean your "safe" means the code will not crash, whereas
> > what I meant is the correctness of the code.  Code which refers to the
> > old string data is incorrect, even if it won't crash.
> 
> I believe that's true.

The above confirms what I thought about this, thanks.

> > So my conclusion is that even with MPS, referring in code after GC to
> > an SDATA pointer taken before GC leads to incorrect code.  IOW,
> > pointers to SDATA of a string cannot be relied upon to be valid
> > (i.e. pointing to the correct string) across GC, so the code must
> > re-initialize such pointer after any fragment that could GC.  This was
> > the rule with the old GC, and it should continue to be the rule with
> > MPS.
> 
> No.

And this seems to contradict it.

> GC doesn't invalidate SDATA pointers.

I never said anything about invalidating the SDATA pointers.  So if
your "No" is because of the (lack of) invalidation, then it is not
relevant to what I was asking about.  Code can be incorrect even if it
doesn't crash or otherwise refers to invalid pointers.  It could be
incorrect because it modifies incorrect memory believing that it
modifies the data of a string.