From: Junio C Hamano <gitster@pobox•com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, me@ttaylorr•com,
"Derrick Stolee" <derrickstolee@github•com>,
"Kleber Tarcísio" <klebertarcisio@yahoo•com.br>
Subject: Re: [PATCH] commit-graph: close file before returning NULL
Date: Wed, 20 Apr 2022 13:55:43 -0700 [thread overview]
Message-ID: <xmqq8rrzo4og.fsf@gitster.g> (raw)
In-Reply-To: <pull.1213.git.1650302007395.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Mon, 18 Apr 2022 17:13:27 +0000")
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail•com> writes:
> From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= <klebertarcisio@yahoo•com.br>
>
> There are two reasons that we could return NULL early within
> load_commit_graph_chain():
>
> 1. The file does not exist, so the file pointer is NULL.
> 2. The file exists, but is too small to contain a single hash.
>
> These were grouped together when the function was first written in
> 5c84b3396 (commit-graph: load commit-graph chains, 2019-06-18) in order
> to simplify how the 'chain_name' string is freed. However, the current
> code leaves a narrow window where the file pointer is not closed when
> the file exists, but is rejected for being too small.
>
> Split out these cases separately to ensure we close the file in this
> case.
>
> Signed-off-by: Kleber Tarcísio <klebertarcisio@yahoo•com.br>
> Signed-off-by: Derrick Stolee <derrickstolee@github•com>
> ---
> commit-graph: close file before returning NULL
>
> This change was originally submitted to the microsoft/git fork [1].
> Kleber discovered this issue using some automated tool they are working
> on. We recommended that this change be submitted to the core Git group,
> but we have not had any word from the original author in some time.
> Hence, I am submitting it on their behalf.
Makes me wonder if it were a better world if fclose() behaved more
like free() ;-)
Will queue. Thanks.
> commit-graph.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 441b36016ba..06107beedcb 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -523,10 +523,13 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
> stat_res = stat(chain_name, &st);
> free(chain_name);
>
> - if (!fp ||
> - stat_res ||
> - st.st_size <= the_hash_algo->hexsz)
> + if (!fp)
> return NULL;
> + if (stat_res ||
> + st.st_size <= the_hash_algo->hexsz) {
> + fclose(fp);
> + return NULL;
> + }
>
> count = st.st_size / (the_hash_algo->hexsz + 1);
> CALLOC_ARRAY(oids, count);
>
> base-commit: ab1f2765f78e75ee51dface57e1071b3b7f42b09
prev parent reply other threads:[~2022-04-20 20:56 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-18 17:13 [PATCH] commit-graph: close file before returning NULL Derrick Stolee via GitGitGadget
2022-04-20 20:55 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq8rrzo4og.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=derrickstolee@github$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=klebertarcisio@yahoo$(echo .)com.br \
--cc=me@ttaylorr$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox