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 A039E1F54E for ; Mon, 18 Jul 2022 15:11:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235201AbiGRPLl (ORCPT ); Mon, 18 Jul 2022 11:11:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58878 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235185AbiGRPLk (ORCPT ); Mon, 18 Jul 2022 11:11:40 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4B80E25E86 for ; Mon, 18 Jul 2022 08:11:39 -0700 (PDT) Received: (qmail 32246 invoked by uid 109); 18 Jul 2022 15:11:38 -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:11:38 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 10083 invoked by uid 111); 18 Jul 2022 15:11:38 -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:11:38 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 18 Jul 2022 11:11:38 -0400 From: Jeff King To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: git@vger.kernel.org, Junio C Hamano Subject: Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing 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:35PM +0200, Ævar Arnfjörð Bjarmason wrote: > There's several potential ways to fix those sort of leaks, we could > add a "nodup" mode to "struct strvec", which would work for the cases > where we push constant strings to it. But that wouldn't work as soon > as we used strvec_pushf(), or otherwise needed to duplicate or create > a string for that "struct strvec". Right, I think this falls down when you have mixed const/non-const entries. You have to know which is which for strvec_clear(). > Let's instead make it the responsibility of the revisions API. If it's > going to clobber elements of argv it can also free() them, which it > will now do if instructed to do so via "free_removed_argv_elements". OK, I think this is a reasonable and minimal fix for the "--" problem on its own. But if you went just a little further and made the option "rev should own its own argv", then I think you can simplify life for callers even more. They could construct a strvec themselves and then hand it off to the rev_info, to be cleaned up when release_revisions() is called (and of course freeing the "--" when we overwrite it in the interim, as you do here). Then all of the bisect callers from the previous patch could avoid having to deal with the strvec at all. They'd call bisect_rev_setup(), which would internally attach the memory to rev_info. > bisect.c | 6 ++++-- > builtin/submodule--helper.c | 5 ++++- > remote.c | 5 ++++- > revision.c | 2 ++ > revision.h | 3 ++- > t/t2020-checkout-detach.sh | 1 + > 6 files changed, 17 insertions(+), 5 deletions(-) The patch itself works as advertised, I think. -Peff