{"thread":{"id":"48604","subject":"[PATCH 1/3] refs/packed-backend.c: close fd of empty file","startedAt":"2018-05-30T17:03:10Z","lastAt":"2018-06-01T08:47:00Z","messageCount":11,"participants":["Stefan Beller","Jeff King","Michael Haggerty","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"348817","messageId":"20180530170302.191176-1-sbeller@google.com","threadId":"48604","inReplyTo":null,"subject":"[PATCH 1/3] refs/packed-backend.c: close fd of empty file","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T17:03:00Z","receivedAt":"2018-05-30T17:03:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis was an oversight in 01caf20d57a (load_contents(): don't try to mmap an\nempty file, 2018-01-24).\n\nThis and the following 2 patches apply on master.\n\n refs/packed-backend.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex cec3fb9e00f..d447a731da0 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)\n \tsize = xsize_t(st.st_size);\n \n \tif (!size) {\n+\t\tclose(fd);\n \t\treturn 0;\n \t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n \t\tsnapshot->buf = xmalloc(size);\n-- \n2.17.1.1185.g55be947832-goog\n\n"},{"id":"348818","messageId":"20180530170302.191176-2-sbeller@google.com","threadId":"48604","inReplyTo":"20180530170302.191176-1-sbeller@google.com","subject":"[PATCH 2/3] sequencer.c: free author variable when merging fails","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T17:03:01Z","receivedAt":"2018-05-30T17:03:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a\nlibified merge_recursive(), 2016-07-26)\n\n sequencer.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 72b4d8ecae3..5c93586cc1c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1771,8 +1771,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \telse if (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || command == TODO_REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t &head, &msgbuf, opts);\n-\t\tif (res < 0)\n+\t\tif (res < 0) {\n+\t\t\tfree(author);\n \t\t\treturn res;\n+\t\t}\n \t\tres |= write_message(msgbuf.buf, msgbuf.len,\n \t\t\t\t     git_path_merge_msg(), 0);\n \t} else {\n-- \n2.17.1.1185.g55be947832-goog\n\n"},{"id":"348819","messageId":"20180530170302.191176-3-sbeller@google.com","threadId":"48604","inReplyTo":"20180530170302.191176-1-sbeller@google.com","subject":"[PATCH 3/3] submodule--helper: plug mem leak in print_default_remote","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T17:03:02Z","receivedAt":"2018-05-30T17:03:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 7c3cd9dbeba..96024fee1b1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)\n \tif (remote)\n \t\tprintf(\"%s\\n\", remote);\n \n+\tfree(remote);\n \treturn 0;\n }\n \n-- \n2.17.1.1185.g55be947832-goog\n\n"},{"id":"348864","messageId":"20180531051546.GA17068@sigill.intra.peff.net","threadId":"48604","inReplyTo":"20180530170302.191176-1-sbeller@google.com","subject":"Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-31T05:15:46Z","receivedAt":"2018-05-31T05:15:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 30, 2018 at 10:03:00AM -0700, Stefan Beller wrote:\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index cec3fb9e00f..d447a731da0 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)\n>  \tsize = xsize_t(st.st_size);\n>  \n>  \tif (!size) {\n> +\t\tclose(fd);\n>  \t\treturn 0;\n>  \t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n>  \t\tsnapshot->buf = xmalloc(size);\n\nYep, this is definitely correct. Good catch.\n\n-Peff\n"},{"id":"348890","messageId":"CAMy9T_Hc_C-bbzcKBq9HJ4ufzcNJa5NtFQb1h2Uwpe6QL2XRCA@mail.gmail.com","threadId":"48604","inReplyTo":"20180530170302.191176-1-sbeller@google.com","subject":"Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2018-05-31T11:49:58Z","receivedAt":"2018-05-31T11:50:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Wed, May 30, 2018 at 7:03 PM, Stefan Beller <sbeller@google.com> wrote:\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> This was an oversight in 01caf20d57a (load_contents(): don't try to mmap an\n> empty file, 2018-01-24).\n>\n> This and the following 2 patches apply on master.\n>\n>  refs/packed-backend.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index cec3fb9e00f..d447a731da0 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)\n>         size = xsize_t(st.st_size);\n>\n>         if (!size) {\n> +               close(fd);\n>                 return 0;\n>         } else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n>                 snapshot->buf = xmalloc(size);\n> --\n> 2.17.1.1185.g55be947832-goog\n>\n\n+1.\n\nThanks,\nMichael\n"},{"id":"348891","messageId":"nycvar.QRO.7.76.6.1805311402210.82@tvgsbejvaqbjf.bet","threadId":"48604","inReplyTo":"20180530170302.191176-2-sbeller@google.com","subject":"Re: [PATCH 2/3] sequencer.c: free author variable when merging fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-31T12:04:46Z","receivedAt":"2018-05-31T12:04:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Wed, 30 May 2018, Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n> This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a\n> libified merge_recursive(), 2016-07-26)\n\nNo, it was not deliberate. It was inadvertent, most likely ;-)\n\n>  sequencer.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 72b4d8ecae3..5c93586cc1c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1771,8 +1771,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \telse if (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || command == TODO_REVERT) {\n>  \t\tres = do_recursive_merge(base, next, base_label, next_label,\n>  \t\t\t\t\t &head, &msgbuf, opts);\n> -\t\tif (res < 0)\n> +\t\tif (res < 0) {\n> +\t\t\tfree(author);\n>  \t\t\treturn res;\n\nWhy not `goto leave;` instead? I wonder what is happening to the commit\nmessage: can we be certain at this point that it was not set yet? And\nalso: should we call `update_abort_safety_file()`?\n\nCiao,\nDscho\n\n> +\t\t}\n>  \t\tres |= write_message(msgbuf.buf, msgbuf.len,\n>  \t\t\t\t     git_path_merge_msg(), 0);\n>  \t} else {\n> -- \n> 2.17.1.1185.g55be947832-goog\n> \n> \n"},{"id":"348892","messageId":"nycvar.QRO.7.76.6.1805311406050.82@tvgsbejvaqbjf.bet","threadId":"48604","inReplyTo":"20180530170302.191176-1-sbeller@google.com","subject":"Re: [PATCH 1/3] refs/packed-backend.c: close fd of empty file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-31T12:06:49Z","receivedAt":"2018-05-31T12:06:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nI am Cc:ing Michael, the original author of the fixed commit.\n\nOn Wed, 30 May 2018, Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n> This was an oversight in 01caf20d57a (load_contents(): don't try to mmap an\n> empty file, 2018-01-24).\n> \n> This and the following 2 patches apply on master.\n> \n>  refs/packed-backend.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index cec3fb9e00f..d447a731da0 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -499,6 +499,7 @@ static int load_contents(struct snapshot *snapshot)\n>  \tsize = xsize_t(st.st_size);\n>  \n>  \tif (!size) {\n> +\t\tclose(fd);\n\nGood catch,\nDscho\n\n>  \t\treturn 0;\n>  \t} else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) {\n>  \t\tsnapshot->buf = xmalloc(size);\n> -- \n> 2.17.1.1185.g55be947832-goog\n> \n> \n"},{"id":"348893","messageId":"nycvar.QRO.7.76.6.1805311407140.82@tvgsbejvaqbjf.bet","threadId":"48604","inReplyTo":"20180530170302.191176-3-sbeller@google.com","subject":"Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-31T12:07:42Z","receivedAt":"2018-05-31T12:07:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Wed, 30 May 2018, Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/submodule--helper.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 7c3cd9dbeba..96024fee1b1 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)\n>  \tif (remote)\n>  \t\tprintf(\"%s\\n\", remote);\n>  \n> +\tfree(remote);\n\nMakes sense.\n\nOut of curiosity (and because a cover letter is missing): how did you\nstumble over these? Coverity?\n\nCiao,\nDscho\n\n>  \treturn 0;\n>  }\n>  \n> -- \n> 2.17.1.1185.g55be947832-goog\n> \n> \n"},{"id":"348920","messageId":"CAGZ79kbhYSuBEEgALAmFT6Cf4+wmqFO_cMScBDs8CcgFwckXQg@mail.gmail.com","threadId":"48604","inReplyTo":"nycvar.QRO.7.76.6.1805311402210.82@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/3] sequencer.c: free author variable when merging fails","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-31T18:52:52Z","receivedAt":"2018-05-31T18:52:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 31, 2018 at 5:04 AM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Stefan,\n>\n> On Wed, 30 May 2018, Stefan Beller wrote:\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>\n>> This was a deliberate oversight in f241ff0d0a9 (prepare the builtins for a\n>> libified merge_recursive(), 2016-07-26)\n>\n> No, it was not deliberate. It was inadvertent, most likely ;-)\n\nok, I am not just bad at writing commit messages, but\nalso bad at reading other peoples commit messages. ;)\n\n\"As this patch is already complex enough, we\nleave that change for a later patch.\" is what lead me to\nbelieve it was deliberate.\n\n>> -             if (res < 0)\n>> +             if (res < 0) {\n>> +                     free(author);\n>>                       return res;\n>\n> Why not `goto leave;` instead? I wonder what is happening to the commit\n> message: can we be certain at this point that it was not set yet? And\n> also: should we call `update_abort_safety_file()`?\n\nI think so, but wasn't sure. I wrote these patches before\nmy usual morning routine. I'll change that.\n\nThanks,\nStefan\n"},{"id":"348921","messageId":"CAGZ79kZcjojZDoyhZovQmBLxUcMgECih4iUAYYFw4o_EeB3m2g@mail.gmail.com","threadId":"48604","inReplyTo":"nycvar.QRO.7.76.6.1805311407140.82@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-31T18:55:15Z","receivedAt":"2018-05-31T18:55:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Stefan,\n>\n> On Wed, 30 May 2018, Stefan Beller wrote:\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  builtin/submodule--helper.c | 1 +\n>>  1 file changed, 1 insertion(+)\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 7c3cd9dbeba..96024fee1b1 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)\n>>       if (remote)\n>>               printf(\"%s\\n\", remote);\n>>\n>> +     free(remote);\n>\n> Makes sense.\n>\n> Out of curiosity (and because a cover letter is missing): how did you\n> stumble over these? Coverity?\n\nYes I found them on coverity as I wanted to find out how bad their\nfalse positives are these days. So I looked at the most recent findings.\n\nI somehow imagined that we could redefine the _INIT macros which\nusually lead to false positives (just alloc&UNLEAK memory instead of\npointing them all at the same memory for _INIT), but that experiment\ndid not work out.\n\nThanks,\nStefan\n"},{"id":"348990","messageId":"nycvar.QRO.7.76.6.1806011046120.82@tvgsbejvaqbjf.bet","threadId":"48604","inReplyTo":"CAGZ79kZcjojZDoyhZovQmBLxUcMgECih4iUAYYFw4o_EeB3m2g@mail.gmail.com","subject":"Re: [PATCH 3/3] submodule--helper: plug mem leak in print_default_remote","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-01T08:46:55Z","receivedAt":"2018-06-01T08:47:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Thu, 31 May 2018, Stefan Beller wrote:\n\n> On Thu, May 31, 2018 at 5:07 AM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> > Hi Stefan,\n> >\n> > On Wed, 30 May 2018, Stefan Beller wrote:\n> >\n> >> Signed-off-by: Stefan Beller <sbeller@google.com>\n> >> ---\n> >>  builtin/submodule--helper.c | 1 +\n> >>  1 file changed, 1 insertion(+)\n> >>\n> >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> >> index 7c3cd9dbeba..96024fee1b1 100644\n> >> --- a/builtin/submodule--helper.c\n> >> +++ b/builtin/submodule--helper.c\n> >> @@ -63,6 +63,7 @@ static int print_default_remote(int argc, const char **argv, const char *prefix)\n> >>       if (remote)\n> >>               printf(\"%s\\n\", remote);\n> >>\n> >> +     free(remote);\n> >\n> > Makes sense.\n> >\n> > Out of curiosity (and because a cover letter is missing): how did you\n> > stumble over these? Coverity?\n> \n> Yes I found them on coverity as I wanted to find out how bad their\n> false positives are these days. So I looked at the most recent findings.\n> \n> I somehow imagined that we could redefine the _INIT macros which\n> usually lead to false positives (just alloc&UNLEAK memory instead of\n> pointing them all at the same memory for _INIT), but that experiment\n> did not work out.\n\nYes, those many, many, *many* false positives really drown out the benefit\nof Coverity for me. It takes all the fun out of looking for quick bug\nfixes.\n\nCiao,\nDscho\n"}]}