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

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,
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