From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x435.google.com (mail-wr1-x435.google.com [IPv6:2a00:1450:4864:20::435]) by sourceware.org (Postfix) with ESMTPS id C64F93858D34 for ; Sun, 5 Sep 2021 08:53:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C64F93858D34 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-wr1-x435.google.com with SMTP id x6so4974770wrv.13 for ; Sun, 05 Sep 2021 01:53:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=wksaVbHcw1A5fvA/Ey0sLmAKNEOpXsSgmMhJsdEfMT0=; b=Hguf5LHqPon3t0eazaDEp4QFdWuEWrgJHBERoJrkJYU8l7iRsnZwKGDqIjibh+a1YO Nnl4QTP6C0WTPeekoi26Nv6t51mat9jj6KZyF8TLySySFBRiIMCevYsYvpiiYFtuunLA Z2s8Clm0b+9R+9NKTIY309ZNpMqqtUjTW2uzSS6fyecNON7+98TOXzWaDmFAyegUF3wl M3IJy0Bi7/vqQZbHYudoQGt0Ao1/X2eGQuAiGRr9XkETDTpuUTtnw8BYKLcNp/E4UPJV 4RMZ3JQD8xyKbgzX6wRDswtjXrYN6r6HOhNNQ5Z7ovUkjHflb06S1rN2/8XIVBlDvict TnEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=wksaVbHcw1A5fvA/Ey0sLmAKNEOpXsSgmMhJsdEfMT0=; b=h0Upfikb1/cnZ6P7Ht2uVdIM0E1D4LLMB/tWTTjAxC8hNj958+fkzfareRd/rqCUrQ 0NrNsjOygoB7rBYS0oCcM2BOMyzMSgQjXULzAeg8Kur2il2kFZHDCMoh5mncXPxrupa/ quQ+SD10qfeyRw1wPQG1kFa6nx354GRW6zn7+6q1iBhCKsWa4U5i8b4LFXy8ieCyUj8I yAKU/9Zi5Axsc9c4vvT0mGMTktEr4PbXW7HCUXSfDcuxo+uFjiGlA5QEywEy6p3iWRTf hHWHz3Fy5wQWZ8fRmGM20ed+mGVigaaV/Bb98YHzA10ZNHOAdhvh1gIWyXcnkGYTyS8c 7fFA== X-Gm-Message-State: AOAM532uG4aEAT83y/tP1dQ2YwC7cgp+vR4myI0jN7fpxx2+jl9bSW1e hITIE/EiXo3eXJNrum8ziP1GFOn2/bv30Q== X-Google-Smtp-Source: ABdhPJzD5O/3A/IM7l+fYKN53c44U4RSH2qdnG9A8Tl5YlCcW3RLUVejvnRkBNKR+1teYMg5sSVHJA== X-Received: by 2002:a5d:6286:: with SMTP id k6mr7438021wru.103.1630831991694; Sun, 05 Sep 2021 01:53:11 -0700 (PDT) Received: from localhost (host86-140-92-92.range86-140.btcentralplus.com. [86.140.92.92]) by smtp.gmail.com with ESMTPSA id o26sm4044894wmc.17.2021.09.05.01.53.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 05 Sep 2021 01:53:11 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: [PATCH] gdb/python: fix memory leak in python inferior code Date: Sun, 5 Sep 2021 09:53:09 +0100 Message-Id: <20210905085309.2153900-1-andrew.burgess@embecosm.com> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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: Sun, 05 Sep 2021 08:53:14 -0000 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. --- gdb/python/py-inferior.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c index cfbc2f6574f..c36f275b23e 100644 --- a/gdb/python/py-inferior.c +++ b/gdb/python/py-inferior.c @@ -864,12 +864,20 @@ static void infpy_dealloc (PyObject *obj) { inferior_object *inf_obj = (inferior_object *) obj; - struct inferior *inf = inf_obj->inferior; - if (! inf) - return; + /* The inferior itself holds a reference to this Python object, which + will keep the reference count of this object above zero until GDB + deletes the inferior and py_free_inferior is called. + + Once py_free_inferior has been called then the link between this + Python object and the inferior is set to nullptr, and then the + reference count on this Python object is decremented. + + The result of all this is that the link between this Python object and + the inferior should always have been set to nullptr before this + function is called. */ + gdb_assert (inf_obj->inferior == nullptr); - set_inferior_data (inf, infpy_inf_data_key, NULL); Py_TYPE (obj)->tp_free (obj); } -- 2.25.4