diff mbox series

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

Message ID 20260817151646.152571-8-simon.marchi@efficios.com
State New
Headers
Series Fix various warnings when building on macOS |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply

Commit Message

Simon Marchi Aug. 17, 2026, 3:16 p.m. UTC 
  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(-)

Comments

Andrew Burgess Aug. 17, 2026, 4:26 p.m. UTC | #1 
Simon Marchi <simon.marchi@efficios.com> 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 <aburgess@redhat.com>

Thanks,
Andrew



>  	  tdesc_reg.num = 189 + remain;
>  	}
>  	break;
>        case 1: /* Bank2.  */
>  	{
> -	  sprintf (tdesc_reg.name, "cp2cr%d", remain);
> +	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp2cr%d",
> +		     remain);
>  	  tdesc_reg.num = 276 + remain;
>  	}
>  	break;
>        case 2: /* Bank3.  */
>  	{
> -	  sprintf (tdesc_reg.name, "cp3cr%d", remain);
> +	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp3cr%d",
> +		     remain);
>  	  tdesc_reg.num = 221 + remain;
>  	}
>  	break;
> @@ -460,7 +463,8 @@ csky_get_supported_register_by_index (int index)
>        case 13: /* Bank14.  */
>  	{
>  	  /* Regitsers in Bank4~14 have continuous regno with start 308.  */
> -	  sprintf (tdesc_reg.name, "cp%dcr%d", (multi + 1), remain);
> +	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp%dcr%d",
> +		     (multi + 1), remain);
>  	  tdesc_reg.num = 308 + ((multi - 3) * 32) + remain;
>  	}
>  	break;
> @@ -482,7 +486,8 @@ csky_get_supported_register_by_index (int index)
>        case 29: /* Bank31.  */
>  	{
>  	  /* Regitsers in Bank16~31 have continuous regno with start 660.  */
> -	  sprintf (tdesc_reg.name, "cp%dcr%d", (multi + 2), remain);
> +	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp%dcr%d",
> +		     (multi + 2), remain);
>  	  tdesc_reg.num = 660 + ((multi - 14) * 32) + remain;
>  	}
>  	break;
> -- 
> 2.55.0
Simon Marchi Aug. 17, 2026, 5:03 p.m. UTC | #2 
On 8/17/26 12:26 PM, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> 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 <aburgess@redhat.com>

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

Simon
Tom Tromey Aug. 17, 2026, 8:50 p.m. UTC | #3 
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> to gdbsupport/common-utils.h.  This is fine except that gcc is unable to
Andrew> track the format literal through the template call, so I think we'd
Andrew> actually have to do:

It might work if instead the 'buf' argument were a gdb::array_view,
which I think can be implicitly constructed from an array.

Tom
Simon Marchi Aug. 18, 2026, 6:26 p.m. UTC | #4 
On 8/17/26 4:50 PM, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> 
> Andrew> to gdbsupport/common-utils.h.  This is fine except that gcc is unable to
> Andrew> track the format literal through the template call, so I think we'd
> Andrew> actually have to do:
> 
> It might work if instead the 'buf' argument were a gdb::array_view,
> which I think can be implicitly constructed from an array.

I tried changing xsnprintf to use array_view, but it was too big of a
change.  I did not think about adding a new overload though, I think
that could work.

Simon
diff mbox series

Patch

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);
 	  tdesc_reg.num = 189 + remain;
 	}
 	break;
       case 1: /* Bank2.  */
 	{
-	  sprintf (tdesc_reg.name, "cp2cr%d", remain);
+	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp2cr%d",
+		     remain);
 	  tdesc_reg.num = 276 + remain;
 	}
 	break;
       case 2: /* Bank3.  */
 	{
-	  sprintf (tdesc_reg.name, "cp3cr%d", remain);
+	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp3cr%d",
+		     remain);
 	  tdesc_reg.num = 221 + remain;
 	}
 	break;
@@ -460,7 +463,8 @@  csky_get_supported_register_by_index (int index)
       case 13: /* Bank14.  */
 	{
 	  /* Regitsers in Bank4~14 have continuous regno with start 308.  */
-	  sprintf (tdesc_reg.name, "cp%dcr%d", (multi + 1), remain);
+	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp%dcr%d",
+		     (multi + 1), remain);
 	  tdesc_reg.num = 308 + ((multi - 3) * 32) + remain;
 	}
 	break;
@@ -482,7 +486,8 @@  csky_get_supported_register_by_index (int index)
       case 29: /* Bank31.  */
 	{
 	  /* Regitsers in Bank16~31 have continuous regno with start 660.  */
-	  sprintf (tdesc_reg.name, "cp%dcr%d", (multi + 2), remain);
+	  xsnprintf (tdesc_reg.name, sizeof (tdesc_reg.name), "cp%dcr%d",
+		     (multi + 2), remain);
 	  tdesc_reg.num = 660 + ((multi - 14) * 32) + remain;
 	}
 	break;