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

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 1354d0e4625..a4f1d117ef6 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c

@@ -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.

You are absolutely correct, and I am somewhat surprised that Coverity did
not complain more loudly.

TBH I really only tried to address these fixes on the cheap, as my main
aim was to get to the critical bugs in the Windows-specific part (I did
not find anything major, though). Therefore, I only looked at what
Coverity labels as the "high impact" issues. The leaks in the loop that
you pointed out may be labeled as minor by Coverity.

I also noticed that a couple of error messages have not been marked as
translateable, I must have forgotten that before submitting the difftool
patch series :-(

In any case, in the next iteration of this patch series, this patch will
convert all returns to `ret = ...; goto finish;` calls, and fclose(fp)
there unless it has been closed already.

Thanks,
Dscho