# [PATCH 1/3] refs/packed-backend.c: close fd of empty file 11 messages from 2018-05-30 to 2018-06-01. Participants: Stefan Beller, Jeff King, Michael Haggerty, Johannes Schindelin. Thread: https://gitlist.dev/t/48604 ## Stefan Beller, 2018-05-30 17:03 Subject: [PATCH 1/3] refs/packed-backend.c: close fd of empty file Message-ID: <20180530170302.191176-1-sbeller@google.com> URL: https://gitlist.dev/e/20180530170302.191176-1-sbeller%40google.com ``` Signed-off-by: Stefan Beller --- 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 ``` ## Stefan Beller, 2018-05-30 17:03 Subject: [PATCH 2/3] sequencer.c: free author variable when merging fails Message-ID: <20180530170302.191176-2-sbeller@google.com> URL: https://gitlist.dev/e/20180530170302.191176-2-sbeller%40google.com In-Reply-To: <20180530170302.191176-1-sbeller@google.com> ``` Signed-off-by: Stefan Beller --- 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 ``` ## Stefan Beller, 2018-05-30 17:03 Subject: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Message-ID: <20180530170302.191176-3-sbeller@google.com> URL: https://gitlist.dev/e/20180530170302.191176-3-sbeller%40google.com In-Reply-To: <20180530170302.191176-1-sbeller@google.com> ``` Signed-off-by: Stefan Beller --- 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 ``` ## Jeff King, 2018-05-31 05:15 Subject: Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file Message-ID: <20180531051546.GA17068@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180531051546.GA17068%40sigill.intra.peff.net In-Reply-To: <20180530170302.191176-1-sbeller@google.com> ``` 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 ``` ## Michael Haggerty, 2018-05-31 11:49 Subject: Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file Message-ID: URL: https://gitlist.dev/e/CAMy9T_Hc_C-bbzcKBq9HJ4ufzcNJa5NtFQb1h2Uwpe6QL2XRCA%40mail.gmail.com In-Reply-To: <20180530170302.191176-1-sbeller@google.com> ``` On Wed, May 30, 2018 at 7:03 PM, Stefan Beller wrote: > Signed-off-by: Stefan Beller > --- > > 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, 2018-05-31 12:04 Subject: Re: [PATCH 2/3] sequencer.c: free author variable when merging fails Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1805311402210.82%40tvgsbejvaqbjf.bet In-Reply-To: <20180530170302.191176-2-sbeller@google.com> ``` Hi Stefan, On Wed, 30 May 2018, Stefan Beller wrote: > Signed-off-by: Stefan Beller > --- > > 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 > > ``` ## Johannes Schindelin, 2018-05-31 12:06 Subject: Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1805311406050.82%40tvgsbejvaqbjf.bet In-Reply-To: <20180530170302.191176-1-sbeller@google.com> ``` 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 > --- > > 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 > > ``` ## Johannes Schindelin, 2018-05-31 12:07 Subject: Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1805311407140.82%40tvgsbejvaqbjf.bet In-Reply-To: <20180530170302.191176-3-sbeller@google.com> ``` Hi Stefan, On Wed, 30 May 2018, Stefan Beller wrote: > Signed-off-by: Stefan Beller > --- > 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 > > ``` ## Stefan Beller, 2018-05-31 18:52 Subject: Re: [PATCH 2/3] sequencer.c: free author variable when merging fails Message-ID: URL: https://gitlist.dev/e/CAGZ79kbhYSuBEEgALAmFT6Cf4%2BwmqFO_cMScBDs8CcgFwckXQg%40mail.gmail.com In-Reply-To: ``` On Thu, May 31, 2018 at 5:04 AM, Johannes Schindelin wrote: > Hi Stefan, > > On Wed, 30 May 2018, Stefan Beller wrote: > >> Signed-off-by: Stefan Beller >> --- >> >> 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 ``` ## Stefan Beller, 2018-05-31 18:55 Subject: Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Message-ID: URL: https://gitlist.dev/e/CAGZ79kZcjojZDoyhZovQmBLxUcMgECih4iUAYYFw4o_EeB3m2g%40mail.gmail.com In-Reply-To: ``` On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin wrote: > Hi Stefan, > > On Wed, 30 May 2018, Stefan Beller wrote: > >> Signed-off-by: Stefan Beller >> --- >> 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, 2018-06-01 08:46 Subject: Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1806011046120.82%40tvgsbejvaqbjf.bet In-Reply-To: ``` Hi Stefan, On Thu, 31 May 2018, Stefan Beller wrote: > On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin > wrote: > > Hi Stefan, > > > > On Wed, 30 May 2018, Stefan Beller wrote: > > > >> Signed-off-by: Stefan Beller > >> --- > >> 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 ```