Re: [PATCH 07/13] gdb/csky: remove uses of sprintf

On 8/17/26 12:26 PM, Andrew Burgess wrote:
> Simon Marchi <[email protected]> writes:
> 
>> When building on macOS, I get a few:
>>
>>     /Users/smarchi/src/binutils-gdb/gdb/csky-tdep.c:434:4: error: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Werror,-Wdeprecated-declarations]
>>       434 |           sprintf (tdesc_reg.name, "cp1cr%d", remain);
>>           |           ^
>>
>> Replace these uses with snprintf, via xsnprintf, which asserts that the
>> destination buffer was large enough for the output string.
>>
>> Change-Id: Idc5c0c42479f767c63b0d0cece5ab14cacec9a60
>> ---
>>  gdb/csky-tdep.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/csky-tdep.c b/gdb/csky-tdep.c
>> index e86f79a42eaf..ad0d50d8218d 100644
>> --- a/gdb/csky-tdep.c
>> +++ b/gdb/csky-tdep.c
>> @@ -431,19 +431,22 @@ csky_get_supported_register_by_index (int index)
>>      {
>>        case 0: /* Bank1.  */
>>  	{
>> -	  sprintf (tdesc_reg.name, "cp1cr%d", remain);
>> +	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp1cr%d",
>> +		     remain);
> 
> Rather than having to include the size of all these buffers, where the
> size is known at compile time, I wondered if we could add something
> like:
> 
>  template<size_t N, typename... Args>
>  int xsnprintf (char (&buf)[N], const char *format, Args &&...args)
>  {
>    return xsnprintf (buf, N, format, std::forward<Args> (args)...);
>  }
> 
> to gdbsupport/common-utils.h.  This is fine except that gcc is unable to
> track the format literal through the template call, so I think we'd
> actually have to do:
> 
>  template<size_t N, typename... Args>
>  int xsnprintf (char (&buf)[N], const char *format, Args &&...args)
>  {
>    DIAGNOSTIC_PUSH
>    DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
>    return xsnprintf (buf, N, format, std::forward<Args> (args)...);
>    DIAGNOSTIC_POP
>  }
> 
> Which isn't ideal, though we do already have things like this in
> gdb/printcmd.c, so maybe it's OK.
> 
> The other option would be C varargs style handling:
> 
>  template<size_t N>
>  int ATTRIBUTE_PRINTF (2, 3)
>  xsnprintf (char (&buf)[N], const char *format, ...)
>  {
>    va_list args;
>    va_start (args, format);
>    int ret = vsnprintf (buf, N, format, args);
>    gdb_assert (ret < static_cast<int> (N));
>    va_end (args);
>    return ret;
>  }
> 
> Or similar.  The benefit of this would be that you could then write:
> 
>  xsnprintf (tdesc_reg.name, "cp1cr%d", remain);
> 
> And you'd still get the buffer length check.
> 
> Anyway, it was just a thought, not a requirement.  The patch as it is
> looks fine.
> 
> Approved-By: Andrew Burgess <[email protected]>

Thanks the the suggestion, I will attempt to do this on top of the
current series.

Simon