* [PATCH] gdbserver: Update require_int function to parse offset for pread packet
@ 2025-04-21 11:59 Kirill Radkin
2025-04-23 23:15 ` Kevin Buettner
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Radkin @ 2025-04-21 11:59 UTC (permalink / raw)
To: gdb-patches; +Cc: Kirill Radkin
Currently gdbserver uses require_int() function to parse the requested offset
(in vFile::pread packet and the like). This function allows integers up to
0x7fffffff (to fit in 32-bit int), however the offset (for pread system call)
has an off_t type which can be larger than 32-bit.
This patch allows require_int() function to parse offset up to the maximum
value implied by the off_t type.
---
gdb/testsuite/gdb.server/pread-offset-size.S | 26 +++++++++++
.../gdb.server/pread-offset-size.exp | 45 +++++++++++++++++++
gdbserver/hostio.cc | 18 +++++---
3 files changed, 84 insertions(+), 5 deletions(-)
create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.S
create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.exp
diff --git a/gdb/testsuite/gdb.server/pread-offset-size.S b/gdb/testsuite/gdb.server/pread-offset-size.S
new file mode 100644
index 00000000000..31748090ac3
--- /dev/null
+++ b/gdb/testsuite/gdb.server/pread-offset-size.S
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2023-2023 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+ .text
+ .globl _start
+_start:
+ .skip 3742415472
+ ret
+ .globl f
+ .type f, @function
+f:
+ ret
diff --git a/gdb/testsuite/gdb.server/pread-offset-size.exp b/gdb/testsuite/gdb.server/pread-offset-size.exp
new file mode 100644
index 00000000000..c6acaa382b2
--- /dev/null
+++ b/gdb/testsuite/gdb.server/pread-offset-size.exp
@@ -0,0 +1,45 @@
+# Copyright (C) 2023-2023 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+load_lib gdbserver-support.exp
+
+require allow_gdbserver_tests
+
+standard_testfile .S
+
+if { [prepare_for_testing ${testfile}.exp $testfile \
+ $srcfile {debug additional_flags=-nostdlib} ] } {
+ return -1
+}
+
+gdb_exit
+gdb_start
+
+gdb_test_no_output "set remote exec-file $binfile" \
+"set remote exec-file"
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+set res [gdbserver_spawn ""]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+gdb_test "target $gdbserver_protocol $gdbserver_gdbport" \
+"Remote debugging using .*" \
+"target $gdbserver_protocol $gdbserver_gdbport"
+
+gdb_test "break f" "Breakpoint 1.*"
diff --git a/gdbserver/hostio.cc b/gdbserver/hostio.cc
index 17b6179d8ca..2207804670b 100644
--- a/gdbserver/hostio.cc
+++ b/gdbserver/hostio.cc
@@ -89,12 +89,18 @@ require_filename (char **pp, char *filename)
return 0;
}
+template <typename T>
static int
-require_int (char **pp, int *value)
+require_int (char **pp, T *value)
{
+ constexpr bool is_signed = std::is_signed<T>::value;
+
char *p;
int count, firstdigit;
+ /* Max count of hexadecimal digits in off_t (1 hex digit is 4 bits) */
+ int max_count = sizeof(T) * CHAR_BIT / 4;
+
p = *pp;
*value = 0;
count = 0;
@@ -111,9 +117,9 @@ require_int (char **pp, int *value)
firstdigit = nib;
/* Don't allow overflow. */
- if (count >= 8 || (count == 7 && firstdigit >= 0x8))
+ if (count >= max_count
+ || (is_signed && count == (max_count - 1) && firstdigit >= 0x8))
return -1;
-
*value = *value * 16 + nib;
p++;
count++;
@@ -343,7 +349,8 @@ handle_open (char *own_buf)
static void
handle_pread (char *own_buf, int *new_packet_len)
{
- int fd, ret, len, offset, bytes_sent;
+ int fd, ret, len, bytes_sent;
+ off_t offset;
char *p, *data;
static int max_reply_size = -1;
@@ -410,7 +417,8 @@ handle_pread (char *own_buf, int *new_packet_len)
static void
handle_pwrite (char *own_buf, int packet_len)
{
- int fd, ret, len, offset;
+ int fd, ret, len;
+ off_t offset;
char *p, *data;
p = own_buf + strlen ("vFile:pwrite:");
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] gdbserver: Update require_int function to parse offset for pread packet
2025-04-21 11:59 [PATCH] gdbserver: Update require_int function to parse offset for pread packet Kirill Radkin
@ 2025-04-23 23:15 ` Kevin Buettner
2025-05-28 11:35 ` Kirill Radkin
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Buettner @ 2025-04-23 23:15 UTC (permalink / raw)
To: Kirill Radkin; +Cc: gdb-patches
Hi Kirill,
Just a few nits...
On Mon, 21 Apr 2025 14:59:41 +0300
Kirill Radkin <kirill.radkin@syntacore.com> wrote:
> Currently gdbserver uses require_int() function to parse the requested offset
> (in vFile::pread packet and the like). This function allows integers up to
> 0x7fffffff (to fit in 32-bit int), however the offset (for pread system call)
> has an off_t type which can be larger than 32-bit.
>
> This patch allows require_int() function to parse offset up to the maximum
> value implied by the off_t type.
> ---
> gdb/testsuite/gdb.server/pread-offset-size.S | 26 +++++++++++
> .../gdb.server/pread-offset-size.exp | 45 +++++++++++++++++++
> gdbserver/hostio.cc | 18 +++++---
> 3 files changed, 84 insertions(+), 5 deletions(-)
> create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.S
> create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.exp
>
> diff --git a/gdb/testsuite/gdb.server/pread-offset-size.S b/gdb/testsuite/gdb.server/pread-offset-size.S
> new file mode 100644
> index 00000000000..31748090ac3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/pread-offset-size.S
> @@ -0,0 +1,26 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2023-2023 Free Software Foundation, Inc.
Please update the second 2023 to 2025.
(Do you even need the first 2023?)
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> + .text
> + .globl _start
> +_start:
> + .skip 3742415472
> + ret
> + .globl f
> + .type f, @function
> +f:
> + ret
> diff --git a/gdb/testsuite/gdb.server/pread-offset-size.exp b/gdb/testsuite/gdb.server/pread-offset-size.exp
> new file mode 100644
> index 00000000000..c6acaa382b2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/pread-offset-size.exp
> @@ -0,0 +1,45 @@
> +# Copyright (C) 2023-2023 Free Software Foundation, Inc.
Likewise.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +load_lib gdbserver-support.exp
> +
> +require allow_gdbserver_tests
> +
> +standard_testfile .S
> +
> +if { [prepare_for_testing ${testfile}.exp $testfile \
> + $srcfile {debug additional_flags=-nostdlib} ] } {
git am complained of a whitespace problem here. Basically, it
wants you to use a tab instead of the leading 8 spaces.
> + return -1
> +}
> +
> +gdb_exit
> +gdb_start
> +
> +gdb_test_no_output "set remote exec-file $binfile" \
> +"set remote exec-file"
> +
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect" ".*"
> +
> +set res [gdbserver_spawn ""]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +gdb_test "target $gdbserver_protocol $gdbserver_gdbport" \
> +"Remote debugging using .*" \
> +"target $gdbserver_protocol $gdbserver_gdbport"
> +
> +gdb_test "break f" "Breakpoint 1.*"
> diff --git a/gdbserver/hostio.cc b/gdbserver/hostio.cc
> index 17b6179d8ca..2207804670b 100644
> --- a/gdbserver/hostio.cc
> +++ b/gdbserver/hostio.cc
> @@ -89,12 +89,18 @@ require_filename (char **pp, char *filename)
> return 0;
> }
>
> +template <typename T>
> static int
> -require_int (char **pp, int *value)
> +require_int (char **pp, T *value)
> {
> + constexpr bool is_signed = std::is_signed<T>::value;
> +
> char *p;
> int count, firstdigit;
>
> + /* Max count of hexadecimal digits in off_t (1 hex digit is 4 bits) */
Please place a period (.) at the end of the comment and make sure
that it's followed by 2 spaces.
> + int max_count = sizeof(T) * CHAR_BIT / 4;
> +
> p = *pp;
> *value = 0;
> count = 0;
> @@ -111,9 +117,9 @@ require_int (char **pp, int *value)
> firstdigit = nib;
>
> /* Don't allow overflow. */
> - if (count >= 8 || (count == 7 && firstdigit >= 0x8))
> + if (count >= max_count
> + || (is_signed && count == (max_count - 1) && firstdigit >= 0x8))
> return -1;
> -
> *value = *value * 16 + nib;
> p++;
> count++;
> @@ -343,7 +349,8 @@ handle_open (char *own_buf)
> static void
> handle_pread (char *own_buf, int *new_packet_len)
> {
> - int fd, ret, len, offset, bytes_sent;
> + int fd, ret, len, bytes_sent;
> + off_t offset;
> char *p, *data;
> static int max_reply_size = -1;
>
> @@ -410,7 +417,8 @@ handle_pread (char *own_buf, int *new_packet_len)
> static void
> handle_pwrite (char *own_buf, int packet_len)
> {
> - int fd, ret, len, offset;
> + int fd, ret, len;
> + off_t offset;
> char *p, *data;
>
> p = own_buf + strlen ("vFile:pwrite:");
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] gdbserver: Update require_int function to parse offset for pread packet
2025-04-23 23:15 ` Kevin Buettner
@ 2025-05-28 11:35 ` Kirill Radkin
2025-05-28 12:24 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Radkin @ 2025-05-28 11:35 UTC (permalink / raw)
To: kirill.radkin; +Cc: gdb-patches
Currently gdbserver uses require_int() function to parse the requested offset
(in vFile::pread packet and the like). This function allows integers up to
0x7fffffff (to fit in 32-bit int), however the offset (for pread system call)
has an off_t type which can be larger than 32-bit.
This patch allows require_int() function to parse offset up to the maximum
value implied by the off_t type.
---
gdb/testsuite/gdb.server/pread-offset-size.S | 26 +++++++++++
.../gdb.server/pread-offset-size.exp | 45 +++++++++++++++++++
gdbserver/hostio.cc | 18 +++++---
3 files changed, 84 insertions(+), 5 deletions(-)
create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.S
create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.exp
diff --git a/gdb/testsuite/gdb.server/pread-offset-size.S b/gdb/testsuite/gdb.server/pread-offset-size.S
new file mode 100644
index 00000000000..f43e059653d
--- /dev/null
+++ b/gdb/testsuite/gdb.server/pread-offset-size.S
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2025 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+ .text
+ .globl _start
+_start:
+ .skip 3742415472
+ ret
+ .globl f
+ .type f, @function
+f:
+ ret
diff --git a/gdb/testsuite/gdb.server/pread-offset-size.exp b/gdb/testsuite/gdb.server/pread-offset-size.exp
new file mode 100644
index 00000000000..5faf2162ef0
--- /dev/null
+++ b/gdb/testsuite/gdb.server/pread-offset-size.exp
@@ -0,0 +1,45 @@
+# Copyright (C) 2025 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+load_lib gdbserver-support.exp
+
+require allow_gdbserver_tests
+
+standard_testfile .S
+
+if { [prepare_for_testing ${testfile}.exp $testfile \
+ $srcfile {debug additional_flags=-nostdlib} ] } {
+ return -1
+}
+
+gdb_exit
+gdb_start
+
+gdb_test_no_output "set remote exec-file $binfile" \
+"set remote exec-file"
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+set res [gdbserver_spawn ""]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+gdb_test "target $gdbserver_protocol $gdbserver_gdbport" \
+"Remote debugging using .*" \
+"target $gdbserver_protocol $gdbserver_gdbport"
+
+gdb_test "break f" "Breakpoint 1.*"
diff --git a/gdbserver/hostio.cc b/gdbserver/hostio.cc
index 17b6179d8ca..e88f8d79728 100644
--- a/gdbserver/hostio.cc
+++ b/gdbserver/hostio.cc
@@ -89,12 +89,18 @@ require_filename (char **pp, char *filename)
return 0;
}
+template <typename T>
static int
-require_int (char **pp, int *value)
+require_int (char **pp, T *value)
{
+ constexpr bool is_signed = std::is_signed<T>::value;
+
char *p;
int count, firstdigit;
+ /* Max count of hexadecimal digits in off_t (1 hex digit is 4 bits). */
+ int max_count = sizeof(T) * CHAR_BIT / 4;
+
p = *pp;
*value = 0;
count = 0;
@@ -111,9 +117,9 @@ require_int (char **pp, int *value)
firstdigit = nib;
/* Don't allow overflow. */
- if (count >= 8 || (count == 7 && firstdigit >= 0x8))
+ if (count >= max_count
+ || (is_signed && count == (max_count - 1) && firstdigit >= 0x8))
return -1;
-
*value = *value * 16 + nib;
p++;
count++;
@@ -343,7 +349,8 @@ handle_open (char *own_buf)
static void
handle_pread (char *own_buf, int *new_packet_len)
{
- int fd, ret, len, offset, bytes_sent;
+ int fd, ret, len, bytes_sent;
+ off_t offset;
char *p, *data;
static int max_reply_size = -1;
@@ -410,7 +417,8 @@ handle_pread (char *own_buf, int *new_packet_len)
static void
handle_pwrite (char *own_buf, int packet_len)
{
- int fd, ret, len, offset;
+ int fd, ret, len;
+ off_t offset;
char *p, *data;
p = own_buf + strlen ("vFile:pwrite:");
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] gdbserver: Update require_int function to parse offset for pread packet
2025-05-28 11:35 ` Kirill Radkin
@ 2025-05-28 12:24 ` Pedro Alves
2025-06-03 13:48 ` Kirill Radkin
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2025-05-28 12:24 UTC (permalink / raw)
To: Kirill Radkin; +Cc: gdb-patches
On 2025-05-28 12:35, Kirill Radkin wrote:
> Currently gdbserver uses require_int() function to parse the requested offset
> (in vFile::pread packet and the like). This function allows integers up to
> 0x7fffffff (to fit in 32-bit int), however the offset (for pread system call)
> has an off_t type which can be larger than 32-bit.
>
> This patch allows require_int() function to parse offset up to the maximum
> value implied by the off_t type.
> ---
> gdb/testsuite/gdb.server/pread-offset-size.S | 26 +++++++++++
> .../gdb.server/pread-offset-size.exp | 45 +++++++++++++++++++
> gdbserver/hostio.cc | 18 +++++---
> 3 files changed, 84 insertions(+), 5 deletions(-)
> create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.S
> create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.exp
>
> diff --git a/gdb/testsuite/gdb.server/pread-offset-size.S b/gdb/testsuite/gdb.server/pread-offset-size.S
> new file mode 100644
> index 00000000000..f43e059653d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/pread-offset-size.S
> @@ -0,0 +1,26 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2025 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
Please add a short comment stating what this is trying to do.
> + .text
> + .globl _start
> +_start:
> + .skip 3742415472
Is this particular number significant?
> + ret
> + .globl f
> + .type f, @function
> +f:
> + ret
> diff --git a/gdb/testsuite/gdb.server/pread-offset-size.exp b/gdb/testsuite/gdb.server/pread-offset-size.exp
> new file mode 100644
> index 00000000000..5faf2162ef0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/pread-offset-size.exp
> @@ -0,0 +1,45 @@
> +# Copyright (C) 2025 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
This needs a short blurb here stating what the testcase is testing.
> +load_lib gdbserver-support.exp
> +
> +require allow_gdbserver_tests
> +
> +standard_testfile .S
> +
> +if { [prepare_for_testing ${testfile}.exp $testfile \
> + $srcfile {debug additional_flags=-nostdlib} ] } {
> + return -1
> +}
> +
> +gdb_exit
> +gdb_start
clean_restart
> +
> +gdb_test_no_output "set remote exec-file $binfile" \
> +"set remote exec-file"
Something's off with the indentation of the second line. As in, it's not indented. :-)
Indent it by four spaces, like:
gdb_test_no_output "set remote exec-file $binfile" \
"set remote exec-file"
> +
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect" ".*"
> +
> +set res [gdbserver_spawn ""]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +gdb_test "target $gdbserver_protocol $gdbserver_gdbport" \
> +"Remote debugging using .*" \
> +"target $gdbserver_protocol $gdbserver_gdbport"
Ditto. But also, you'll notice that other tests doing something similar avoid
putting the port number in gdb.sum, so that comparing test results is a bit
more stable. So:
gdb_test "target $gdbserver_protocol $gdbserver_gdbport" \
"Remote debugging using .*" \
"target $gdbserver_protocol"
> +
> +gdb_test "break f" "Breakpoint 1.*"
Is this where the problem would manifest? Some comments would help.
> diff --git a/gdbserver/hostio.cc b/gdbserver/hostio.cc
> index 17b6179d8ca..e88f8d79728 100644
> --- a/gdbserver/hostio.cc
> +++ b/gdbserver/hostio.cc
> @@ -89,12 +89,18 @@ require_filename (char **pp, char *filename)
> return 0;
> }
>
> +template <typename T>
> static int
> -require_int (char **pp, int *value)
> +require_int (char **pp, T *value)
> {
> + constexpr bool is_signed = std::is_signed<T>::value;
> +
> char *p;
> int count, firstdigit;
>
> + /* Max count of hexadecimal digits in off_t (1 hex digit is 4 bits). */
"T", not "off_t".
> + int max_count = sizeof(T) * CHAR_BIT / 4;
Space before parens.
> +
> p = *pp;
> *value = 0;
> count = 0;
> @@ -111,9 +117,9 @@ require_int (char **pp, int *value)
> firstdigit = nib;
>
> /* Don't allow overflow. */
> - if (count >= 8 || (count == 7 && firstdigit >= 0x8))
> + if (count >= max_count
> + || (is_signed && count == (max_count - 1) && firstdigit >= 0x8))
> return -1;
> -
Keep the empty line, so the comment above is clear to refer
to the whole block up to the empty line.
> *value = *value * 16 + nib;
> p++;
> count++;
> @@ -343,7 +349,8 @@ handle_open (char *own_buf)
> static void
> handle_pread (char *own_buf, int *new_packet_len)
> {
> - int fd, ret, len, offset, bytes_sent;
> + int fd, ret, len, bytes_sent;
> + off_t offset;
> char *p, *data;
> static int max_reply_size = -1;
>
> @@ -410,7 +417,8 @@ handle_pread (char *own_buf, int *new_packet_len)
> static void
> handle_pwrite (char *own_buf, int packet_len)
> {
> - int fd, ret, len, offset;
> + int fd, ret, len;
> + off_t offset;
> char *p, *data;
>
> p = own_buf + strlen ("vFile:pwrite:");
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] gdbserver: Update require_int function to parse offset for pread packet
2025-05-28 12:24 ` Pedro Alves
@ 2025-06-03 13:48 ` Kirill Radkin
2025-06-20 14:22 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Kirill Radkin @ 2025-06-03 13:48 UTC (permalink / raw)
To: pedro; +Cc: gdb-patches, kirill.radkin
I've addressed all review comments.
Also, additional information about issue was added to commit message.
---
Recently I have encountered an issue with gdbserver not being able to send debug information from large \
files (> 2Gb). I found that this issue is connected with parsing offsets (integer) from vFile::pread \
packets. There is already a patch (https://sourceware.org/bugzilla/show_bug.cgi?id=23198) that allows to \
parse integers up to 0x7fffffff (32-bit integer), but it doesn't fix the problem at all, because offsets \
can have a off_t type which can be larger than 32-bit integer (it depends on system).
This patch allows require_int() function to parse offset up to the maximum value implied by the off_t \
type.
Currently gdbserver uses require_int() function to parse the requested offset
(in vFile::pread packet and the like). This function allows integers up to
0x7fffffff (to fit in 32-bit int), however the offset (for pread system call)
has an off_t type which can be larger than 32-bit.
This patch allows require_int() function to parse offset up to the maximum
value implied by the off_t type.
---
gdb/testsuite/gdb.server/pread-offset-size.S | 29 +++++++++++
.../gdb.server/pread-offset-size.exp | 50 +++++++++++++++++++
gdbserver/hostio.cc | 17 +++++--
3 files changed, 92 insertions(+), 4 deletions(-)
create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.S
create mode 100644 gdb/testsuite/gdb.server/pread-offset-size.exp
diff --git a/gdb/testsuite/gdb.server/pread-offset-size.S b/gdb/testsuite/gdb.server/pread-offset-size.S
new file mode 100644
index 00000000000..63b2faa213d
--- /dev/null
+++ b/gdb/testsuite/gdb.server/pread-offset-size.S
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2025 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+/* Here we are trying to create a large binary (> 2 GB),
+ 3742415472 bytes is about 3,5 gigabytes. */
+
+ .text
+ .globl _start
+_start:
+ .skip 3742415472
+ ret
+ .globl f
+ .type f, @function
+f:
+ ret
diff --git a/gdb/testsuite/gdb.server/pread-offset-size.exp b/gdb/testsuite/gdb.server/pread-offset-size.exp
new file mode 100644
index 00000000000..e224e971ad5
--- /dev/null
+++ b/gdb/testsuite/gdb.server/pread-offset-size.exp
@@ -0,0 +1,50 @@
+# Copyright (C) 2025 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# Testing fix of a problem with gdbserver not being able to send
+# debug information from large files (> 2GB)
+
+load_lib gdbserver-support.exp
+
+require allow_gdbserver_tests
+
+standard_testfile .S
+
+if { [prepare_for_testing ${testfile}.exp $testfile \
+ $srcfile {debug additional_flags=-nostdlib} ] } {
+ return -1
+}
+
+clean_restart
+
+gdb_test_no_output "set remote exec-file $binfile" \
+ "set remote exec-file"
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+set res [gdbserver_spawn ""]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+gdb_test "target $gdbserver_protocol $gdbserver_gdbport" \
+ "Remote debugging using .*" \
+ "target $gdbserver_protocol"
+
+# If load of a large binary was successful,
+# we should be able to place a break on f
+
+gdb_test "break f" "Breakpoint 1.*"
diff --git a/gdbserver/hostio.cc b/gdbserver/hostio.cc
index 17b6179d8ca..4e77253750a 100644
--- a/gdbserver/hostio.cc
+++ b/gdbserver/hostio.cc
@@ -89,12 +89,18 @@ require_filename (char **pp, char *filename)
return 0;
}
+template <typename T>
static int
-require_int (char **pp, int *value)
+require_int (char **pp, T *value)
{
+ constexpr bool is_signed = std::is_signed<T>::value;
+
char *p;
int count, firstdigit;
+ /* Max count of hexadecimal digits in T (1 hex digit is 4 bits). */
+ int max_count = sizeof (T) * CHAR_BIT / 4;
+
p = *pp;
*value = 0;
count = 0;
@@ -111,7 +117,8 @@ require_int (char **pp, int *value)
firstdigit = nib;
/* Don't allow overflow. */
- if (count >= 8 || (count == 7 && firstdigit >= 0x8))
+ if (count >= max_count
+ || (is_signed && count == (max_count - 1) && firstdigit >= 0x8))
return -1;
*value = *value * 16 + nib;
@@ -343,7 +350,8 @@ handle_open (char *own_buf)
static void
handle_pread (char *own_buf, int *new_packet_len)
{
- int fd, ret, len, offset, bytes_sent;
+ int fd, ret, len, bytes_sent;
+ off_t offset;
char *p, *data;
static int max_reply_size = -1;
@@ -410,7 +418,8 @@ handle_pread (char *own_buf, int *new_packet_len)
static void
handle_pwrite (char *own_buf, int packet_len)
{
- int fd, ret, len, offset;
+ int fd, ret, len;
+ off_t offset;
char *p, *data;
p = own_buf + strlen ("vFile:pwrite:");
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-06-20 14:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-21 11:59 [PATCH] gdbserver: Update require_int function to parse offset for pread packet Kirill Radkin
2025-04-23 23:15 ` Kevin Buettner
2025-05-28 11:35 ` Kirill Radkin
2025-05-28 12:24 ` Pedro Alves
2025-06-03 13:48 ` Kirill Radkin
2025-06-20 14:22 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox