From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32f.google.com (mail-wm1-x32f.google.com [IPv6:2a00:1450:4864:20::32f]) by sourceware.org (Postfix) with ESMTPS id 08ADA385843B for ; Mon, 27 Sep 2021 13:40:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 08ADA385843B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wm1-x32f.google.com with SMTP id b192-20020a1c1bc9000000b0030cfaf18864so1100wmb.4 for ; Mon, 27 Sep 2021 06:40:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=gjwpkD+5FrW+Hgz396tS+KM3EJCxLZVmjp0ev0kPjx4=; b=CeXTAStl3bTHcvJfuELd21YKVYQ4Cn0BuyTDXAS/bJhKiRNVIkXvSD7cEAP9cZOxsq sbW/PE9c4QYOu6EIujXO7jFvSFUnEX8c7F1jjF3eNtKDQhaHfh9V/rHAQKcQnQTKGaSD 4dOZ39MewnjR9x/5JMjNzv2VpnKYFvXtxfJb3HR5v5DkW6CoIa0CMgc2AfktbT5WWhIY xw71GdQ+yQySIkfjUmOhrHmsGGkpf4QmPqu63G/Cp8gyGUcmbWE2pg1nkQmtpnmwE8qb rkYsz/q7he28SbNccqG/aWU41F67vbh8O962wvFRpwiOxDguxEOqhuotnRp6WX7pKUYd grDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=gjwpkD+5FrW+Hgz396tS+KM3EJCxLZVmjp0ev0kPjx4=; b=aGjpm0IrOWMJu28qHPvOfurmwNgMfdOI5MXUVWxkdp9+E/xsBhB4KwK3EIDaj8yIIf r4WdFgbHKKcSCt4CExXXPi4aK8wYfUzjFz5J1ssT4Evb3D4cNBIicqMrnXa9A/GHe3Px oly0/YxCdpQM9bMsiHa8P0tMINNmYDE0LOg4YaJFDE0ZQv4eudxoc0sTRfnwdlL7vrI7 erIa7lQMfb3oWged8vwa1Nhb7en3Qfi2nffoAQm3U8oCOH9L16u1kyr5aVg0E9/pE97M pG/oByDlkurI36L3ZJGdu1yC7iAQlalAOMwkcD6aE417yUf83us4YXddGkASmbPD867O dEcQ== X-Gm-Message-State: AOAM532TEYzDBGKfb3+d+Q5N8Js61oD1Of7ioHGcdh9UtDmJtGlfImcK 8SQr5BdqF2BiOFIlB/a9Sm2uUMLvwhgqeQ== X-Google-Smtp-Source: ABdhPJz8HhEcdkgkC4+slxbCVsliDsgZrZ9eFTBWqGdbm/kSUyy4biEm+jQgIN1BgqNm05uH3pWOtA== X-Received: by 2002:a1c:f713:: with SMTP id v19mr86383wmh.188.1632750045725; Mon, 27 Sep 2021 06:40:45 -0700 (PDT) Received: from localhost (host86-169-137-11.range86-169.btcentralplus.com. [86.169.137.11]) by smtp.gmail.com with ESMTPSA id l26sm20644165wmi.25.2021.09.27.06.40.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Sep 2021 06:40:45 -0700 (PDT) Date: Mon, 27 Sep 2021 14:40:43 +0100 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/python: fix memory leak in python inferior code Message-ID: <20210927134043.GG1900093@embecosm.com> References: <20210905085309.2153900-1-andrew.burgess@embecosm.com> <08fb4316-00d8-2337-ba3b-cd86c1765b5d@polymtl.ca> <20210908092634.GU2581@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210908092634.GU2581@embecosm.com> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 14:40:27 up 8 days, 4:49, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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-List-Received-Date: Mon, 27 Sep 2021 13:40:48 -0000 Ping! Thanks, Andrew * Andrew Burgess [2021-09-08 10:26:34 +0100]: > * Simon Marchi [2021-09-07 11:10:20 -0400]: > > > > > > > On 2021-09-05 4:53 a.m., Andrew Burgess wrote: > > > When a user creates a gdb.Inferior object for the first time a new > > > Python object is created. This object is then cached within GDB's > > > inferior object using the registry mechanism (see > > > inferior_to_inferior_object in py-inferior.c, specifically the calls > > > to inferior_data and set_inferior_data). > > > > > > This reference to the Python object, held within the real inferior > > > object, counts as a reference to the Python object, and so holds the > > > reference count of the Python object above zero. > > > > > > At the same time, the Python object maintains a pointer back to GDB's > > > real inferior object. > > > > > > When GDB's inferior object is deleted (say the inferior exits) then > > > py_free_inferior is called (thanks to the registry system), this > > > function looks up the Python object and sets the inferior pointer to > > > nullptr and finally deletes reduces the reference count on the Python > > > object. > > > > > > If at this point the user still holds a reference to the Python object > > > then nothing happens. The gdb.Inferior Python object is now in the > > > non-valid state (see infpy_is_valid in py-inferior.c), but otherwise, > > > everything is fine. > > > > > > However, if there are no further references to the gdb.Inferior > > > object, or, once the user has given up all their references to the > > > gdb.Inferior object, then infpy_dealloc is called. > > > > > > This function currently checks to see if the inferior pointer within > > > the gdb.Inferior object is nullptr or not. If the pointer is nullptr > > > then infpy_dealloc immediately returns. > > > > > > Only when the inferior point in the gdb.Inferior is not nullptr do > > > we (a) set the gdb.Inferior reference inside GDB's inferior to > > > nullptr, and (b) call the underlying Python tp_free function. > > > > > > There are a number things wrong here: > > > > > > 1. The gdb.Inferior reference within GDB's inferior object hold a > > > reference count, thus, setting this reference to nullptr without > > > first decrementing the reference count would leak a reference. This > > > could be solved by having the inferior hold a gdbpy_ref object, > > > however... > > > > > > 2. As GDB's inferior holds a reference then infpy_dealloc will never > > > be called until GDB's inferior object is deleted, calls > > > py_free_inferior, and so gives up the reference. At this point > > > there is no longer a need to call set_inferior_data to set the field > > > back to NULL, that field must have been cleared in order to get the > > > reference count to zero, which means... > > > > > > 3. If we know that py_free_inferior must be called before > > > infpy_dealloc, then we know that the inferior pointer in > > > gdb.Inferior will always be nullptr when infpy_dealloc is called, > > > this means that the call to the underlying tp_free function will > > > always be skipped. Skipping this call at all always seems like a > > > bad idea, but it turns out we will always skip it. > > > > > > Given all of the above, I assert that the gdb.Inferior field will > > > always be nullptr when infpy_dealloc is called. That's what this > > > patch does. > > > > > > I assume that failing to call tp_free will mean that Python leaks some > > > memory associated with the gdb.Inferior object, but I don't know any > > > way that I could show that in a test. As such, I've got no new tests > > > for this commit, but if anyone has any ideas I'd be happy to add > > > something. > > > > I tried to read this a few times, and I'm stuck on not understanding how > > there isn't a reference cycle here. If the Python object holds a > > reference on the inferior, and the inferior holds a reference on the > > Python object (through the registry), how can that work? Or is it a > > weak reference in one of the directions? > > There's no "reference" from the Python object to the inferior in the > same way there's a "reference" from the inferior to the Python object. > > That is the inferior (via the registry) holds a reference to the > Python object. This means that the Python object can't be deleted > until the reference in the inferior is released (that reference holds > the refcount above zero). > > However, the Python object only holds a pointer back to the inferior, > even though inferiors are a child of refcounted_object, the pointer > within the Python object isn't handled as a refcounted_object, just a > raw pointer. > > So, the inferior's refcount can hit zero with no consideration for the > pointer held within the Python object, but that's OK, when the > inferior is deleted we clean up the registry. This calls into the > Python code, which sets the inferior pointer to nullptr, and > decrements the refcount on the Python object. > > When the user gives up their last gdb.Inferior reference the Python > object can then be free'd. > > Hope that makes things a little clearer, > > Thanks, > Andrew