From patchwork Thu Jun 8 13:33:52 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 70784 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0C58D38582B7 for ; Thu, 8 Jun 2023 13:34:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0C58D38582B7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1686231266; bh=NvnIt+NkJaw/CleUVTlc7TH16ZvsD+gsYJcF6rumiDI=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=hGB4pbn3L6Ntr58W7MYt2xZqIBpDPFsh2MEy+CiCqRLrtOj0eHVU4Ziz8VcAEVKgm k//VBLWtdU2wXjh/aIlvJosKuWvYcqnXF+ey81LGe0bLvvD04s7L1FRVP3siCEgJnb Q+sKcb/NNYDEqhL/d69fnp1KeIW41gDxa943aPNE= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 34F2F3858C62 for ; Thu, 8 Jun 2023 13:33:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 34F2F3858C62 Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-551-emxotCmZMLmdBln7icb1OA-1; Thu, 08 Jun 2023 09:33:57 -0400 X-MC-Unique: emxotCmZMLmdBln7icb1OA-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-30ae7bd987dso278643f8f.2 for ; Thu, 08 Jun 2023 06:33:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686231236; x=1688823236; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=NvnIt+NkJaw/CleUVTlc7TH16ZvsD+gsYJcF6rumiDI=; b=FZpLlmR2fOCbSCReOUsFGP6pwDQ1Yy8WvRtsErTRfEdCIfPnFXdNUi6w0oOswMgZgj cKE3PQ0N5Q5mm7C3Gg3PSQvCZtBJvQe5XgqZOzeCBRpmJvk+3aMUSvvFpFF3PBPZh+a7 TP420eeAUJ4VrCSI5iouZGuiudrJ4iarR+f5klG7LvMj9IdDVRO6QlI1eVb9qP8VfnfG NYYBiMBfvSkpC7kriEuxmob/byX0tF0VmtqTnsHlbTxoPX8G0LpceJ6lEspg7sDZWXTU ay9WKa5x2u7TKidIkTzTztiS4AdfbQjGS7W6HWG6QSPmE57wHKNACUxMdaY0JXJqwDM7 sZFQ== X-Gm-Message-State: AC+VfDxJSl5mm2s+PZ5qYj9G0Y8io30OD4bKv1W0mgo5xbejYQCGcaLB tH7vnX/1G0FIYUYnrZ5HdSvvZGlls+LPKzYwQmqjuIFR7vJknCejHQ/dQoqL0n8C5LK7YChv7Fx NBsszm2xzMAfKhuY0tzL82Fynio9APLK3GO3i/5N/H48y46cbHadcqqwmPku/WDUp/htI/wGqhK /Q9ToPIg== X-Received: by 2002:a5d:5642:0:b0:307:bd64:f5a4 with SMTP id j2-20020a5d5642000000b00307bd64f5a4mr7389606wrw.52.1686231236233; Thu, 08 Jun 2023 06:33:56 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6LX9uhFI9rqFdqGeYmfHa0Tza/CGs8h+7ePxiNlJEnmJfiOGZ5zU6ou+MYz4D2gD5irlOY9Q== X-Received: by 2002:a5d:5642:0:b0:307:bd64:f5a4 with SMTP id j2-20020a5d5642000000b00307bd64f5a4mr7389580wrw.52.1686231235789; Thu, 08 Jun 2023 06:33:55 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id k10-20020adff5ca000000b003077a19cf75sm1628143wrp.60.2023.06.08.06.33.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Jun 2023 06:33:55 -0700 (PDT) To: gdb-patches@sourceware.org Cc: Andrew Burgess , Simon Marchi , Aaron Merey , Mark Wielaard Subject: [PATCHv2] gdb/debuginfod: cleanup debuginfod earlier Date: Thu, 8 Jun 2023 14:33:52 +0100 Message-Id: X-Mailer: git-send-email 2.25.4 In-Reply-To: <46c030dcd5fc7a1a2ef4e078a92798f18c947ace.1684840908.git.aburgess@redhat.com> References: <46c030dcd5fc7a1a2ef4e078a92798f18c947ace.1684840908.git.aburgess@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.8 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, 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 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-Patchwork-Original-From: Andrew Burgess via Gdb-patches From: Andrew Burgess Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" In V2: - Complete rewrite inline with Simon's suggestion. Reviewed-By: Tom Tromey --- 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. This commit removes the debuginfod_client_up type, and instead has GDB hold a raw pointer to the debuginfod_client object. We then make use of GDB's make_final_cleanup to register a function that will call debuginfod_end. As GDB's final cleanups are called before exit is called, this means that debuginfod_end will be called before the at_exit handlers are called, and the crash identified above is resolved. It's not obvious how this issue can easily be tested for. The bug does not appear to manifest when using a local debuginfod server, so we'd need to setup something more involved. For now I'm proposing this patch without any associated tests. Co-Authored-By: Mark Wielaard Co-Authored-By: Simon Marchi --- gdb/debuginfod-support.c | 47 +++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 18 deletions(-) base-commit: baab375361c365afee2577c94cbbd3fdd443d6da diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index 5853f420a18..a41a4c95785 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,45 @@ progressfn (debuginfod_client *c, long cur, long total) return 0; } +/* Cleanup ARG, which is a debuginfod_client pointer. */ + +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 = nullptr; if (global_client == nullptr) { - global_client.reset (debuginfod_begin ()); + global_client = debuginfod_begin (); if (global_client != nullptr) - debuginfod_set_progressfn (global_client.get (), progressfn); + { + /* It is important that we cleanup the debuginfod_client object + before calling exit. Some of the libraries used by debuginfod + make use of at_exit handlers to perform cleanup. + + If we wrapped the debuginfod_client in a unique_ptr and relied + on its destructor to cleanup then this would be run as part of + the global C++ object destructors, which is after the at_exit + handlers, which is too late. + + So instead, we make use of GDB's final cleanup mechanism. */ + 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