refs/packed-backend.c: close fd of empty file

PATCH In review Stefan Beller 4 participants 11 messages May 30, 2018 – Jun 1, 2018

lore

Share & tools

lore

Stefan Beller

author

2018-05-30 17:03 UTC #0 lore raw

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

Signed-off-by Stefan Beller <[email protected]>

---

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(+)

refs/packed-backend.c +1 -0

index cec3fb9e00f..d447a731da0 100644
@@ -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);

Signature · 1 line

2.17.1.1185.g55be947832-goog

Stefan Beller

author

2018-05-30 17:03 UTC #1 lore raw

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

Signed-off-by Stefan Beller <[email protected]>

---

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(-)

sequencer.c +3 -1

index 72b4d8ecae3..5c93586cc1c 100644
@@ -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 {

Signature · 1 line

2.17.1.1185.g55be947832-goog

Johannes Schindelin

2018-05-31 12:04 UTC #2 lore raw

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

Hi Stefan,

Quoted 5 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>
---
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 ;-)

Quoted 8 lines

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

sequencer.c +0 -0

index 72b4d8ecae3..5c93586cc1c 100644

sequencer.c static int do_pick_commit(enum todo_command command, struct commit *commit, · line 1776

Quoted 7 lines

sequencer.c +2 -1

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

Quoted 8 lines

sequencer.c +1 -0

+		}
 		res |= write_message(msgbuf.buf, msgbuf.len,
 				     git_path_merge_msg(), 0);
 	} else {

Signature · 1 line

2.17.1.1185.g55be947832-goog

Stefan Beller

author

2018-05-31 18:52 UTC #3 lore raw

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

Quoted 11 lines · Johannes Schindelin

Hi Stefan,
Quoted 5 lines · Stefan Beller
Signed-off-by Stefan Beller <[email protected]>
---
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.

Quoted 8 lines

sequencer.c static int do_pick_commit(enum todo_command command, struct commit *commit, · line 1776
Quoted 4 lines
sequencer.c +2 -1
-             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

author

2018-05-30 17:03 UTC #4 lore raw

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

In reply to Stefan Beller

Signed-off-by Stefan Beller <[email protected]>

---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)

builtin/submodule--helper.c +1 -0

index 7c3cd9dbeba..96024fee1b1 100644
@@ -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;
 }

Signature · 1 line

2.17.1.1185.g55be947832-goog

Johannes Schindelin

2018-05-31 12:07 UTC #5 lore raw

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

Hi Stefan,

Quoted 10 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>
---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)
builtin/submodule--helper.c +0 -0
index 7c3cd9dbeba..96024fee1b1 100644

builtin/submodule--helper.c static int print_default_remote(int argc, const char **argv, const char *prefix) · line 66

Quoted 4 lines

builtin/submodule--helper.c +1 -0

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

Quoted 7 lines

builtin/submodule--helper.c +0 -0
 	return 0;
 }
 
Signature · 1 line
2.17.1.1185.g55be947832-goog

Stefan Beller

author

2018-05-31 18:55 UTC #6 lore raw

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

Quoted 17 lines · Johannes Schindelin

Hi Stefan,

Quoted 13 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>

---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)

builtin/submodule--helper.c +0 -0

index 7c3cd9dbeba..96024fee1b1 100644
@@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)
      if (remote)
              printf("%s\n", remote);

Quoted 6 lines

builtin/submodule--helper.c static int print_default_remote(int argc, const char **argv, const char *prefix) · line 66
Quoted 1 line
builtin/submodule--helper.c +1 -0
+     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

2018-06-01 08:47 UTC #7 lore raw

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

Hi Stefan,

Quoted 26 lines · Stefan Beller

Quoted 17 lines · Johannes Schindelin
Hi Stefan,
Quoted 13 lines · Stefan Beller
Signed-off-by Stefan Beller <[email protected]>
---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)
builtin/submodule--helper.c +0 -0
index 7c3cd9dbeba..96024fee1b1 100644
@@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)
      if (remote)
              printf("%s\n", remote);
 
Quoted 6 lines
builtin/submodule--helper.c static int print_default_remote(int argc, const char **argv, const char *prefix) · line 66
Quoted 1 line
builtin/submodule--helper.c +1 -0
+     free(remote);
Makes sense.
Out of curiosity (and because a cover letter is missing): how did you stumble over these? Coverity?

Quoted 7 lines

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

2018-05-31 05:15 UTC #8 lore raw

In reply to Stefan Beller

refs/packed-backend.c static int load_contents(struct snapshot *snapshot) · line 505

Quoted 12 lines · Stefan Beller

refs/packed-backend.c +1 -0

index cec3fb9e00f..d447a731da0 100644
@@ -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

2018-05-31 11:50 UTC #9 lore raw

In reply to Stefan Beller

Quoted 24 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>

---

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(+)

refs/packed-backend.c +1 -1

index cec3fb9e00f..d447a731da0 100644
@@ -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);
--

Quoted 2 lines

2.17.1.1185.g55be947832-goog

+1.

Thanks,
Michael

Johannes Schindelin

2018-05-31 12:06 UTC #10 lore raw

In reply to Stefan Beller

Hi Stefan,

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

Quoted 16 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>
---
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(+)
refs/packed-backend.c +0 -0
index cec3fb9e00f..d447a731da0 100644

refs/packed-backend.c static int load_contents(struct snapshot *snapshot) · line 502

Quoted 4 lines

refs/packed-backend.c +1 -0

@@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)
 	size = xsize_t(st.st_size);
 
 	if (!size) {
+		close(fd);

Good catch,
Dscho

Quoted 7 lines

refs/packed-backend.c +0 -0

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

Signature · 1 line

2.17.1.1185.g55be947832-goog