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

[Kevin Buettner <kevinb@redhat.com>]

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
> 
>