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 624063854562 for ; Fri, 18 Nov 2022 17:34:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 624063854562 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.11] (unknown [217.28.27.60]) (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 ED0DF1E0D3; Fri, 18 Nov 2022 12:34:23 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1668792864; bh=5jdGkL20dHKBH57w/OXV32kglJDL/CLWw8VsVOZKRHA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=T1Zn8Y5vUk7UWrdHFlgMDf8pZm5SNNF3vUArLz5c4YUQ8aNS6p+oY4rCLrT6yb8UR lQ9b1jLtTmtfuVU4VgMrYYigRwoo2Ev002W2qsXW93BFdHApU6mO5xlu1sBZvkERDj xX8mQ0n2R9bTEsbMCgfKSP+9bKMX1nS4d5pJDZbk= Message-ID: <346d6d47-bb5b-1a88-0cdd-2164175a50e6@simark.ca> Date: Fri, 18 Nov 2022 12:34:23 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH 7/8] gdbserver: switch to right process in find_one_thread Content-Language: en-US To: Andrew Burgess , Simon Marchi via Gdb-patches Cc: Simon Marchi References: <20221117194241.1776125-1-simon.marchi@efficios.com> <20221117194241.1776125-8-simon.marchi@efficios.com> <875yfc8kne.fsf@redhat.com> From: Simon Marchi In-Reply-To: <875yfc8kne.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP,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 List-Id: On 11/18/22 08:19, Andrew Burgess via Gdb-patches wrote: > Simon Marchi via Gdb-patches writes: > >> When I do this: >> >> $ ./gdb -nx --data-directory=data-directory -q \ >> /bin/sleep \ >> -ex "maint set target-non-stop on" \ >> -ex "tar ext :1234" \ >> -ex "set remote exec-file /bin/sleep" \ >> -ex "run 1231 &" \ >> -ex add-inferior \ >> -ex "inferior 2" >> Reading symbols from /bin/sleep... >> (No debugging symbols found in /bin/sleep) >> Remote debugging using :1234 >> Starting program: /bin/sleep 1231 >> Reading /lib64/ld-linux-x86-64.so.2 from remote target... >> warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead. >> Reading /lib64/ld-linux-x86-64.so.2 from remote target... >> Reading /usr/lib/debug/.build-id/a6/7a1408f18db3576757eea210d07ba3fc560dff.debug from remote target... >> [New inferior 2] >> Added inferior 2 on connection 1 (extended-remote :1234) >> [Switching to inferior 2 [] ()] >> (gdb) Reading /lib/x86_64-linux-gnu/libc.so.6 from remote target... >> attach 3659848 >> Attaching to process 3659848 >> /home/smarchi/src/binutils-gdb/gdb/thread.c:85: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed. >> >> The internal error of GDB is actually caused by GDBserver crashing, and >> the error recovery of GDB is not on point. This patch aims to fix just >> the GDBserver crash, not the GDB problem. >> >> GDBserver crashes with a segfault here: >> >> (gdb) bt >> #0 0x00005555557fb3f4 in find_one_thread (ptid=...) at /home/smarchi/src/binutils-gdb/gdbserver/thread-db.cc:177 >> #1 0x00005555557fd5cf in thread_db_thread_handle (ptid=, handle=0x7fffffffc400, handle_len=0x7fffffffc3f0) >> at /home/smarchi/src/binutils-gdb/gdbserver/thread-db.cc:461 >> #2 0x000055555578a0b6 in linux_process_target::thread_handle (this=0x5555558a64c0 , ptid=, handle=0x7fffffffc400, >> handle_len=0x7fffffffc3f0) at /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:6905 >> #3 0x00005555556dfcc6 in handle_qxfer_threads_worker (thread=0x60b000000510, buffer=0x7fffffffc8a0) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:1645 >> #4 0x00005555556e00e6 in operator() (__closure=0x7fffffffc5e0, thread=0x60b000000510) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:1696 >> #5 0x00005555556f54be in for_each_thread >(struct {...}) (func=...) at /home/smarchi/src/binutils-gdb/gdbserver/gdbthread.h:159 >> #6 0x00005555556e0242 in handle_qxfer_threads_proper (buffer=0x7fffffffc8a0) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:1694 >> #7 0x00005555556e04ba in handle_qxfer_threads (annex=0x629000000213 "", readbuf=0x621000019100 '\276' ..., writebuf=0x0, offset=0, len=4097) >> at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:1732 >> #8 0x00005555556e1989 in handle_qxfer (own_buf=0x629000000200 "qXfer:threads", packet_len=26, new_packet_len_p=0x7fffffffd630) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:2045 >> #9 0x00005555556e720a in handle_query (own_buf=0x629000000200 "qXfer:threads", packet_len=26, new_packet_len_p=0x7fffffffd630) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:2685 >> #10 0x00005555556f1a01 in process_serial_event () at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4176 >> #11 0x00005555556f4457 in handle_serial_event (err=0, client_data=0x0) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4514 >> #12 0x0000555555820f56 in handle_file_event (file_ptr=0x607000000250, ready_mask=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573 >> #13 0x0000555555821895 in gdb_wait_for_event (block=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694 >> #14 0x000055555581f533 in gdb_do_one_event (mstimeout=-1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264 >> #15 0x00005555556ec9fb in start_event_loop () at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3512 >> #16 0x00005555556f0769 in captured_main (argc=4, argv=0x7fffffffe0d8) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3992 >> #17 0x00005555556f0e3f in main (argc=4, argv=0x7fffffffe0d8) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4078 >> >> The reason is a wrong current process when find_one_thread is called. >> The current process is the 2nd one, which was just attached. It does >> not yet have thread_db data (proc->priv->thread_db is nullptr). As we >> iterate on all threads of all process to fulfull the qxfer:threads:read >> request, we get to a thread of process 1 for which we haven't read >> thread_db information yet (lwp_info::thread_known is false), so we get >> into find_one_thread. find_one_thread uses >> `current_process ()->priv->thread_db`, assuming the current process >> matches the ptid passed as a parameter, which is wrong. A segfault >> happens when trying to dereference that thread_db pointer. >> >> Fix this by making find_one_thread not assume what the current process / >> current thread is. If it needs to call into libthread_db, which we know >> will try to read memory from the current process, then temporarily set >> the current process. >> >> In the case where the thread is already know and we return early, we >> don't need to switch process. >> >> I hit this case when running the test included with the following patch, >> "gdb: disable commit resumed in target_kill", so the fix is exercised by >> that test. > > I'm not able to reproduce the failure you describe. I've applied this > series except for this patch, and run the test from patch #8 with > native-extended-gdbserver board, and it passes just fine. > > Actually, I do see an issue with the test, but that issue doesn't seem > to be related to this problem, and is present with and without this > patch - I'll reply to patch #8 describing that issue. > > Is this bug intermittent? Or is it likely to depend on certain parts of > the environment? I got the impression from the description that it if > we did the steps in the right order then we'd get a nullptr dereference, > which didn't feel like an intermittent issue, but maybe I don't > understand correctly. The crash reproduces all the time for me with the instructions provided in the commit message, I think it's deterministic. However, I got confused by my own instructions :P. There's an "attach" command buried in there that is easy to miss. It's on that attach that things fail. If I put that attach on the command line, I don't get the crash (more on this later). I looked into it a bit more, it takes a relatively precise context for this to reproduce: - The first process must be far enough to have loaded its libc or libpthread (whatever triggers the loading of libthread_db), such that its proc->priv->thread_db is not nullptr - However, its lwp must still be in the `!lwp->thread_known` state, meaning GDBserver hasn't asked libthread_db to compute the thread handle yet. That means, GDB must not have refreshed the thread list yet, since that would cause the thread handles to be computed. That means, no stopping on a breakpoint, since that causes a thread list update. That's why the first inferior needs to be started with "run &". It hits some internal breakpoints when shared libraries are loaded, GDBserver asks for the symbols necessary to load libthread_db, but GDB never asks for a full thread list. - The attach then causes GDB to ask for a thread list update here: #18 0x000055ac22438062 in remote_target::update_thread_list (this=0x61700003d800) at /home/simark/src/binutils-gdb/gdb/remote.c:3946 #19 0x000055ac224445df in extended_remote_target::attach (this=0x61700003d800, args=0x602000077950 "936019", from_tty=1) at /home/simark/src/binutils-gdb/gdb/remote.c:6166 #20 0x000055ac21e1ffe2 in attach_command (args=0x602000077950 "936019", from_tty=1) at /home/simark/src/binutils-gdb/gdb/infcmd.c:2644 At this point, the current process is the second one (the one we attach to). Since we are early in the attach process, this process' proc->priv->thread_db is still nullptr. In thread_db_thread_handle, while handling a thread from the first process, we determined (correctly) that this thread's process uses thread_db but this thread's handle is not know yet. But then in find_one_thread, we use the current process, the second process, and try to dereference its nullptr proc->priv->thread_db. If I put the attach on the command line, GDB doesn't go back to the event loop between the "run &" and the "attach", so it won't handle events for the first process, not to the qSymbol dance, so the first process' thread_db is still nullptr when doing the attach. And we don't see the bug because thread_db_thread_handle will return early and not call find_one_thread. Ah, I know now why you don't see the crash when running the test without this patch applied. It's because I later modified the test to grab the first process' pid. To do so, it runs to some spot (after a getpid call), reads a variable and then does "continue &". The breakpoint stop causes a thread list update, and breaks the second condition listed above. I will make a dedicated test for this specific bug then and include it in a v2 for this patch. > > That said, the change itself looks reasonable - but it would be nice to > know why I can't reproduce the failure. You're right to ask, it made me look at it more and understand the conditions more clearly. Simon