Re: [PATCH 08/26] difftool: close file descriptors after reading

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 27, 2017, 06:05 UTC
Message-ID: <xmqqzif2mlm1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <c634ea5dad0ce1c1f424f54a12a5614e6fa4a847.1493237937.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 19 quoted lines

> Spotted by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/difftool.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 1354d0e4625..a4f1d117ef6 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -226,6 +226,7 @@ static void changed_files(struct hashmap *result, const char *index_path,
>  		hashmap_entry_init(entry, strhash(buf.buf));
>  		hashmap_add(result, entry);
>  	}
> +	fclose(fp);
>  	if (finish_command(&diff_files))
>  		die("diff-files did not exit properly");
>  	strbuf_release(&index_env);

This one looks sensible.

Show 5 quoted lines

> @@ -497,6 +498,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  		}
>  	}
>  
> +	fclose(fp);

The huge loop we see in the pre-context of this hunk has many "return"s and "goto finish"es that can leave fp still open; while this patch does not hurt, it is probably somewhat insufficient.

>  	if (finish_command(&child)) {
>  		ret = error("error occurred running diff --raw");
>  		goto finish;