Re: [PATCH] gdbserver: Update require_int function to parse offset for pread packet

[Pedro Alves <pedro@palves.net>]

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:");