[PATCH] sim-utils.c: prevent buffer overflow.

[PATCH] sim-utils.c: prevent buffer overflow.

[Pavel I. Kryukov]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0001-sim-utils.c-prevent-buffer-overflow.patch --]
[-- Type: application/octet-stream, Size: 1519 bytes --]

From 5958a57053a755030e930c63168f09ca8fab1c84 Mon Sep 17 00:00:00 2001
From: "Pavel I. Kryukov" <kryukov@frtk.ru>
Date: Sun, 1 Dec 2019 01:40:21 +0300
Subject: [PATCH] sim-utils.c: prevent buffer overflow.

Representation of max 32-bit integer is 10 chars.
The potential issue is observed by GCC 7 targeted to AArch64.

sim/common/ChangeLog:
2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>

	* sim-utils.c: Prevent buffer overflow.
---
 sim/common/ChangeLog   | 4 ++++
 sim/common/sim-utils.c | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
index a7ec5c7..12d900e 100644
--- a/sim/common/ChangeLog
+++ b/sim/common/ChangeLog
@@ -1,3 +1,7 @@
+2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
+
+	* sim-utils.c: Prevent buffer overflow.
+
 2019-09-23  Dimitar Dimitrov  <dimitar@dinux.eu>
 
 	* gennltvals.sh: Add PRU libgloss target.
diff --git a/sim/common/sim-utils.c b/sim/common/sim-utils.c
index e83a2e4..a60dd92 100644
--- a/sim/common/sim-utils.c
+++ b/sim/common/sim-utils.c
@@ -355,7 +355,7 @@ map_to_str (unsigned map)
     case io_map: return "io";
     default:
       {
-	static char str[10];
+	static char str[16];
 	sprintf (str, "(%ld)", (long) map);
 	return str;
       }
@@ -385,7 +385,7 @@ access_to_str (unsigned access)
     case access_read_write_exec_io: return "read_write_exec_io";
     default:
       {
-	static char str[10];
+	static char str[16];
 	sprintf (str, "(%ld)", (long) access);
 	return str;
       }
-- 
2.7.4

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Simon Marchi]

On 2019-11-30 5:53 p.m., Pavel I. Kryukov wrote:

Is the sim built in C++ like GDB?  If so, maybe this function could return
an std::string, so there's no more concerns of allocating a static buffer
with sufficient space?

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Павел Крюков]

No, simulators are built in C. Probably they may be switched to C++, but that would require more effort; plus, there may be external C simulators.

Thanks,
--
Pavel

> 1 дек. 2019 г., в 17:46, Simon Marchi <simark@simark.ca> написал(а):
> 
> On 2019-11-30 5:53 p.m., Pavel I. Kryukov wrote:
> 
> Is the sim built in C++ like GDB?  If so, maybe this function could return
> an std::string, so there's no more concerns of allocating a static buffer
> with sufficient space?

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Andrew Burgess]

* Pavel I. Kryukov <kryukov@frtk.ru> [2019-12-01 01:53:59 +0300]:

> 

> From 5958a57053a755030e930c63168f09ca8fab1c84 Mon Sep 17 00:00:00 2001
> From: "Pavel I. Kryukov" <kryukov@frtk.ru>
> Date: Sun, 1 Dec 2019 01:40:21 +0300
> Subject: [PATCH] sim-utils.c: prevent buffer overflow.
> 
> Representation of max 32-bit integer is 10 chars.
> The potential issue is observed by GCC 7 targeted to AArch64.
> 
> sim/common/ChangeLog:
> 2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
> 
>         * sim-utils.c: Prevent buffer overflow.

Approved with the nit below fixed.

Thanks,
Andrew

> ---
>  sim/common/ChangeLog   | 4 ++++
>  sim/common/sim-utils.c | 4 ++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
> index a7ec5c7..12d900e 100644
> --- a/sim/common/ChangeLog
> +++ b/sim/common/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
> +
> +       * sim-utils.c: Prevent buffer overflow.
> +
>  2019-09-23  Dimitar Dimitrov  <dimitar@dinux.eu>
>  
>         * gennltvals.sh: Add PRU libgloss target.
> diff --git a/sim/common/sim-utils.c b/sim/common/sim-utils.c
> index e83a2e4..a60dd92 100644
> --- a/sim/common/sim-utils.c
> +++ b/sim/common/sim-utils.c
> @@ -355,7 +355,7 @@ map_to_str (unsigned map)
>      case io_map: return "io";
>      default:
>        {
> -       static char str[10];
> +       static char str[16];
>         sprintf (str, "(%ld)", (long) map);

I think you could/should change this call to sprintf to use
xsnprintf instead.

>         return str;
>        }
> @@ -385,7 +385,7 @@ access_to_str (unsigned access)
>      case access_read_write_exec_io: return "read_write_exec_io";
>      default:
>        {
> -       static char str[10];
> +       static char str[16];
>         sprintf (str, "(%ld)", (long) access);

Same.

>         return str;
>        }

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Pavel I. Kryukov]

> > diff --git a/sim/common/sim-utils.c b/sim/common/sim-utils.c
> > index e83a2e4..a60dd92 100644
> > --- a/sim/common/sim-utils.c
> > +++ b/sim/common/sim-utils.c
> > @@ -355,7 +355,7 @@ map_to_str (unsigned map)
> >      case io_map: return "io";
> >      default:
> >        {
> > -       static char str[10];
> > +       static char str[16];
> >         sprintf (str, "(%ld)", (long) map);
>
> I think you could/should change this call to sprintf to use
> xsnprintf instead.

If I understand correctly, xsnprintf is defined in GDB header
(gdb/gdbsupport/common-utils.h) which is C++.

Can we use "pure" snprintf instead?

Thanks,
--
Pavel

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Pavel I. Kryukov]

[-- Attachment #1: Type: text/plain, Size: 39 bytes --]

Attached new version

Thanks,
--
Pavel

[-- Attachment #2: 0001-sim-utils.c-prevent-buffer-overflow.patch --]
[-- Type: application/octet-stream, Size: 1644 bytes --]

From 2d6383b7baa715d65191f0f6818ecdd8f5e8fc7d Mon Sep 17 00:00:00 2001
From: "Pavel I. Kryukov" <kryukov@frtk.ru>
Date: Sun, 1 Dec 2019 01:40:21 +0300
Subject: [PATCH] sim-utils.c: prevent buffer overflow.

Representation of max 32-bit integer is 10 chars.
The potential issue is observed by GCC 7 targeted to AArch64.

sim/common/ChangeLog:
2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>

	* sim-utils.c: Prevent buffer overflow.
---
 sim/common/ChangeLog   | 4 ++++
 sim/common/sim-utils.c | 8 ++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
index a7ec5c7..12d900e 100644
--- a/sim/common/ChangeLog
+++ b/sim/common/ChangeLog
@@ -1,3 +1,7 @@
+2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
+
+	* sim-utils.c: Prevent buffer overflow.
+
 2019-09-23  Dimitar Dimitrov  <dimitar@dinux.eu>
 
 	* gennltvals.sh: Add PRU libgloss target.
diff --git a/sim/common/sim-utils.c b/sim/common/sim-utils.c
index e83a2e4..0c46662 100644
--- a/sim/common/sim-utils.c
+++ b/sim/common/sim-utils.c
@@ -355,8 +355,8 @@ map_to_str (unsigned map)
     case io_map: return "io";
     default:
       {
-	static char str[10];
-	sprintf (str, "(%ld)", (long) map);
+	static char str[16];
+	snprintf (str, sizeof(str), "(%ld)", (long) map);
 	return str;
       }
     }
@@ -385,8 +385,8 @@ access_to_str (unsigned access)
     case access_read_write_exec_io: return "read_write_exec_io";
     default:
       {
-	static char str[10];
-	sprintf (str, "(%ld)", (long) access);
+	static char str[16];
+	snprintf (str, sizeof(str), "(%ld)", (long) access);
 	return str;
       }
     }
-- 
2.7.4

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Andrew Burgess]

* Pavel I. Kryukov <kryukov@frtk.ru> [2019-12-02 22:06:04 +0300]:

> From 2d6383b7baa715d65191f0f6818ecdd8f5e8fc7d Mon Sep 17 00:00:00 2001
> From: "Pavel I. Kryukov" <kryukov@frtk.ru>
> Date: Sun, 1 Dec 2019 01:40:21 +0300
> Subject: [PATCH] sim-utils.c: prevent buffer overflow.
> 
> Representation of max 32-bit integer is 10 chars.
> The potential issue is observed by GCC 7 targeted to AArch64.
> 
> sim/common/ChangeLog:
> 2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
> 
>         * sim-utils.c: Prevent buffer overflow.

Approved.

Sorry about the xsnprintf confusion - I'd assumed that came from
libiberty, my mistake.

Thanks,
Andrew



> ---
>  sim/common/ChangeLog   | 4 ++++
>  sim/common/sim-utils.c | 8 ++++----
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
> index a7ec5c7..12d900e 100644
> --- a/sim/common/ChangeLog
> +++ b/sim/common/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
> +
> +	* sim-utils.c: Prevent buffer overflow.
> +
>  2019-09-23  Dimitar Dimitrov  <dimitar@dinux.eu>
> 
>         * gennltvals.sh: Add PRU libgloss target.
> diff --git a/sim/common/sim-utils.c b/sim/common/sim-utils.c
> index e83a2e4..0c46662 100644
> --- a/sim/common/sim-utils.c
> +++ b/sim/common/sim-utils.c
> @@ -355,8 +355,8 @@ map_to_str (unsigned map)
>      case io_map: return "io";
>      default:
>        {
> -	static char str[10];
> -	sprintf (str, "(%ld)", (long) map);
> +	static char str[16];
> +	snprintf (str, sizeof(str), "(%ld)", (long) map);
>         return str;
>        }
>      }
> @@ -385,8 +385,8 @@ access_to_str (unsigned access)
>      case access_read_write_exec_io: return "read_write_exec_io";
>      default:
>        {
> -	static char str[10];
> -	sprintf (str, "(%ld)", (long) access);
> +	static char str[16];
> +	snprintf (str, sizeof(str), "(%ld)", (long) access);
>         return str;
>        }
>      }
> -- 
> 2.7.4

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Pavel I. Kryukov]

Thanks. Could you please tell if there is any action required from me?

--
Pavel

вт, 3 дек. 2019 г. в 00:20, Andrew Burgess <andrew.burgess@embecosm.com>:
>
> * Pavel I. Kryukov <kryukov@frtk.ru> [2019-12-02 22:06:04 +0300]:
>
> > From 2d6383b7baa715d65191f0f6818ecdd8f5e8fc7d Mon Sep 17 00:00:00 2001
> > From: "Pavel I. Kryukov" <kryukov@frtk.ru>
> > Date: Sun, 1 Dec 2019 01:40:21 +0300
> > Subject: [PATCH] sim-utils.c: prevent buffer overflow.
> >
> > Representation of max 32-bit integer is 10 chars.
> > The potential issue is observed by GCC 7 targeted to AArch64.
> >
> > sim/common/ChangeLog:
> > 2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
> >
> >         * sim-utils.c: Prevent buffer overflow.
>
> Approved.
>
> Sorry about the xsnprintf confusion - I'd assumed that came from
> libiberty, my mistake.
>
> Thanks,
> Andrew
>
>
>
> > ---
> >  sim/common/ChangeLog   | 4 ++++
> >  sim/common/sim-utils.c | 8 ++++----
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
> > index a7ec5c7..12d900e 100644
> > --- a/sim/common/ChangeLog
> > +++ b/sim/common/ChangeLog
> > @@ -1,3 +1,7 @@
> > +2019-12-01  Pavel I. Kryukov  <kryukov@frtk.ru>
> > +
> > +     * sim-utils.c: Prevent buffer overflow.
> > +
> >  2019-09-23  Dimitar Dimitrov  <dimitar@dinux.eu>
> >
> >         * gennltvals.sh: Add PRU libgloss target.
> > diff --git a/sim/common/sim-utils.c b/sim/common/sim-utils.c
> > index e83a2e4..0c46662 100644
> > --- a/sim/common/sim-utils.c
> > +++ b/sim/common/sim-utils.c
> > @@ -355,8 +355,8 @@ map_to_str (unsigned map)
> >      case io_map: return "io";
> >      default:
> >        {
> > -     static char str[10];
> > -     sprintf (str, "(%ld)", (long) map);
> > +     static char str[16];
> > +     snprintf (str, sizeof(str), "(%ld)", (long) map);
> >         return str;
> >        }
> >      }
> > @@ -385,8 +385,8 @@ access_to_str (unsigned access)
> >      case access_read_write_exec_io: return "read_write_exec_io";
> >      default:
> >        {
> > -     static char str[10];
> > -     sprintf (str, "(%ld)", (long) access);
> > +     static char str[16];
> > +     snprintf (str, sizeof(str), "(%ld)", (long) access);
> >         return str;
> >        }
> >      }
> > --
> > 2.7.4
>

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Tom Tromey]

>>>>> "Pavel" == Pavel I Kryukov <kryukov@frtk.ru> writes:

Pavel> Thanks. Could you please tell if there is any action required from me?

I've pushed it.

I didn't check to see if you have a copyright assignment.
This patch doesn't need one, but if you plan to do more work on gdb, you
should get that set up.  You can contact me off-list for details.

thanks,
Tom

Re: [PATCH] sim-utils.c: prevent buffer overflow.

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On 01 Dec 2019 18:33, Павел Крюков wrote:
> 1 дек. 2019 г., в 17:46, Simon Marchi <simark@simark.ca> написал(а):
> > Is the sim built in C++ like GDB?  If so, maybe this function could return
> > an std::string, so there's no more concerns of allocating a static buffer
> > with sufficient space?
>
> No, simulators are built in C. Probably they may be switched to C++, but that would require more effort; plus, there may be external C simulators.

i've been thinking about the sim & C-vs-C++.  the only reason i
hesitate to go all in is that we provide libsim.a for people to
link the runtime into other programs, and providing a static lib
that requires C++ feels wrong.

gdb has the advantage that it doesn't provide a library interface.
maybe it should ...
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]