Improve overflow detection in gdbserver

Message ID 90c408285a59434982d00986635c8da9@HQMAIL108.nvidia.com
State New
Headers show
Series
  • Improve overflow detection in gdbserver
Related show

Commit Message

ekurzinger@nvidia.com May 22, 2018, 11:26 p.m.
Hi GDB Team,

Currently, the function used by gdbserver to parse integers from
received commands will detect overflow and fail for any value over
0xfffffff. Among other things, this has the effect of limiting the
file offsets for reading or writing to about 268MB which can be
insufficient for particularly large libraries.

This change allows the parsing of integers up to the true maximum
positive value of 0x7fffffff, increasing the file size limit to
about 2GB.

Note that I don't currently have a copyright assignment form on file,
but your contributor guidlines state that one is not required for
minor changes, so I was hoping this would qualify.

Also, just wanted to say I really appreciate the work you folks do on
this awesome tool, and am glad to be able to make a contribution
(however small it may be)!

Cheers,
Erik

Comments

Pedro Alves May 23, 2018, 11:36 a.m. | #1
On 05/23/2018 12:26 AM, ekurzinger@nvidia.com wrote:
> Hi GDB Team,


Hi!

> 

> Currently, the function used by gdbserver to parse integers from

> received commands will detect overflow and fail for any value over

> 0xfffffff. Among other things, this has the effect of limiting the

> file offsets for reading or writing to about 268MB which can be

> insufficient for particularly large libraries.

> 

> This change allows the parsing of integers up to the true maximum

> positive value of 0x7fffffff, increasing the file size limit to

> about 2GB.


I don't see why we can't we parse offsets into a LONGEST (64-bit)
instead of int (32-bit).  AFAICT, GDB uses long, in gdb/remote-fileio.c:

 static int
 remote_fileio_extract_int (char **buf, long *retint)
 {

and even that should use LONGEST because long is not 64-bit on all
hosts.

Might be a good idea to merge that gdb code and gdbserver's,
say, put shared integer extract routines in gdb/common/fileio.c.

Anyway, your patch is certainly good progress!

> 

> Note that I don't currently have a copyright assignment form on file,

> but your contributor guidlines state that one is not required for

> minor changes, so I was hoping this would qualify.


I think it does.  I'd be delighted if you guys filed one, though.  :-)

> 

> Also, just wanted to say I really appreciate the work you folks do on

> this awesome tool, and am glad to be able to make a contribution

> (however small it may be)!


Thanks!

Merged, as below.

From 81e25b7c91efcc3ff54605b11859375a5c885c8b Mon Sep 17 00:00:00 2001
From: Erik Kurzinger <ekurzinger@nvidia.com>

Date: Wed, 23 May 2018 12:04:39 +0100
Subject: [PATCH] Improve File I/O overflow detection in gdbserver (PR
 server/23198)

Currently, the function used by gdbserver to parse integers from
received File I/O commands will detect overflow and fail for any value
over 0xfffffff.  Among other things, this has the effect of limiting
the file offsets for reading or writing to about 268MB which can be
insufficient for particularly large libraries.

This change allows the parsing of integers up to the true maximum
positive value of 0x7fffffff, increasing the file size limit to about
2GB.

gdb/gdbserver/ChangeLog:
2018-05-23  Erik Kurzinger  <ekurzinger@nvidia.com>

	PR server/23198
	* hostio.c (require_int): Do not report overflow for integers
	between 0xfffffff and 0x7fffffff.
---
 gdb/gdbserver/ChangeLog |  6 ++++++
 gdb/gdbserver/hostio.c  | 13 +++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 54a07b8f30c..7aa59467ac3 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2018-05-23  Erik Kurzinger  <ekurzinger@nvidia.com>
+
+	PR server/23198
+	* hostio.c (require_int): Do not report overflow for integers
+	between 0xfffffff and 0x7fffffff.
+
 2018-05-22  Maciej W. Rozycki  <macro@mips.com>
 
 	* linux-mips-low.c [HAVE_PTRACE_GETREGS] (mips_collect_register)
diff --git a/gdb/gdbserver/hostio.c b/gdb/gdbserver/hostio.c
index d2b5a71bade..c621edfef56 100644
--- a/gdb/gdbserver/hostio.c
+++ b/gdb/gdbserver/hostio.c
@@ -96,22 +96,27 @@ static int
 require_int (char **pp, int *value)
 {
   char *p;
-  int count;
+  int count, firstdigit;
 
   p = *pp;
   *value = 0;
   count = 0;
+  firstdigit = -1;
 
   while (*p && *p != ',')
     {
       int nib;
 
-      /* Don't allow overflow.  */
-      if (count >= 7)
+      if (safe_fromhex (p[0], &nib))
 	return -1;
 
-      if (safe_fromhex (p[0], &nib))
+      if (firstdigit == -1)
+	firstdigit = nib;
+
+      /* Don't allow overflow.  */
+      if (count >= 8 || (count == 7 && firstdigit >= 0x8))
 	return -1;
+
       *value = *value * 16 + nib;
       p++;
       count++;
-- 
2.14.3

Patch

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 5e7ea108b5..58a5f2c30c 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,7 @@ 
+2018-05-18  Erik Kurzinger  <ekurzinger@nvidia.com>
+	* hostio.c (require_int): do not report overflow
+	for integers between 0xfffffff and 0x7fffffff
+
 2018-05-10  Joel Brobecker  <brobecker@adacore.com>
 
 	* lynx-i386-low.c (LYNXOS_178): New macro.
diff --git a/gdb/gdbserver/hostio.c b/gdb/gdbserver/hostio.c
index d2b5a71bad..c621edfef5 100644
--- a/gdb/gdbserver/hostio.c
+++ b/gdb/gdbserver/hostio.c
@@ -96,22 +96,27 @@  static int
 require_int (char **pp, int *value)
 {
   char *p;
-  int count;
+  int count, firstdigit;
 
   p = *pp;
   *value = 0;
   count = 0;
+  firstdigit = -1;
 
   while (*p && *p != ',')
     {
       int nib;
 
-      /* Don't allow overflow.  */
-      if (count >= 7)
+      if (safe_fromhex (p[0], &nib))
 	return -1;
 
-      if (safe_fromhex (p[0], &nib))
+      if (firstdigit == -1)
+	firstdigit = nib;
+
+      /* Don't allow overflow.  */
+      if (count >= 8 || (count == 7 && firstdigit >= 0x8))
 	return -1;
+
       *value = *value * 16 + nib;
       p++;
       count++;