Sender: Junio C Hamano <jch2355@gmail.com>
From:   Junio C Hamano <gitster@pobox.com>
To:     Taylor Blau <me@ttaylorr.com>
Cc:     git@vger.kernel.org, Jeff King <peff@peff.net>,
        Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v2 01/10] pack-write.c: plug a leak in
 stage_tmp_packfiles()
References: <cover.1681764848.git.me@ttaylorr.com>
        <cover.1681850424.git.me@ttaylorr.com>
        <c477b754e7ddde0d6e696cfd4027ad88c18aeff3.1681850424.git.me@ttaylorr.com>
Date:   Wed, 19 Apr 2023 15:00:48 -0700
In-Reply-To: <c477b754e7ddde0d6e696cfd4027ad88c18aeff3.1681850424.git.me@ttaylorr.com>
        (Taylor Blau's message of "Tue, 18 Apr 2023 16:40:32 -0400")
Message-ID: <xmqqpm7z7crz.fsf@gitster.g>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Taylor Blau <me@ttaylorr.com> writes:

> The function `stage_tmp_packfiles()` generates a filename to use for
> staging the contents of what will become the pack's ".mtimes" file.
>
> The name is generated in `write_mtimes_file()` and the result is
> returned back to `stage_tmp_packfiles()` which uses it to rename the
> temporary file into place via `rename_tmp_packfiles()`.
>
> `write_mtimes_file()` returns a `const char *`, indicating that callers
> are not expected to free its result (similar to, e.g., `oid_to_hex()`).
> But callers are expected to free its result, so this return type is
> incorrect.

Indeed the string that holds the name of the file returned by
write_mtimes_file() is leaking.  Does the same logic apply to the
returned filename from write_rev_file() and stored in rev_tmp_name
that is not freed in stage_tmp_packfiles() in another topic?

> @@ -544,7 +544,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
>  			 char **idx_tmp_name)
>  {
>  	const char *rev_tmp_name = NULL;
> -	const char *mtimes_tmp_name = NULL;
> +	char *mtimes_tmp_name = NULL;
>  
>  	if (adjust_shared_perm(pack_tmp_name))
>  		die_errno("unable to make temporary pack file readable");
> @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
>  		rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
>  	if (mtimes_tmp_name)
>  		rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes");
> +
> +	free(mtimes_tmp_name);
>  }
>  
>  void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)