[PATCH 1/3] refs/packed-backend.c: close fd of empty file

[PATCH 1/3] refs/packed-backend.c: close fd of empty file

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This was an oversight in 01caf20d57a (load_contents(): don't try to mmap an
empty file, 2018-01-24).

This and the following 2 patches apply on master.

 refs/packed-backend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index cec3fb9e00f..d447a731da0 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)
 	size = xsize_t(st.st_size);
 
 	if (!size) {
+		close(fd);
 		return 0;
 	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
 		snapshot->buf = xmalloc(size);
-- 
2.17.1.1185.g55be947832-goog

[PATCH 2/3] sequencer.c: free author variable when merging fails

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a
libified merge_recursive(), 2016-07-26)

 sequencer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 72b4d8ecae3..5c93586cc1c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1771,8 +1771,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	else if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 &head, &msgbuf, opts);
-		if (res < 0)
+		if (res < 0) {
+			free(author);
 			return res;
+		}
 		res |= write_message(msgbuf.buf, msgbuf.len,
 				     git_path_merge_msg(), 0);
 	} else {
-- 
2.17.1.1185.g55be947832-goog

[PATCH 3/3] submodule--helper: plug mem leak in print_default_remote

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7c3cd9dbeba..96024fee1b1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)
 	if (remote)
 		printf("%s\n", remote);
 
+	free(remote);
 	return 0;
 }
 
-- 
2.17.1.1185.g55be947832-goog

Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file

[Jeff King]

On Wed, May 30, 2018 at 10:03:00AM -0700, Stefan Beller wrote:

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index cec3fb9e00f..d447a731da0 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)
>  	size = xsize_t(st.st_size);
>  
>  	if (!size) {
> +		close(fd);
>  		return 0;
>  	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
>  		snapshot->buf = xmalloc(size);

Yep, this is definitely correct. Good catch.

-Peff

Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file

[Michael Haggerty]

On Wed, May 30, 2018 at 7:03 PM, Stefan Beller <sbeller@google.com> wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> This was an oversight in 01caf20d57a (load_contents(): don't try to mmap an
> empty file, 2018-01-24).
>
> This and the following 2 patches apply on master.
>
>  refs/packed-backend.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index cec3fb9e00f..d447a731da0 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)
>         size = xsize_t(st.st_size);
>
>         if (!size) {
> +               close(fd);
>                 return 0;
>         } else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
>                 snapshot->buf = xmalloc(size);
> --
> 2.17.1.1185.g55be947832-goog
>

+1.

Thanks,
Michael

Re: [PATCH 2/3] sequencer.c: free author variable when merging fails

[Johannes Schindelin]

Hi Stefan,

On Wed, 30 May 2018, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a
> libified merge_recursive(), 2016-07-26)

No, it was not deliberate. It was inadvertent, most likely ;-)

>  sequencer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 72b4d8ecae3..5c93586cc1c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1771,8 +1771,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  	else if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) {
>  		res = do_recursive_merge(base, next, base_label, next_label,
>  					 &head, &msgbuf, opts);
> -		if (res < 0)
> +		if (res < 0) {
> +			free(author);
>  			return res;

Why not `goto leave;` instead? I wonder what is happening to the commit
message: can we be certain at this point that it was not set yet? And
also: should we call `update_abort_safety_file()`?

Ciao,
Dscho

> +		}
>  		res |= write_message(msgbuf.buf, msgbuf.len,
>  				     git_path_merge_msg(), 0);
>  	} else {
> -- 
> 2.17.1.1185.g55be947832-goog
> 
> 

Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file

[Johannes Schindelin]

Hi Stefan,

I am Cc:ing Michael, the original author of the fixed commit.

On Wed, 30 May 2018, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> This was an oversight in 01caf20d57a (load_contents(): don't try to mmap an
> empty file, 2018-01-24).
> 
> This and the following 2 patches apply on master.
> 
>  refs/packed-backend.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index cec3fb9e00f..d447a731da0 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)
>  	size = xsize_t(st.st_size);
>  
>  	if (!size) {
> +		close(fd);

Good catch,
Dscho

>  		return 0;
>  	} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {
>  		snapshot->buf = xmalloc(size);
> -- 
> 2.17.1.1185.g55be947832-goog
> 
> 

Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote

[Johannes Schindelin]

Hi Stefan,

On Wed, 30 May 2018, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7c3cd9dbeba..96024fee1b1 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)
>  	if (remote)
>  		printf("%s\n", remote);
>  
> +	free(remote);

Makes sense.

Out of curiosity (and because a cover letter is missing): how did you
stumble over these? Coverity?

Ciao,
Dscho

>  	return 0;
>  }
>  
> -- 
> 2.17.1.1185.g55be947832-goog
> 
> 

Re: [PATCH 2/3] sequencer.c: free author variable when merging fails

[Stefan Beller]

On Thu, May 31, 2018 at 5:04 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Stefan,
>
> On Wed, 30 May 2018, Stefan Beller wrote:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a
>> libified merge_recursive(), 2016-07-26)
>
> No, it was not deliberate. It was inadvertent, most likely ;-)

ok, I am not just bad at writing commit messages, but
also bad at reading other peoples commit messages. ;)

"As this patch is already complex enough, we
leave that change for a later patch." is what lead me to
believe it was deliberate.

>> -             if (res < 0)
>> +             if (res < 0) {
>> +                     free(author);
>>                       return res;
>
> Why not `goto leave;` instead? I wonder what is happening to the commit
> message: can we be certain at this point that it was not set yet? And
> also: should we call `update_abort_safety_file()`?

I think so, but wasn't sure. I wrote these patches before
my usual morning routine. I'll change that.

Thanks,
Stefan

Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote

[Stefan Beller]

On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Stefan,
>
> On Wed, 30 May 2018, Stefan Beller wrote:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/submodule--helper.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 7c3cd9dbeba..96024fee1b1 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)
>>       if (remote)
>>               printf("%s\n", remote);
>>
>> +     free(remote);
>
> Makes sense.
>
> Out of curiosity (and because a cover letter is missing): how did you
> stumble over these? Coverity?

Yes I found them on coverity as I wanted to find out how bad their
false positives are these days. So I looked at the most recent findings.

I somehow imagined that we could redefine the _INIT macros which
usually lead to false positives (just alloc&UNLEAK memory instead of
pointing them all at the same memory for _INIT), but that experiment
did not work out.

Thanks,
Stefan

Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote

[Johannes Schindelin]

Hi Stefan,

On Thu, 31 May 2018, Stefan Beller wrote:

> On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Stefan,
> >
> > On Wed, 30 May 2018, Stefan Beller wrote:
> >
> >> Signed-off-by: Stefan Beller <sbeller@google.com>
> >> ---
> >>  builtin/submodule--helper.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> >> index 7c3cd9dbeba..96024fee1b1 100644
> >> --- a/builtin/submodule--helper.c
> >> +++ b/builtin/submodule--helper.c
> >> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)
> >>       if (remote)
> >>               printf("%s\n", remote);
> >>
> >> +     free(remote);
> >
> > Makes sense.
> >
> > Out of curiosity (and because a cover letter is missing): how did you
> > stumble over these? Coverity?
> 
> Yes I found them on coverity as I wanted to find out how bad their
> false positives are these days. So I looked at the most recent findings.
> 
> I somehow imagined that we could redefine the _INIT macros which
> usually lead to false positives (just alloc&UNLEAK memory instead of
> pointing them all at the same memory for _INIT), but that experiment
> did not work out.

Yes, those many, many, *many* false positives really drown out the benefit
of Coverity for me. It takes all the fun out of looking for quick bug
fixes.

Ciao,
Dscho