[PATCH] debug: State buffer overflow message more precisely

[Adhemerval Zanella Netto]

On 11/07/24 17:16, Ingo Blechschmidt wrote:
> Dear Adhemerval,
> 
> thank you for your detailed review of my proposed patch! Much
> appreciated.
> 
> On Thu Jul 11 11:34:45 2024, Adhemerval Zanella Netto wrote:
>> And I don't think changing the error message improves anything here,
>> if __chk_fail is called the program will be terminated anyway.  If you
>> think this is a false-positive, we will need to check whether the compiler
>> expansion for the strlcpy is being correct (meaning that __glibc_objsize
>> is passing the expected values), not to paper over the log.
> 
> I'm not sure I follow.
> 
> In my view, source fortification correctly identified a potentially
> problematic call in Privoxy. But closer inspection shows that the call
> is actually safe, hence strictly speaking it was a false positive.
> 
> But I wouldn't blame source fortification: It sure looks like a true
> positive! &(c->userid) is indeed a pointer to a region of just size 1.
> How should source fortification know that, at runtime, the memory
> directly proceeding c->userid is allocated as well?
> 
> Privoxy could help source fortification by using a proper variable
> length array instead of faking it. But still, I believe the Privoxy code
> to be correct and hence the abort to be erroneous. As it's safer to
> erroneously abort instead of erroneously continuing, I am all in favor
> of the currect practice of aborting the program. I just thought that the
> error message could be made more precise.> 
> Please correct me if I'm mistaken: I got the impression that the purpose
> of source fortification is to abort not only in cases where a buffer
> overflow would undoubtedly occur, but also in cases where it's very
> reasonable to believe that a buffer overflow is about to occur.

The problem is once you add UB in the mix, there is no much the compiler
can do to help the fortification.  The object size passed along on the
fortify wrappers through __builtin_object_size/__builtin_dynamic_object_size
required well-behave code, if you start to using alias violation to make
clever hacks there no much compiler or runtime can do.

So sorry, but this patch does not make any sense.