threads / patch / 48604

patch, 3 partsrefs/packed-backend.c: close fd of empty file

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

## tl;dr

11 messages between May 30, 2018 and Jun 1, 2018. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Stefan Beller· May 30, 2018, 17:03 UTC · lore
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(+)
Show changes to refs/packed-backend.c +1 −0
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
Stefan Beller· May 30, 2018, 17:03 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to sequencer.c +3 −1
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
Johannes Schindelin· May 31, 2018, 12:04 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Wed, 30 May 2018, Stefan Beller wrote:
Show 5 quoted lines
> 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 ;-)
Show 15 quoted lines
>  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

Show 8 quoted lines
> +		}
>  		res |= write_message(msgbuf.buf, msgbuf.len,
>  				     git_path_merge_msg(), 0);
>  	} else {
> -- 
> 2.17.1.1185.g55be947832-goog
> 
>
Stefan Beller· May 31, 2018, 18:52 UTC · re: Johannes Schindelin · lore

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

On Thu, May 31, 2018 at 5:04 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 11 quoted lines
> 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.

Show 8 quoted lines
>> -             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

Stefan Beller· May 30, 2018, 17:03 UTC · re: Stefan Beller · lore

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to builtin/submodule--helper.c +1 −0
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
Johannes Schindelin· May 31, 2018, 12:07 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Wed, 30 May 2018, Stefan Beller wrote:
Show 14 quoted lines
> 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

Show 7 quoted lines
>  	return 0;
>  }
>  
> -- 
> 2.17.1.1185.g55be947832-goog
> 
>
Stefan Beller· May 31, 2018, 18:55 UTC · re: Johannes Schindelin · lore

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

On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 23 quoted lines
> 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

Johannes Schindelin· Jun 1, 2018, 08:46 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Thu, 31 May 2018, Stefan Beller wrote:
Show 33 quoted lines
> 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

Jeff King· May 31, 2018, 05:15 UTC · re: Stefan Beller · lore

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

On Wed, May 30, 2018 at 10:03:00AM -0700, Stefan Beller wrote:
Show 12 quoted lines
> 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
Michael Haggerty· May 31, 2018, 11:49 UTC · re: Stefan Beller · lore

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

On Wed, May 30, 2018 at 7:03 PM, Stefan Beller <sbeller@google.com> wrote:
Show 26 quoted lines
> 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

Johannes Schindelin· May 31, 2018, 12:06 UTC · re: Stefan Beller · lore

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

Hi Stefan,
I am Cc:ing Michael, the original author of the fixed commit.
On Wed, 30 May 2018, Stefan Beller wrote:
Show 20 quoted lines
> 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

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

← back to recent threads