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: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id 5498D1F54E for ; Mon, 18 Jul 2022 15:05:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235065AbiGRPFL (ORCPT ); Mon, 18 Jul 2022 11:05:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55144 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235041AbiGRPFK (ORCPT ); Mon, 18 Jul 2022 11:05:10 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5E781252A2 for ; Mon, 18 Jul 2022 08:05:09 -0700 (PDT) Received: (qmail 32234 invoked by uid 109); 18 Jul 2022 15:05:08 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 18 Jul 2022 15:05:08 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 10035 invoked by uid 111); 18 Jul 2022 15:05:08 -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; Mon, 18 Jul 2022 11:05:08 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 18 Jul 2022 11:05:08 -0400 From: Jeff King To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: git@vger.kernel.org, Junio C Hamano Subject: Re: [PATCH 5/6] bisect.c: partially fix bisect_rev_setup() memory leak Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Jul 13, 2022 at 03:10:34PM +0200, Ævar Arnfjörð Bjarmason wrote: > Partially fix the memory leak noted in in 8a534b61241 (bisect: use > argv_array API, 2011-09-13), which added the "XXX" comment seen in the > context. We can partially fix it by having the bisect_rev_setup() > function take a "struct strvec", rather than constructing it. > > As the comment notes we need to keep the construct "rev_argv" around > while the "struct rev_info" is around, which as seen in the newly > added "strvec_clear()" calls here we do after "release_revisions()". This seems like not too bad a solution on its face. The caller is responsible for keeping rev_argv around as long as rev is valid. This would be simpler if setup_revisions() made its own copy of the argv strings and dropped them via release_revisions(). That has a small cost, but I suspect it doesn't matter much in practice. I thought that would be too much of a pain to add, but it kind of looks like you went there anyway in the next patch. I'll comment further there. > This "partially" fixes the memory leak because we're leaking the "--" > added to the "rev_argv" here still, which will be addressed in a > subsequent commit. This part confused me, because the "--" is in the strvec which gets cleared. I'd guess that the revisions code just overwrites it with NULL, and we lose access to it. But it should become clear in the next commit. > - setup_revisions(rev_argv.nr, rev_argv.v, revs, NULL); > + setup_revisions(rev_argv->nr, rev_argv->v, revs, NULL); > /* XXX leak rev_argv, as "revs" may still be pointing to it */ I wouldn't really call this a "leak" anymore; the caller now owns it. Maybe nit-picking, since this comment goes away later. -Peff