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 BFF203858D35 for ; Tue, 23 May 2023 12:31:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BFF203858D35 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.170] (unknown [167.248.160.41]) (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 B40D01E0D6; Tue, 23 May 2023 08:31:30 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1684845091; bh=Cc6L4OpBon8R4PCIxwQ6MPfq95VZmGzygWMeS3gtyLc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=X9rh1JC9OHNBBGGRUZQE49JvXee2aWpD8zlJ7qCuS8eeQz+PssLdFlOboUxr3nPYv btu77wB0o006Lam3NusCPXYbpm9rbZ2U94FypkcC5fjTY3x6+VvvIJ0+icIza8qLIi AxLF5KZG5UONosFat14kwmoPcDiIpSYFEmLdSrMs= Message-ID: Date: Tue, 23 May 2023 08:31:30 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH] gdb/debuginfod: cleanup debuginfod earlier Content-Language: fr To: Andrew Burgess , gdb-patches@sourceware.org Cc: Aaron Merey , Mark Wielaard References: <46c030dcd5fc7a1a2ef4e078a92798f18c947ace.1684840908.git.aburgess@redhat.com> From: Simon Marchi In-Reply-To: <46c030dcd5fc7a1a2ef4e078a92798f18c947ace.1684840908.git.aburgess@redhat.com> 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,NICE_REPLY_A,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 5/23/23 07:23, Andrew Burgess via Gdb-patches wrote: > A GDB crash was discovered on Fedora GDB that was tracked back to an > issue with the way that debuginfod is cleaned up. > > The bug was reported on Fedora 37, 38, and 39. Here are the steps to > reproduce: > > 1. The file /etc/ssl/openssl.cnf contains the following lines: > > [provider_sect] > default = default_sect > ##legacy = legacy_sect > ## > [default_sect] > activate = 1 > > ##[legacy_sect] > ##activate = 1 > > The bug will occur when the '##' characters are removed so that the > lines in question look like this: > > [provider_sect] > default = default_sect > legacy = legacy_sect > > [default_sect] > activate = 1 > > [legacy_sect] > activate = 1 > > 2. Clean up any existing debuginfod cache data: > > > rm -rf $HOME/.cache/debuginfod_client > > 3. Run GDB: > > > gdb -nx -q -iex 'set trace-commands on' \ > -iex 'set debuginfod enabled on' \ > -iex 'set confirm off' \ > -ex 'start' -ex 'quit' /bin/ls > +set debuginfod enabled on > +set confirm off > Reading symbols from /bin/ls... > Downloading separate debug info for /usr/bin/ls > ... snip ... > Temporary breakpoint 1, main (argc=1, argv=0x7fffffffde38) at ../src/ls.c:1646 > 1646 { > +quit > > Fatal signal: Segmentation fault > ----- Backtrace ----- > ... snip ... > > So GDB ends up crashing during exit. > > What's happening is that when debuginfod is initialised > debuginfod_begin is called (this is in the debuginfod library), this > in turn sets up libcurl, which makes use of openssl. Somewhere during > this setup process an at_exit function is registered to cleanup some > state. > > Back in GDB the debuginfod_client object is managed using this code: > > /* Deleter for a debuginfod_client. */ > > struct debuginfod_client_deleter > { > void operator() (debuginfod_client *c) > { > debuginfod_end (c); > } > }; > > using debuginfod_client_up > = std::unique_ptr; > > And then a global debuginfod_client_up is created to hold a pointer to > the debuginfod_client object. As a global this will be cleaned up > using the standard C++ global object destructor mechanism, which is > run after the at_exit handlers. > > However, it is expected that when debuginfod_end is called the > debuginfod_client object will still be in a usable state, that is, we > don't expect the at_exit handlers to have run and started cleaning up > the library state. > > To fix this issue we need to ensure that debuginfod_end is called > before the at_exit handlers have a chance to run. > > I propose that we should make use of GDB's make_final_cleanup > mechanism to call debuginfod_end. Or rather, I intend to continue > using the debuginfod_client_up class, but ensure that the global > instance of this class is set back to nullptr before GDB calls exit. > Setting debuginfod_client_up to nullptr will invoke the deleter, which > will call debuginfod_end. > > For the implementation I've created a new class which holds the > debuginfod_client_up we can then pass a pointer to this class to > make_final_cleanup. The alternative, passing a pointer to a > unique_ptr made me feel really uncomfortable -- we'd then have > multiple references to the unique_ptr, which seems like a bad idea. > > There's no test associated with this patch. I have no idea how I > might trigger this bug from within the testsuite. If anyone has any > ideas then I'm happy to have a go at writing something. > > None of the existing debuginfod tests fail after this change. > > Co-Authored-By: Mark Wielaard I was able to reproduce on Arch Linux using the same openssl configuration. Using the final cleanup mechanism sounds fine to me. But if we're not going to manage the lifetime of the object with the destructor, I think that there is no point in having a debuginfod_client_up, it just adds an additional and unnecessary layer to run the same code (we can just call debuginfod_end where you would reset the unique pointer). Also, I think the debuginfod_client_manager class is unnecessary and just adds some more unnecessary layers, making the code hard to follow for no benefit. We can just make the existing get_debuginfod_client function set up the final cleanup handler, as follow: diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index 5853f420a18d..4ab650ef2494 100644 --- a/gdb/debuginfod-support.c +++ b/gdb/debuginfod-support.c @@ -96,20 +96,6 @@ struct user_data ui_out::progress_update progress; }; -/* Deleter for a debuginfod_client. */ - -struct debuginfod_client_deleter -{ - void operator() (debuginfod_client *c) - { - debuginfod_end (c); - } -}; - -using debuginfod_client_up - = std::unique_ptr; - - /* Convert SIZE into a unit suitable for use with progress updates. SIZE should in given in bytes and will be converted into KB, MB, GB or remain unchanged. UNIT will be set to "B", "KB", "MB" or "GB" @@ -180,20 +166,32 @@ progressfn (debuginfod_client *c, long cur, long total) return 0; } +static void cleanup_debuginfod_client (void *arg) +{ + debuginfod_client *client = static_cast (arg); + debuginfod_end (client); +} + +/* Return a pointer to the single global debuginfod_client, initialising it + first if needed. */ + static debuginfod_client * get_debuginfod_client () { - static debuginfod_client_up global_client; + static debuginfod_client *global_client; if (global_client == nullptr) { - global_client.reset (debuginfod_begin ()); + global_client = debuginfod_begin (); if (global_client != nullptr) - debuginfod_set_progressfn (global_client.get (), progressfn); + { + make_final_cleanup (cleanup_debuginfod_client, global_client); + debuginfod_set_progressfn (global_client, progressfn); + } } - return global_client.get (); + return global_client; } /* Check if debuginfod is enabled. If configured to do so, ask the user --- As for the test I don't see any other way of truly testing this than running the regular tests on the affected systems. Simon