Re: [PATCH] commit-graph: close file before returning NULL

[Junio C Hamano <gitster@pobox.com>]

"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