Re: [PATCH v3 6/7] Use PRINTF_FORTIFY instead of _IO_FLAGS2_FORTIFY (bug 11319)

[Szabolcs Nagy <Szabolcs dot Nagy at arm dot com>]

On 18/12/2018 17:31, Gabriel F. T. Gomes wrote:
> On Tue, 18 Dec 2018, Florian Weimer wrote:
> 
>> * Szabolcs Nagy:
>>
>>> On 15/11/2018 21:44, Gabriel F. T. Gomes wrote:  
>>>> +/* This function is called by regular vsprintf with maxlen set to -1,
>>>> +   and by vsprintf_chk with maxlen set to the size of the output
>>>> +   string.  In the former case, _IO_str_chk_overflow will never be
>>>> +   called; in the latter case it will crash the program if the buffer
>>>> +   overflows.  */
>>>> +
>>>>  int
>>>> -__vsprintf_internal (char *string, const char *format, va_list args,
>>>> +__vsprintf_internal (char *string, size_t maxlen,
>>>> +		     const char *format, va_list args,
>>>>  		     unsigned int mode_flags)
>>>>  {
>>>>    _IO_strfile sf;
>>>> @@ -38,17 +77,22 @@ __vsprintf_internal (char *string, const char *format, va_list args,
>>>>    sf._sbf._f._lock = NULL;
>>>>  #endif
>>>>    _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL);
>>>> -  _IO_JUMPS (&sf._sbf) = &_IO_str_jumps;
>>>> -  _IO_str_init_static_internal (&sf, string, -1, string);
>>>> +  _IO_JUMPS (&sf._sbf) = &_IO_str_chk_jumps;
>>>> +  string[0] = '\0';  
>>>
>>> note that since this change the following code changed behaviour:
>>>
>>> #include <stdio.h>
>>> int main()
>>> {
>>>   char buf[20] = "AB";
>>>   sprintf (buf, "%sCD", buf);
>>>   puts (buf);
>>> }
>>>
>>> this is ub in iso c, but previously printed "ABCD", but now
>>> buf[0]=0 before the format string is processed, so it is "CD".
>>>
>>> this is a heads up, since this pattern seems to appear in existing code,
>>> in particular in SPEC2017 507.cactuBSSN_r/src/PUGH/PughUtils.c:
>>>
>>> sprintf(mess,"  Size:");
>>> for (i=0;i<dim+1;i++)
>>> {
>>>     sprintf(mess,"%s %d",mess,pughGH->GFExtras[dim]->nsize[i]);
>>> }  
>>
>> I think we should make this specific to the fortify flag, even though
>> it's undefined.
> 
> So, do you think that moving the `string[0] = '\0';' statement from
> __vsprintf_internal to ___vsprintf_chk (before the second calls the first)
> is an adequate fix?
> 
> I could write this with an accompanying test case to avoid regressions in
> both fortified and non-fortified cases.
> 


i would just do

if (maxlen == -1)
  .. old code ..
else
  .. new code ..

to revert the old sprintf behaviour.

later the code may be refactored further (it should be
possible to use shared code between sprintf, snprintf
and sprintf_chk, but that requires a lot more changes).