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 A6281384D158 for ; Wed, 29 Jun 2022 16:38:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A6281384D158 Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 3D6B51E222; Wed, 29 Jun 2022 12:38:34 -0400 (EDT) Message-ID: <54e4257b-cdcc-77bd-fe4c-e96f2c51a688@simark.ca> Date: Wed, 29 Jun 2022 12:38:33 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH] Fix GDBserver regression due to change to avoid reading shell registers Content-Language: fr To: Pedro Alves , Andrew Burgess , gdb-patches@sourceware.org References: <83ceec06f74f4ce6a2dc8d794031fb233567ba6d.1655136816.git.aburgess@redhat.com> <48d211cc-96f3-8c84-5aec-7023054d45ac@palves.net> <87fsjqdqco.fsf@redhat.com> <6db0b4b8-25fb-8f4c-382e-d45f6edcff17@palves.net> <87a69xdqfo.fsf@redhat.com> <87ea7153-6f53-06f9-3aa7-4ce90031b3e8@simark.ca> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jun 2022 16:38:36 -0000 On 6/29/22 12:22, Pedro Alves wrote: > On 2022-06-29 16:17, Simon Marchi wrote: > >> This introduces the following ASan crash for me: > > ... > >> It looks like in that non-existing-program case, the process gets deleted before >> the starting_up flag gets restored to false. > > Thanks. The patch below fixes it. I've also run the whole testsuite with > --target_board=native-extended-gdbserver, on x86-64 Ubuntu 20.04, with Asan, > and saw no regressions. > > From 5ceb44cdf51b764193bea229c80e55ebb0d9c55c Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Wed, 29 Jun 2022 16:38:43 +0100 > Subject: [PATCH] Fix GDBserver regression due to change to avoid reading shell > registers > > Simon reported that the recent change to make GDB and GDBserver avoid > reading shell registers caused a GDBserver regression, caught with > ASan while running gdb.server/non-existing-program.exp: > > $ /home/smarchi/build/binutils-gdb/gdb/testsuite/../../gdb/../gdbserver/gdbserver stdio non-existing-program > ================================================================= > ==127719==ERROR: AddressSanitizer: heap-use-after-free on address 0x60f0000000e9 at pc 0x55bcbfa301f4 bp 0x7ffd238a7320 sp 0x7ffd238a7310 > WRITE of size 1 at 0x60f0000000e9 thread T0 > #0 0x55bcbfa301f3 in scoped_restore_tmpl::~scoped_restore_tmpl() /home/smarchi/src/binutils-gdb/gdbserver/../gdbsupport/scoped_restore.h:86 > #1 0x55bcbfa2ffe9 in post_fork_inferior(int, char const*) /home/smarchi/src/binutils-gdb/gdbserver/fork-child.cc:120 > #2 0x55bcbf9c9199 in linux_process_target::create_inferior(char const*, std::__debug::vector > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:991 > #3 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941 > #4 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084 > #5 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2) > #6 0x55bcbf8ef2bd in _start (/home/smarchi/build/binutils-gdb/gdbserver/gdbserver+0x1352bd) > > 0x60f0000000e9 is located 169 bytes inside of 176-byte region [0x60f000000040,0x60f0000000f0) > freed by thread T0 here: > #0 0x7ff9d6c6f0c7 in operator delete(void*) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:160 > #1 0x55bcbf910d00 in remove_process(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/inferiors.cc:164 > #2 0x55bcbf9c4ac7 in linux_process_target::remove_linux_process(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:454 > #3 0x55bcbf9cdaa6 in linux_process_target::mourn(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:1599 > #4 0x55bcbf988dc4 in target_mourn_inferior(ptid_t) /home/smarchi/src/binutils-gdb/gdbserver/target.cc:205 > #5 0x55bcbfa32020 in startup_inferior(process_stratum_target*, int, int, target_waitstatus*, ptid_t*) /home/smarchi/src/binutils-gdb/gdbserver/../gdb/nat/fork-inferior.c:515 > #6 0x55bcbfa2fdeb in post_fork_inferior(int, char const*) /home/smarchi/src/binutils-gdb/gdbserver/fork-child.cc:111 > #7 0x55bcbf9c9199 in linux_process_target::create_inferior(char const*, std::__debug::vector > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:991 > #8 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941 > #9 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084 > #10 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2) > > previously allocated by thread T0 here: > #0 0x7ff9d6c6e5a7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99 > #1 0x55bcbf910ad0 in add_process(int, int) /home/smarchi/src/binutils-gdb/gdbserver/inferiors.cc:144 > #2 0x55bcbf9c477d in linux_process_target::add_linux_process_no_mem_file(int, int) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:425 > #3 0x55bcbf9c8f4c in linux_process_target::create_inferior(char const*, std::__debug::vector > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:985 > #4 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941 > #5 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084 > #6 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2) > > Above we see that in the non-existing-program case, the process gets > deleted before the starting_up flag gets restored to false. > > This happens because startup_inferior call target_mourn_inferior > before throwing an error, and in GDBserver, unlike in GDB, mourning > deletes the process. > > Fix this by not using a scoped_restore to manage the starting_up flag, > since we should only clear it when startup_inferior doesn't throw. > > Change-Id: I67325d6f81c64de4e89e20e4ec4556f57eac7f6c > --- > gdbserver/fork-child.cc | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/gdbserver/fork-child.cc b/gdbserver/fork-child.cc > index 7ea66f2a435..078612f7091 100644 > --- a/gdbserver/fork-child.cc > +++ b/gdbserver/fork-child.cc > @@ -105,12 +105,21 @@ post_fork_inferior (int pid, const char *program) > #endif > > process_info *proc = find_process_pid (pid); > - scoped_restore save_starting_up > - = make_scoped_restore (&proc->starting_up, true); > + > + /* If the inferior fails to start, startup_inferior mourns the > + process (which deletes it), and then throws an error. This means > + that on exception return, we don't need or want to clear this > + flag back, as PROC won't exist anymore. Thus, we don't use a > + scoped_restore. */ > + proc->starting_up = true; > > startup_inferior (the_target, pid, > START_INFERIOR_TRAPS_EXPECTED, > &cs.last_status, &cs.last_ptid); > + > + /* If we get here, the process was successfully started. */ > + proc->starting_up = false; > + > current_thread->last_resume_kind = resume_stop; > current_thread->last_status = cs.last_status; > signal_pid = pid; > > base-commit: b955c336f93a1b8ab433b713739c145c56f5c027 > -- > 2.36.0 > Thanks, LGTM. This is the same fix I had imagined. Simon