From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 49A1E3858D37 for ; Tue, 23 May 2023 11:23:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 49A1E3858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1684841012; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=XHabWbRE0M9Q2REIsRYYnOzetlnNvDaZErW2j5BFnZI=; b=K3bTKt1d6xWaRfNS5XMgUyNgydV+Nvwk60qq2VPLx6UVHZcvmKB+NGTyqStui11yHHJU0y R+E5ZnzyB+FScC1W3QlDWc9PMHaapC3Ge+ASpCWD1eSvLc7Kq1Kaj40ZtADDBLOZbHTw/z ztxrflz2kv1pV3qf0GCSBR+BgdHmDy8= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-516-_sigJOL2MnWILcTt_22T5Q-1; Tue, 23 May 2023 07:23:30 -0400 X-MC-Unique: _sigJOL2MnWILcTt_22T5Q-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-3f604260ef3so13737365e9.3 for ; Tue, 23 May 2023 04:23:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684841009; x=1687433009; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=XHabWbRE0M9Q2REIsRYYnOzetlnNvDaZErW2j5BFnZI=; b=fD/ijKDkQl4url5/mh5VcqqsowQwyA904fppQ5SDdf3acR5MTqV1xvU5DjGxucHcAl 4w6CquCNmcCisYXGqvPLy2fLfXQ7hyDMKr520B6P3UiI/IOgjDAZ/LB5bvN8xnqvBACJ NIBKL9Tnk3hbouGRlK+JXP9WnwefTzfeo9g8BTvZhmf5XrzVL6VFVeZvRK4ZvK8YDK9B PNq5A5ZGpq1sEkD5Jp+szfqyHj22r0AKxagR1+gCSnZQWiQvfIHCZK8sT05jYd/BKqPp N+yziZh/DilHI7t85hNoHk7d7dsOzk62PJcPnrs2Hd0N71fRvvtOpaSmXKl8H0JfF8B/ Tksw== X-Gm-Message-State: AC+VfDzula/zQ3drxlNAB/FneifGf7f/W7LB3p9dOWemlZYS9mslqec5 9dZOYi/p7UXyU4QVWybozBTbii+cvOv7NabLbNgOLaHewqC92bFBd3FWZuBsCEdHaak0/3Tc1QE m/+sqrmkBUHfbQgRqrro1OATzG78VH7vciFR56kG6hILnCcrNOVujy6SttWFudvg/dIeiDRpBkD 9T/PeloA== X-Received: by 2002:a7b:c8c4:0:b0:3f6:4c4:d0ce with SMTP id f4-20020a7bc8c4000000b003f604c4d0cemr4581404wml.8.1684841009443; Tue, 23 May 2023 04:23:29 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4z8SzFstN4QfTonPmH8fOx7EgIOvGthzboc0pfCEIX6UVPuWpLt6YaJ/Ntj7g7LmXq2Y3vRQ== X-Received: by 2002:a7b:c8c4:0:b0:3f6:4c4:d0ce with SMTP id f4-20020a7bc8c4000000b003f604c4d0cemr4581377wml.8.1684841009045; Tue, 23 May 2023 04:23:29 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id q21-20020a056000137500b0030732d6e104sm10937183wrz.105.2023.05.23.04.23.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 May 2023 04:23:28 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Aaron Merey , Andrew Burgess , Mark Wielaard Subject: [PATCH] gdb/debuginfod: cleanup debuginfod earlier Date: Tue, 23 May 2023 12:23:25 +0100 Message-Id: <46c030dcd5fc7a1a2ef4e078a92798f18c947ace.1684840908.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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: 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 --- gdb/debuginfod-support.c | 104 +++++++++++++++++++++++++++++++++++---- 1 file changed, 94 insertions(+), 10 deletions(-) diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index 5853f420a18..5dfb313f0af 100644 --- a/gdb/debuginfod-support.c +++ b/gdb/debuginfod-support.c @@ -180,20 +180,104 @@ progressfn (debuginfod_client *c, long cur, long total) return 0; } -static debuginfod_client * -get_debuginfod_client () +/* A class that manages a debuginfod_client object. There should only be a + single debuginfod_client created within GDB, so we should only ever have + a single instance of this class. + + The critical part of this class is that we register a final cleanup with + GDB that is responsible for ensuring the debuginfod_client object is + correctly cleaned up before GDB calls exit. If we rely on the C++ + global object destructors to clean up the debuginfod_client object then + we run into problems with the order in which object destructors are + called relative to at_exit callbacks. See the init method below for + more details. */ + +struct debuginfod_client_manager { - static debuginfod_client_up global_client; + /* Return a pointer to the managed debuginfod_client object, initialising + it first if needed. */ - if (global_client == nullptr) - { - global_client.reset (debuginfod_begin ()); + debuginfod_client *get () + { + if (m_global_client == nullptr) + this->init (); + return m_global_client.get (); + } - if (global_client != nullptr) - debuginfod_set_progressfn (global_client.get (), progressfn); - } + /* There should only be a single, global, instance of this class. The + managed debuginfod_client will be reset back to nullptr by GDB's + make_final_cleanup mechanism before the global object is destroyed, + which is why the assertion here makes sense. */ + + ~debuginfod_client_manager () + { + gdb_assert (m_global_client == nullptr); + } - return global_client.get (); +private: + + /* Callback used by GDB's final cleanup mechanism. Cleans up the + debuginfod_client object being managed by ARG, which should be a + debuginfod_client_manager pointer. */ + + static void cleanup_debuginfod_client (void *arg) + { + debuginfod_client_manager *m = (debuginfod_client_manager *) arg; + m->cleanup (); + } + + /* Set the managed debuginfod_client back to nullptr. This will ensure + that the debuginfod cleanup function is called. */ + + void cleanup () + { + m_global_client = nullptr; + } + + /* Initialise the managed debuginfod_client object and register with + GDB's global cleanup mechanism to ensure that the object is cleaned up + correctly before GDB calls exit. */ + + void init () + { + gdb_assert (m_global_client == nullptr); + + debuginfod_client *client = debuginfod_begin (); + + if (client != nullptr) + { + /* Debuginfod (or the libraries it uses) use at_exit to cleanup + some of their state. However, it is requires that the + debuginfod_client object be cleaned up before the libraries that + debuginfod uses are cleaned up. + + If we rely on the C++ global object destructors to cleanup the + debuginfod object then these are run after the at_exit + callbacks, which is too late. + + So instead, we register with GDB's final cleanup mechanism, this + is run before GDB calls exit, which is before the at_exit + handlers are run, this ensures things are cleaned up in the + correct order. */ + make_final_cleanup (cleanup_debuginfod_client, this); + debuginfod_set_progressfn (client, progressfn); + m_global_client.reset (client); + } + } + + /* The managed debuginfod_client object. */ + + debuginfod_client_up m_global_client; +}; + +/* Return a pointer to the single global debuginfod_client, initialising it + first if needed. */ + +static debuginfod_client * +get_debuginfod_client () +{ + static debuginfod_client_manager global_client_manager; + return global_client_manager.get (); } /* Check if debuginfod is enabled. If configured to do so, ask the user base-commit: e84060b489746d031ed1ec9e7b6b39fdf4b6cfe3 -- 2.25.4