From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 297AE3858030 for ; Thu, 21 Dec 2023 19:42:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 297AE3858030 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 297AE3858030 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703187745; cv=none; b=ULfgyHj0SP6Wz1pnZuJ4TzUoO1RDXlGdshBf4ku29wQNHjcuzawRZ5tKq10DdRVkA8qUYQXluUAuB3NV+5Q+grwCdrF0H0VvCh51TlMaHLwR4JwhoF5IdGkvbNm+K6hoqB2ekM9HOtJGb+SqqVM8IRpmZN6A9DhD9f23uXbpwIw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703187745; c=relaxed/simple; bh=4zUhkh3choykzUZrhuTOrW1lvVbwXhKSfxtI+z2Q9tA=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=TRTZ6b8Jrkb98eOvOstcGDSYJMeIRLvtU7BTu4uMPlK8F/6WuYRWgpO+S9hb8iHaG7alCFj5W6rOlOba/3+uRqKrELLyx/4HS3i4fyOf0d5J0hETxUVEBnTj+PvsysNmbY/NzuaNE2P5FIAUdR9LpMkpV8BQiz2YF1efkJzSw/c= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1703187742; bh=4zUhkh3choykzUZrhuTOrW1lvVbwXhKSfxtI+z2Q9tA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=kdd1cxX6waPkt42GLMLkEJm7fvwxUQ0+ZWaBp8eQFD/fgM7tW1j7Tv/HmkyIE2v4A hLqP1SP1d8G/QwtbdA+vnj/dCmMN5ti9Qw8f51E9RQEMgc3/uMhHDkqyrTXhxl7fP/ 82NCUC8puIsWfOiwV7DbIBOg0T7DCXcnSmh1nwLc= Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8A0761E0AC; Thu, 21 Dec 2023 14:42:22 -0500 (EST) Message-ID: <477a743c-b318-4089-bfbc-0035686b4cad@simark.ca> Date: Thu, 21 Dec 2023 14:42:21 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] [gdbserver] Fix overflow detection in gdbserver Content-Language: fr To: Kirill Radkin , "gdb-patches@sourceware.org" Cc: Konstantin Vladimirov , Ivan Tetyushkin References: From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 12/21/23 03:38, Kirill Radkin wrote: > Hi, > > > 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. Hi, Thanks for the patch. First, the boring stuff: I think that the patch is more than an obvious fix, so we'll need to make sure you are covered with an FSF copyright assignment [1]. Do you know if you (or your employer, since it looks like you're doing this as part of your employment) are covered with one already? [1] https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment Then, I am unable to apply your patch with git-am. It looks like you copy pasted the patch in your email client, which usually gives bad results. Are you able to use git-send-email [2]? This is the most trustworthy way. [2] https://sourceware.org/gdb/wiki/ContributionChecklist#Submitting_patches Some comments below. > From 01117d757a0eb4f632aded4b15a06dbcccc7adf7 Mon Sep 17 00:00:00 2001 > From: Kirill Radkin > Date: Mon, 13 Nov 2023 16:27:15 +0300 > Subject: [PATCH] gdbserver: Fix overflow detection in gdbserver > > Currently gdbserver use require_int() function to > parse offset which we get, for example, in vFile::pread > packet. This function allows integers up to 0x7fffffff, > (to fit in 32-bit int) but actually offset (for pread > system call) have a 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 | 47 +++++++++++++++++++ > gdbserver/hostio.cc | 16 +++++-- > 3 files changed, 85 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..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. Just "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 . */ > + > +.text > +.globl_start > +_start: > +.skip 3742415472 Does this create a sparse file on systems that support it? I could see it as a problem for some test environments if we create and leave a file occupying ~4GB file in the build directory. Well, two actually, there will be the .o and the final executable. Note that I don't have a better idea on how to test this... > + ret > +.globlf > +.typef, @function > +f: > + ret Do you expect this .S file to work for all architectures? It's simple, but I guess that the "ret" instruction doesn't exist on all architectures. Can you think of a way to produce a big binary that is not architecture-dependent at all? Maybe produce an executable, and then insert a large dummy section in the beginning? > 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..a4b648b29fc > --- /dev/null > +++ b/gdb/testsuite/gdb.server/pread-offset-size.exp > @@ -0,0 +1,47 @@ > +# Copyright (C) 2023-2023 Free Software Foundation, Inc. Just "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 . > + Please add a short comment that explains the intention of this test. > +load_lib gdbserver-support.exp > + > +if {[skip_gdbserver_tests]} { > + return > +} > + > +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 ea70c26da0f..068771428f9 100644 > --- a/gdbserver/hostio.cc > +++ b/gdbserver/hostio.cc > @@ -90,12 +90,16 @@ require_filename (char **pp, char *filename) > return 0; > } > > +template > static int > -require_int (char **pp, int *value) > +require_int (char **pp, 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; > @@ -112,7 +116,9 @@ require_int (char **pp, int *value) > firstdigit = nib; > > /* Don't allow overflow. */ > - if (count >= 8 || (count == 7 && firstdigit >= 0x8)) > + if (count >= max_count || (static_cast(-1) < 0 > +&& count == (max_count - 1) > +&& firstdigit >= 0x8)) > return -1; I guess that the static_cast thing is to determine if the type is signed or not? Can you give it a name, for clarity, like (maybe there's a better way, just an example): constexpr bool is_signed = static_cast (-1) < 0; Simon