[PATCH] gdb/nat/linux-osdata.c: fix build on gcc-12 (string overfow)

[PATCH] gdb/nat/linux-osdata.c: fix build on gcc-12 (string overfow)

[Sergei Trofimovich]

From: Sergei Trofimovich <siarheit@google.com>

On gcc-12 build fails as:

    gdbserver/../gdb/nat/linux-osdata.c: In function 'void linux_xfer_osdata_processes(buffer*)':
    gdbserver/../gdb/nat/linux-osdata.c:330:39: error:
      '__builtin___sprintf_chk' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
      330 |                 sprintf (core_str, "%d", i);
          |                                       ^

It's an off-by-one case in an unlinely scenario for negative
huge core count. The change assumes smalles negative value to
include sign.
---
 gdb/nat/linux-osdata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index 9746d1210fe..18b6f40cb78 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -320,12 +320,12 @@ linux_xfer_osdata_processes (struct buffer *buffer)
 	  /* Find CPU cores used by the process.  */
 	  cores = XCNEWVEC (int, num_cores);
 	  task_count = get_cores_used_by_process (pid, cores, num_cores);
-	  cores_str = (char *) xcalloc (task_count, sizeof ("4294967295") + 1);
+	  cores_str = (char *) xcalloc (task_count, sizeof ("-2147483648") + 1);
 
 	  for (i = 0; i < num_cores && task_count > 0; ++i)
 	    if (cores[i])
 	      {
-		char core_str[sizeof ("4294967295")];
+		char core_str[sizeof ("-2147483648")];
 
 		sprintf (core_str, "%d", i);
 		strcat (cores_str, core_str);
-- 
2.33.1

Re: [PATCH] gdb/nat/linux-osdata.c: fix build on gcc-12 (string overfow)

[Simon Marchi]

On 2021-11-14 12:32, Sergei Trofimovich via Gdb-patches wrote:
> From: Sergei Trofimovich <siarheit@google.com>
> 
> On gcc-12 build fails as:
> 
>     gdbserver/../gdb/nat/linux-osdata.c: In function 'void linux_xfer_osdata_processes(buffer*)':
>     gdbserver/../gdb/nat/linux-osdata.c:330:39: error:
>       '__builtin___sprintf_chk' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
>       330 |                 sprintf (core_str, "%d", i);
>           |                                       ^
> 
> It's an off-by-one case in an unlinely scenario for negative
> huge core count. The change assumes smalles negative value to
> include sign.
> ---
>  gdb/nat/linux-osdata.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
> index 9746d1210fe..18b6f40cb78 100644
> --- a/gdb/nat/linux-osdata.c
> +++ b/gdb/nat/linux-osdata.c
> @@ -320,12 +320,12 @@ linux_xfer_osdata_processes (struct buffer *buffer)
>  	  /* Find CPU cores used by the process.  */
>  	  cores = XCNEWVEC (int, num_cores);
>  	  task_count = get_cores_used_by_process (pid, cores, num_cores);
> -	  cores_str = (char *) xcalloc (task_count, sizeof ("4294967295") + 1);
> +	  cores_str = (char *) xcalloc (task_count, sizeof ("-2147483648") + 1);
>  
>  	  for (i = 0; i < num_cores && task_count > 0; ++i)
>  	    if (cores[i])
>  	      {
> -		char core_str[sizeof ("4294967295")];
> +		char core_str[sizeof ("-2147483648")];
>  
>  		sprintf (core_str, "%d", i);
>  		strcat (cores_str, core_str);
> 

The problem could also be avoided by using %u and making i unsigned.

But actually, would you mind updating this code so that cores_str is an
std::string and you just append to it?  That would get rid of the manual
memory management.  I'm expecting that it could just look like this (but
I haven't tested):

	  std::string cores_str;
	  for (i = 0; i < num_cores && task_count > 0; ++i)
	    if (cores[i])
	      {
		cores_str += string_printf ("%d", i);

		task_count -= cores[i];
		if (task_count > 0)
		  cores_str += ',';
	      }

Simon

Re: [PATCH] gdb/nat/linux-osdata.c: fix build on gcc-12 (string overfow)

[Pedro Alves]

On 2021-11-15 00:30, Simon Marchi via Binutils wrote:

> But actually, would you mind updating this code so that cores_str is an
> std::string and you just append to it?  That would get rid of the manual
> memory management.  I'm expecting that it could just look like this (but
> I haven't tested):
> 
> 	  std::string cores_str;
> 	  for (i = 0; i < num_cores && task_count > 0; ++i)
> 	    if (cores[i])
> 	      {
> 		cores_str += string_printf ("%d", i);

You can use string_appendf instead to save allocating/destroying a new string:

 		 string_printf (cores_str, "%d", i);

Pedro Alves

> 
> 		task_count -= cores[i];
> 		if (task_count > 0)
> 		  cores_str += ',';
> 	      }
> 
> Simon
>