From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id 1F2103858D39 for ; Thu, 26 Sep 2024 05:49:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1F2103858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gnu.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 1F2103858D39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:470:142:3::10 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727329764; cv=none; b=Ekuk5sfvz8QWz+sgI3Es5CXPIHwj1xI8gH6eXGqiExent0Vcxz1X0ER9blhpr4LDWa/PydaleNqU3yRBdq41eZ3nl6aZyKRiOwNrntDIzkgwwiKywM0R7HVtM5UgE1g16LxKc5BazDhDtAMuw96i0XADfVhKmcoc0+AJtuOOWYA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727329764; c=relaxed/simple; bh=7HLkh8TLXR9aHFRZz69TPt/JlsFBMO3umEobT4gfV2E=; h=DKIM-Signature:Date:Message-Id:From:To:Subject; b=Uh3j4d9x9Ufy2FTD1IA1FglI1hnGbPH2c+azfBxt4TAhNjDD5mg2yWSw/kpxSrWdJ77nGOUNjcF0q7CSDLb/zMoyN00tAhgNOdRSM0dSBAYNgBCqTKVjYLulpXBWgCItCLKjQiIymc1cwWUwqfRM4oFB6O3RB6MUspTLi9AG8Ss= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sthNP-0000SF-Fb; Thu, 26 Sep 2024 01:49:19 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=IHl3TMpbBj496KxnVS7/sM66+5Q6pAdg4tTuNeONB6M=; b=NPhxUwVj11Ug Dd9JhwYgvarpj21E0ak5T6fkNJpc+T5ZEbX7VESxVLdokos8JOXiX+GjEGX0Km46x8DVYtwJh+EbB TlBHd0CR7YYm6/ryFMzPCQVmHJt3D3RyFo0sGTJh61XUFggtAA3R0d2iujlHidZ4qUE5D+ipmxQ44 TwzKTxyvQ2OKLuz3n8FmN4JwaqiKGAXpb+sOvvtGDNadvyO0Y9Ucc9HEF9UodI2kGgydYY3G1Ah0o vxV6/dbuujlrzfzWw7drbV2weL2xb7uV7UnMIoQhFaOZJBcyG9uE9reKW/32CqKVXstu05wAWQx31 k9YALuuWDcTyvsuADnC1dA==; Date: Thu, 26 Sep 2024 08:49:17 +0300 Message-Id: <86msjvars2.fsf@gnu.org> From: Eli Zaretskii To: Guinevere Larsen Cc: gdb-patches@sourceware.org In-Reply-To: <20240925175340.1850969-1-guinevere@redhat.com> (message from Guinevere Larsen on Wed, 25 Sep 2024 14:53:40 -0300) Subject: Re: [PATCH] gdb, configure: Add disable-formats option for configure References: <20240925175340.1850969-1-guinevere@redhat.com> X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_PASS,SPF_PASS,TXREP 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: > From: Guinevere Larsen > Cc: Guinevere Larsen > Date: Wed, 25 Sep 2024 14:53:40 -0300 > > GDB has support for many file formats, some which might be very unlikely > to be found in some situations (such as the COFF format in linux). This > commit introduces the option for a user to choose which formats GDB will > support at build configuration time. > > This is especially useful to avoid possible security concerns with > readers that aren't expected to be used at all, as they are one of > the simplest vectors for an attacker to try and hit GDB with. This > change also can reduce the size of the final binary, if that is a > concern. > > This commit adds a switch to the configure script allowing a user to > only enable selected file formats. The default behavior when the switch > is omitted is to compile all file formats, keeping the original behavior > of the script. I always thought that the reason we compile into GDB all the available readers is to enable remote debugging via gdbserver. If I'm right, then using this option you propose will produce GDB that is unable to remote-debug targets which use the omitted formats. This fact should at least be prominently explained in the documentation, because users might not realize they shoot themselves in the foot. I might be confused about this aspect, though; see below. > When enabling selected readers, the configure script will also look at > the selected targets and may choose to add some readers that are the > default - or only - format available for the target. If that happens, > the script will emit the following warning: > > FOO is required to support one or more targets requested. Adding it > > Users aren't able to force the disabling of those formats, since tdep > files may directly call functions from the required readers. I think you only consider targets for native and cross-debugging here; see above. > +# All files that relate to GDB's ability to read debug information. > +# Used with --enable-formats=all. > +ALL_FORMAT_OBS = \ > + coff-pe-read.o \ > + coffread.o \ > + dbxread.o \ > + mipsread.o \ > + xcoffread.o What about elfread.o, mdebugread, and machoread.o? > diff --git a/gdb/NEWS b/gdb/NEWS > index cfc9cb05f77..8d127558a1d 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -92,6 +92,17 @@ vFile:stat > vFile:fstat but takes a filename rather than an open file > descriptor. > > +* Configure changes > + > +enable-formats=[FORMAT,]... > +enable-formats=all > + A user can now decide to only compile support for certain file formats. I think "file format" is too generic a term to be a good name for this option. I think something like "objfile format" or maybe "binary file format", while being longer, are more focused, and thus better. > + The available formats at this point are: dbx, coff, xcoff, elf, mach-o > + and mips. Some targets require specific file formats to be available, > + and in such cases, the configure script will warn the user and add > + support anyway. By default, all formats will be compiled in, to > + continue the behavior from before adding the switch. This part is otherwise okay English-wise, but see my basic concern above regarding remote debugging. > --- a/gdb/README > +++ b/gdb/README > @@ -403,6 +403,11 @@ more obscure GDB `configure' options are not listed here. > specified list of targets. The special value `all' configures > GDB for debugging programs running on any target it supports. > > +`--enable-formats=FORMAT,FORMAT,...' > +`--enable-formats=all` ^^^^^^^^^^^^^^^^^^^^^^ Please don't quote `like this`: it's a markdown-style quoting we don't use in our documentation. > + --enable-formats=FILE_FORMATS > + enable support for selected file formats(default ^^ Space missing there. > +# File formats that will be enabled based on the selected > +# target(s). These are chosen because most, if not all, executables for ^^ GNU standards use US English convention of leaving 2 spaces between sentences. > + # Decide which file formats are absolutely required based on > + # the requested targets. Warn later that they are added, in > + # case the user manually requested them, or requested all. > + # It's fine to add xcoff multiple times since the loop that > + # adds it to FORMAT_OBS will ensure that it is only added once. > + echo $i > + case "${i}" in > + *"windows-tdep.o" ) target_formats="${target_formats} coff";; > + "linux-tdep.o" ) target_formats="${target_formats} elf";; > + "rs6000-aix-tdep.o" ) target_formats="${target_formats} xcoff";; > + "rs6000-lynx178-tdep.o" ) target_formats="${target_formats} xcoff";; > + "mips-tdep.o" ) target_formats="${target_formats} mips";; > + esac This list seems to imply that only non-ELF targets should be in it (but then why linux-tdep.o is in the list?), because otherwise the list is way too short; there are a lot more *-tdep.o files that are built for various platforms, just look what "ls *-tdep.c" produces. If indeed this mentions only non-ELF targets, that should be mentioned in the comment. Also, this misses at least i386-go32-tdep.o (which needs coff). > + # Formats that are required by some requested target(s). > + # Warn users that they are added, so no one is surprised. > + for req in $target_formats; do > + if ! echo "$enable_formats" | grep -wq "$req"; then > + echo "$req is required to support one or more targets requested. Adding it" ^^ Two spaces are needed there. > + # Despite the naming convention implying coff-pe to be a separate > + # reader, it is in fact just a helper for coffread; > + coff) FORMAT_OBS="$FORMAT_OBS coffread.o coff-pe-read.o" ;; But the DJGPP (a.k.a. "go32") native target needs only coff, it doesn't need coff-pe. > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -41177,6 +41177,13 @@ Configure @value{GDBN} for cross-debugging programs running on the > specified list of targets. The special value @samp{all} configures > @value{GDBN} for debugging programs running on any target it supports. > > +@item --enable-formats=@r{[}@var{format}@r{]}@dots{} > +@itemx --enable-formats=all See above about the too-generic name of the option. > +Configure @value{GDBN} to support certain binary file formats. If a ^^^^^^^^^^^^^^^^^^^ Here you use the correct, much more focused, terminology, but IMO the configure option should also use it. > +format is the main (or only) file format for a given target, the What is a "given target" in this context? This should be explained, because it is not clear from the surrounding context. And see below regarding the general confusion this "targets" business causes. > +configure script may add support to it anyway, and warn the user. > +If not given, all file formats that @value{GDBN} supports are compiled. ^^^^^^^^ I'd say "compiled in". And I'm a bit confused by this whole issue and its relation to "GDB targets". The documentation of --target and --enable-targets options to the configure script says: '--target=TARGET' Configure GDB for cross-debugging programs running on the specified TARGET. Without this option, GDB is configured to debug programs that run on the same machine (HOST) as GDB itself. [...] '--enable-targets=[TARGET]...' '--enable-targets=all' Configure GDB for cross-debugging programs running on the specified list of targets. The special value 'all' configures GDB for debugging programs running on any target it supports. First, this doesn't say what is the default if --enable-targets is not specified; I think we should add that. More importantly, it says "cross-debugging", not "remote debugging", and my reading of configure.ac matches that: this option affects the value of TARGET_OBS, which is the list of target-specific files needed for debugging by GDB itself, without the help of gdbserver. So using the value of --enable-targets= for the purpose of deciding which readers will be compiled into GDB seems to be wrong, because when a target is debugged remotely via gdbserver, it is my understanding that the reader of the target's binary file format is needed in GDB itself, no? Maybe I'm confused, but if I am, it means we lack some important information in the manual which would clarify this. Thanks. Reviewed-By: Eli Zaretskii