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 EFA5B385696E for ; Fri, 9 Jun 2023 14:38:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EFA5B385696E 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=1686321510; 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: in-reply-to:in-reply-to:references:references; bh=VMRrcDShtb3t/qYeUMAFLXTFvX4xEb/0X4J774Qbs8o=; b=eNKGkIck6fEt4RT1Cpqc9ns9zjTk5zsowTBOodzd+Rc5DgzeqGxySIojvkjIpKrSvGtm0+ QvdAUREawAAj6MByvxZPawZeDmgMgaHAOitFWxxi6qXN6Gym9wdRwlQHI9o1/0uPWuyxwv sq1MQ9CURuQa14vp2qxjBkSWBRWSbBg= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-122-PUw1WgjuPE6cP_xByoAceA-1; Fri, 09 Jun 2023 10:38:29 -0400 X-MC-Unique: PUw1WgjuPE6cP_xByoAceA-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-3f739cc1aafso8962335e9.1 for ; Fri, 09 Jun 2023 07:38:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686321508; x=1688913508; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VMRrcDShtb3t/qYeUMAFLXTFvX4xEb/0X4J774Qbs8o=; b=B84mNGzQSdkJ2KjnCLU4O4GuGTJAphL3nA/dLTepGmTwXEokXiGZljKEGYE+kz5qch 4FJ7AS/r26EGFV/uo71H2lBpVUX0mTmjJdBj0VIn69N/pCuvf85ZKDY0Rygvw7J0qvH/ wWeTs8mW1AhVuyhY+j58PFPgf70rHd9UqT1SbgboD0G7IGsgUqg/d55ON1l25lt9fWC1 1QtL75PCmlYNKHasl5602NpFIGMiAgofNCuDMRa8N+K/hHIENGrouUCkZ2sJNEBnTiW5 sP3ZhI5yIrw1PISFxSQRdsqaRFrb76zJ+kigiNsc7jDGiYjmcwHfirZcL65RKNs5OwMQ vKqw== X-Gm-Message-State: AC+VfDztZV3rXdZpGc0f154R8mcBMcV2cEXzrn4MhP8nAplqUE/IZpaY J5nrQ/UMbbYKb0A4LNEPJywHON5jyoePgGesrpRzzw7e9BLQ2nySfxulAO3M3f8MturCTndsXPt 6YGTUVQaoPGY7mGKq2l1H9N9GR+8J/cf/ieJRttSP1UhU+KNY+63BnPWTyCmmZJ9WI+ZBeQhptM lC26FLgQ== X-Received: by 2002:a1c:cc0e:0:b0:3f6:113a:2023 with SMTP id h14-20020a1ccc0e000000b003f6113a2023mr1113863wmb.12.1686321508339; Fri, 09 Jun 2023 07:38:28 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6MP0S+RB0JsJ8xfLefmNBL2Nk0pbjhuWYMyI7qcX7/xt0EDshBqJuUrwLP/uDU2sO6Qi6wTA== X-Received: by 2002:a1c:cc0e:0:b0:3f6:113a:2023 with SMTP id h14-20020a1ccc0e000000b003f6113a2023mr1113837wmb.12.1686321507826; Fri, 09 Jun 2023 07:38:27 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id h4-20020adffa84000000b0030647d1f34bsm4648064wrr.1.2023.06.09.07.38.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 09 Jun 2023 07:38:27 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Simon Marchi , Aaron Merey , Mark Wielaard Subject: Re: [PATCHv2] gdb/debuginfod: cleanup debuginfod earlier In-Reply-To: References: <46c030dcd5fc7a1a2ef4e078a92798f18c947ace.1684840908.git.aburgess@redhat.com> Date: Fri, 09 Jun 2023 15:38:26 +0100 Message-ID: <87jzwcya3h.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,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: Andrew Burgess writes: > In V2: > > - Complete rewrite inline with Simon's suggestion. > Thanks for all the reviews. I've now pushed this patch. Thanks, Andrew > -- > > 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(-) > > 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 > > base-commit: baab375361c365afee2577c94cbbd3fdd443d6da > -- > 2.25.4