* [PATCH 1/3] refs/packed-backend.c: close fd of empty file
@ 2018-05-30 17:03 Stefan Beller
2018-05-30 17:03 ` [PATCH 2/3] sequencer.c: free author variable when merging fails Stefan Beller
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Stefan Beller @ 2018-05-30 17:03 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] sequencer.c: free author variable when merging fails
2018-05-30 17:03 [PATCH 1/3] refs/packed-backend.c: close fd of empty file Stefan Beller
@ 2018-05-30 17:03 ` Stefan Beller
2018-05-31 12:04 ` Johannes Schindelin
2018-05-30 17:03 ` [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Stefan Beller
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Stefan Beller @ 2018-05-30 17:03 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote
2018-05-30 17:03 [PATCH 1/3] refs/packed-backend.c: close fd of empty file Stefan Beller
2018-05-30 17:03 ` [PATCH 2/3] sequencer.c: free author variable when merging fails Stefan Beller
@ 2018-05-30 17:03 ` Stefan Beller
2018-05-31 12:07 ` Johannes Schindelin
2018-05-31 5:15 ` [PATCH 1/3] refs/packed-backend.c: close fd of empty file Jeff King
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Stefan Beller @ 2018-05-30 17:03 UTC (permalink / raw)
To: git; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file
2018-05-30 17:03 [PATCH 1/3] refs/packed-backend.c: close fd of empty file Stefan Beller
2018-05-30 17:03 ` [PATCH 2/3] sequencer.c: free author variable when merging fails Stefan Beller
2018-05-30 17:03 ` [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Stefan Beller
@ 2018-05-31 5:15 ` Jeff King
2018-05-31 11:49 ` Michael Haggerty
2018-05-31 12:06 ` Johannes Schindelin
4 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2018-05-31 5:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, Michael Haggerty
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file
2018-05-30 17:03 [PATCH 1/3] refs/packed-backend.c: close fd of empty file Stefan Beller
` (2 preceding siblings ...)
2018-05-31 5:15 ` [PATCH 1/3] refs/packed-backend.c: close fd of empty file Jeff King
@ 2018-05-31 11:49 ` Michael Haggerty
2018-05-31 12:06 ` Johannes Schindelin
4 siblings, 0 replies; 11+ messages in thread
From: Michael Haggerty @ 2018-05-31 11:49 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git Mailing List
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] sequencer.c: free author variable when merging fails
2018-05-30 17:03 ` [PATCH 2/3] sequencer.c: free author variable when merging fails Stefan Beller
@ 2018-05-31 12:04 ` Johannes Schindelin
2018-05-31 18:52 ` Stefan Beller
0 siblings, 1 reply; 11+ messages in thread
From: Johannes Schindelin @ 2018-05-31 12:04 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
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
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file
2018-05-30 17:03 [PATCH 1/3] refs/packed-backend.c: close fd of empty file Stefan Beller
` (3 preceding siblings ...)
2018-05-31 11:49 ` Michael Haggerty
@ 2018-05-31 12:06 ` Johannes Schindelin
4 siblings, 0 replies; 11+ messages in thread
From: Johannes Schindelin @ 2018-05-31 12:06 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, Michael Haggerty
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
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote
2018-05-30 17:03 ` [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Stefan Beller
@ 2018-05-31 12:07 ` Johannes Schindelin
2018-05-31 18:55 ` Stefan Beller
0 siblings, 1 reply; 11+ messages in thread
From: Johannes Schindelin @ 2018-05-31 12:07 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
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
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] sequencer.c: free author variable when merging fails
2018-05-31 12:04 ` Johannes Schindelin
@ 2018-05-31 18:52 ` Stefan Beller
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Beller @ 2018-05-31 18:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote
2018-05-31 12:07 ` Johannes Schindelin
@ 2018-05-31 18:55 ` Stefan Beller
2018-06-01 8:46 ` Johannes Schindelin
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Beller @ 2018-05-31 18:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote
2018-05-31 18:55 ` Stefan Beller
@ 2018-06-01 8:46 ` Johannes Schindelin
0 siblings, 0 replies; 11+ messages in thread
From: Johannes Schindelin @ 2018-06-01 8:46 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-06-01 8:47 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-30 17:03 [PATCH 1/3] refs/packed-backend.c: close fd of empty file Stefan Beller
2018-05-30 17:03 ` [PATCH 2/3] sequencer.c: free author variable when merging fails Stefan Beller
2018-05-31 12:04 ` Johannes Schindelin
2018-05-31 18:52 ` Stefan Beller
2018-05-30 17:03 ` [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Stefan Beller
2018-05-31 12:07 ` Johannes Schindelin
2018-05-31 18:55 ` Stefan Beller
2018-06-01 8:46 ` Johannes Schindelin
2018-05-31 5:15 ` [PATCH 1/3] refs/packed-backend.c: close fd of empty file Jeff King
2018-05-31 11:49 ` Michael Haggerty
2018-05-31 12:06 ` Johannes Schindelin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.