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

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  writes:
> > 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

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

[Junio C Hamano]

Johannes Schindelin  writes:

> Spotted by Coverity.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  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(_files))
>   die("diff-files did not exit properly");
>   strbuf_release(_env);

This one looks sensible.

> @@ -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()) {
>   ret = error("error occurred running diff --raw");
>   goto finish;

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

[Johannes Schindelin]

Spotted by Coverity.

Signed-off-by: Johannes Schindelin 
---
 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(_files))
die("diff-files did not exit properly");
strbuf_release(_env);
@@ -497,6 +498,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
}
}
 
+   fclose(fp);
if (finish_command()) {
ret = error("error occurred running diff --raw");
goto finish;
-- 
2.12.2.windows.2.800.gede8f145e06