From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 5FD131F4B4 for ; Wed, 14 Apr 2021 05:19:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237952AbhDNFSu (ORCPT ); Wed, 14 Apr 2021 01:18:50 -0400 Received: from cloud.peff.net ([104.130.231.41]:52000 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230375AbhDNFSt (ORCPT ); Wed, 14 Apr 2021 01:18:49 -0400 Received: (qmail 3245 invoked by uid 109); 14 Apr 2021 05:18:29 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 14 Apr 2021 05:18:29 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 4150 invoked by uid 111); 14 Apr 2021 05:18:29 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 14 Apr 2021 01:18:29 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 14 Apr 2021 01:18:27 -0400 From: Jeff King To: Junio C Hamano Cc: SZEDER =?utf-8?B?R8OhYm9y?= , Rafael Silva , Jonathan Tan , Git Mailing List Subject: Re: [PATCH 1/3] is_promisor_object(): free tree buffer after parsing Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Apr 13, 2021 at 01:17:55PM -0700, Junio C Hamano wrote: > > diff --git a/packfile.c b/packfile.c > > index 8668345d93..b79cbc8cd4 100644 > > --- a/packfile.c > > +++ b/packfile.c > > @@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid, > > return 0; > > while (tree_entry_gently(&desc, &entry)) > > oidset_insert(set, &entry.oid); > > + free_tree_buffer(tree); > > } else if (obj->type == OBJ_COMMIT) { > > struct commit *commit = (struct commit *) obj; > > struct commit_list *parents = commit->parents; > > Hmph, does an added free() without removing one later mean we've > been leaking? Yes. Though perhaps not technically a leak, in that we are still holding on to the "struct tree" entries via the obj_hash table. But nobody was freeing them at all until the end of the program. I actually think it may be a mistake for "struct tree" to have buffer/len fields at all. It is a slight convenience to be able to pass them around with the struct, but it makes the expected lifetime much more confusing. In practice, all code wants to deal with one tree at a time, then drop the buffer when it's done (we might hold several when recursing through subtrees, but we'd never hold more than the distance from the leaf to the root, and each recursive invocation of something like process_tree() is holding exactly one tree buffer). It may not be worth the trouble to try to clean it up at this point, though. -Peff