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

["Gabriel F. T. Gomes" <gabriel at inconstante dot eti dot br>]

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.