# [PATCH 0/6] Some cleanups 24 messages from 2016-03-30 to 2016-03-30. Participants: Stefan Beller, Junio C Hamano, Eric Sunshine, Jeff King. Thread: https://gitlist.dev/t/41867 ## Stefan Beller, 2016-03-30 00:38 Subject: [PATCH 0/6] Some cleanups Message-ID: <1459298333-21899-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459298333-21899-1-git-send-email-sbeller%40google.com ``` One of my first patches to Git were cleanup patches, and I fell back to my old pattern here, while thinking on how to write better commit messages for the submodule bugfixes I currently have in flight. Just some one liners to not leak memory or file descriptors. They are bundled as a series, but no patch relies on any predessor. This applies on v2.8.0. Thanks, Stefan Stefan Beller (6): path.c: allocate enough memory for string imap-send.c, cram: allocate enough memory for null terminated string notes: don't leak memory in git_config_get_notes_strategy abbrev_sha1_in_line: don't leak memory bundle: don't leak an fd in case of early return credential-cache, send_request: close fd when done builtin/notes.c | 1 + bundle.c | 10 ++++++++-- credential-cache.c | 1 + imap-send.c | 2 +- path.c | 2 +- wt-status.c | 2 +- 6 files changed, 13 insertions(+), 5 deletions(-) -- 2.8.0.8.g27a27a6.dirty ``` ## Stefan Beller, 2016-03-30 00:38 Subject: [PATCH 1/6] path.c: allocate enough memory for string Message-ID: <1459298333-21899-2-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459298333-21899-2-git-send-email-sbeller%40google.com In-Reply-To: <1459298333-21899-1-git-send-email-sbeller@google.com> ``` `strlen` returns the length of a string without the terminating null byte. To make sure enough memory is allocated we need to pass `strlen(..) + 1` to the allocation function. Signed-off-by: Stefan Beller --- path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/path.c b/path.c index 969b494..0ae8af5 100644 --- a/path.c +++ b/path.c @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value) struct trie *new_node = xcalloc(1, sizeof(*new_node)); new_node->len = strlen(key); if (new_node->len) { - new_node->contents = xmalloc(new_node->len); + new_node->contents = xmalloc(new_node->len + 1); memcpy(new_node->contents, key, new_node->len); } new_node->value = value; -- 2.8.0.8.g27a27a6.dirty ``` ## Stefan Beller, 2016-03-30 00:38 Subject: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string Message-ID: <1459298333-21899-3-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459298333-21899-3-git-send-email-sbeller%40google.com In-Reply-To: <1459298333-21899-1-git-send-email-sbeller@google.com> ``` `strlen` returns the length of a string without the terminating null byte. To make sure enough memory is allocated we need to pass `strlen(..) + 1` to the allocation function. Signed-off-by: Stefan Beller --- imap-send.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap-send.c b/imap-send.c index 2c52027..f7e9909 100644 --- a/imap-send.c +++ b/imap-send.c @@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char *user, const char *pass) * enough upper bound for challenge (decoded result). */ encoded_len = strlen(challenge_64); - challenge = xmalloc(encoded_len); + challenge = xmalloc(encoded_len + 1); decoded_len = EVP_DecodeBlock((unsigned char *)challenge, (unsigned char *)challenge_64, encoded_len); if (decoded_len < 0) -- 2.8.0.8.g27a27a6.dirty ``` ## Stefan Beller, 2016-03-30 00:38 Subject: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy Message-ID: <1459298333-21899-4-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459298333-21899-4-git-send-email-sbeller%40google.com In-Reply-To: <1459298333-21899-1-git-send-email-sbeller@google.com> ``` `value` is just a temporary scratchpad, so we need to make sure it doesn't leak. It is xstrdup'd in `git_config_get_string_const` and `parse_notes_merge_strategy` just compares the string against predefined values, so no need to keep it around longer. Signed-off-by: Stefan Beller --- builtin/notes.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/notes.c b/builtin/notes.c index ed6f222..d6bab17 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key, if (parse_notes_merge_strategy(value, strategy)) git_die_config(key, "unknown notes merge strategy %s", value); + free((void*)value); return 0; } -- 2.8.0.8.g27a27a6.dirty ``` ## Stefan Beller, 2016-03-30 00:38 Subject: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory Message-ID: <1459298333-21899-5-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459298333-21899-5-git-send-email-sbeller%40google.com In-Reply-To: <1459298333-21899-1-git-send-email-sbeller@google.com> ``` `split` is of type `struct strbuf **` and just before the new free, we release the inner strbufs. Make sure to also release the memory containing the pointers to the individual strbufs. Signed-off-by: Stefan Beller --- wt-status.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wt-status.c b/wt-status.c index ef74864..a78cfc0 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line) } for (i = 0; split[i]; i++) strbuf_release(split[i]); - + free(split); } static void read_rebase_todolist(const char *fname, struct string_list *lines) -- 2.8.0.8.g27a27a6.dirty ``` ## Stefan Beller, 2016-03-30 00:38 Subject: [PATCH 5/6] bundle: don't leak an fd in case of early return Message-ID: <1459298333-21899-6-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459298333-21899-6-git-send-email-sbeller%40google.com In-Reply-To: <1459298333-21899-1-git-send-email-sbeller@google.com> ``` In successful operation `write_pack_data` will close the `bundle_fd`, but when we exit early, we need to take care of the file descriptor ourselves. Signed-off-by: Stefan Beller --- bundle.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bundle.c b/bundle.c index 506ac49..04d62af 100644 --- a/bundle.c +++ b/bundle.c @@ -434,8 +434,11 @@ int create_bundle(struct bundle_header *header, const char *path, init_revisions(&revs, NULL); /* write prerequisites */ - if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) + if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) { + if (!bundle_to_stdout) + close(bundle_fd); return -1; + } argc = setup_revisions(argc, argv, &revs, NULL); @@ -447,8 +450,11 @@ int create_bundle(struct bundle_header *header, const char *path, ref_count = write_bundle_refs(bundle_fd, &revs); if (!ref_count) die(_("Refusing to create empty bundle.")); - else if (ref_count < 0) + else if (ref_count < 0) { + if (!bundle_to_stdout) + close(bundle_fd); return -1; + } /* write pack */ if (write_pack_data(bundle_fd, &revs)) -- 2.8.0.8.g27a27a6.dirty ``` ## Stefan Beller, 2016-03-30 00:38 Subject: [PATCH 6/6] credential-cache, send_request: close fd when done Message-ID: <1459298333-21899-7-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1459298333-21899-7-git-send-email-sbeller%40google.com In-Reply-To: <1459298333-21899-1-git-send-email-sbeller@google.com> ``` No need to keep it open any further. Signed-off-by: Stefan Beller --- credential-cache.c | 1 + 1 file changed, 1 insertion(+) diff --git a/credential-cache.c b/credential-cache.c index f4afdc6..86e21de 100644 --- a/credential-cache.c +++ b/credential-cache.c @@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct strbuf *out) write_or_die(1, in, r); got_data = 1; } + close(fd); return got_data; } -- 2.8.0.8.g27a27a6.dirty ``` ## Junio C Hamano, 2016-03-30 00:56 Subject: Re: [PATCH 1/6] path.c: allocate enough memory for string Message-ID: URL: https://gitlist.dev/e/xmqq7fgkvl47.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459298333-21899-2-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > `strlen` returns the length of a string without the terminating null byte. > To make sure enough memory is allocated we need to pass `strlen(..) + 1` > to the allocation function. > > Signed-off-by: Stefan Beller > --- > path.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/path.c b/path.c > index 969b494..0ae8af5 100644 > --- a/path.c > +++ b/path.c > @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value) > struct trie *new_node = xcalloc(1, sizeof(*new_node)); > new_node->len = strlen(key); > if (new_node->len) { > - new_node->contents = xmalloc(new_node->len); > + new_node->contents = xmalloc(new_node->len + 1); > memcpy(new_node->contents, key, new_node->len); > } This structure looks like a counted string that does not want to have a terminating NUL after the contents, judging from the way memcpy() copies only len and not len+1. Did I write this (wondering why this was addressed to me)? > new_node->value = value; ``` ## Eric Sunshine, 2016-03-30 00:57 Subject: Re: [PATCH 1/6] path.c: allocate enough memory for string Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSqC1muy%2Bvcmpkp9uM9T3CC4pa5jWGUot_FtrVshEAUEw%40mail.gmail.com In-Reply-To: <1459298333-21899-2-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote: > `strlen` returns the length of a string without the terminating null byte. > To make sure enough memory is allocated we need to pass `strlen(..) + 1` > to the allocation function. > > Signed-off-by: Stefan Beller > --- > diff --git a/path.c b/path.c > @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value) > struct trie *new_node = xcalloc(1, sizeof(*new_node)); > new_node->len = strlen(key); > if (new_node->len) { > - new_node->contents = xmalloc(new_node->len); > + new_node->contents = xmalloc(new_node->len + 1); > memcpy(new_node->contents, key, new_node->len); Huh? This is a trie. It never accesses 'contents' as a NUL-terminated string. Plus, no NUL is ever even copied, thus this is just overallocating. How is this an improvement? > } > new_node->value = value; > -- ``` ## Eric Sunshine, 2016-03-30 01:02 Subject: Re: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSX4Jye60CKxR%3DOjhtAai09WSMNMOau2WHUxgXbKX2aDQ%40mail.gmail.com In-Reply-To: <1459298333-21899-3-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote: > `strlen` returns the length of a string without the terminating null byte. > To make sure enough memory is allocated we need to pass `strlen(..) + 1` > to the allocation function. > > Signed-off-by: Stefan Beller > --- > imap-send.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/imap-send.c b/imap-send.c > index 2c52027..f7e9909 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char *user, const char *pass) > * enough upper bound for challenge (decoded result). > */ > encoded_len = strlen(challenge_64); > - challenge = xmalloc(encoded_len); > + challenge = xmalloc(encoded_len + 1); 'challenge' is never used as a NUL-terminated string, and EVP_DecodeBlock() is not advertised as adding a terminating NUL, so why is this an improvement? > decoded_len = EVP_DecodeBlock((unsigned char *)challenge, > (unsigned char *)challenge_64, encoded_len); > if (decoded_len < 0) > -- ``` ## Jeff King, 2016-03-30 01:07 Subject: Re: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string Message-ID: <20160330010756.GA2237@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160330010756.GA2237%40sigill.intra.peff.net In-Reply-To: <1459298333-21899-3-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 05:38:49PM -0700, Stefan Beller wrote: > `strlen` returns the length of a string without the terminating null byte. > To make sure enough memory is allocated we need to pass `strlen(..) + 1` > to the allocation function. > > Signed-off-by: Stefan Beller > --- > imap-send.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/imap-send.c b/imap-send.c > index 2c52027..f7e9909 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char *user, const char *pass) > * enough upper bound for challenge (decoded result). > */ > encoded_len = strlen(challenge_64); > - challenge = xmalloc(encoded_len); > + challenge = xmalloc(encoded_len + 1); > decoded_len = EVP_DecodeBlock((unsigned char *)challenge, > (unsigned char *)challenge_64, encoded_len); > if (decoded_len < 0) Others pointed out that patches 1 and 2 here probably don't need the extra byte. But as a side note, for any cases that do, please use xmallocz() instead of manually adding 1 to the length. Even if you don't need the final byte to be NUL, it checks for integer overflow. -Peff ``` ## Jeff King, 2016-03-30 01:11 Subject: Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory Message-ID: <20160330011111.GB2237@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160330011111.GB2237%40sigill.intra.peff.net In-Reply-To: <1459298333-21899-5-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote: > `split` is of type `struct strbuf **` and just before the new free, > we release the inner strbufs. Make sure to also release the memory > containing the pointers to the individual strbufs. > > Signed-off-by: Stefan Beller > --- > wt-status.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/wt-status.c b/wt-status.c > index ef74864..a78cfc0 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line) > } > for (i = 0; split[i]; i++) > strbuf_release(split[i]); > - > + free(split); > } I think this can just combine with the for-loop above to become strbuf_list_free(). -Peff ``` ## Eric Sunshine, 2016-03-30 01:11 Subject: Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQxtFDY8kAx-UD_dYatTJJnx260CaY5Ke3yDOS666YEtA%40mail.gmail.com In-Reply-To: <1459298333-21899-4-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote: > `value` is just a temporary scratchpad, so we need to make sure it doesn't > leak. It is xstrdup'd in `git_config_get_string_const` and > `parse_notes_merge_strategy` just compares the string against predefined > values, so no need to keep it around longer. > > Signed-off-by: Stefan Beller > --- > diff --git a/builtin/notes.c b/builtin/notes.c > @@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key, > if (parse_notes_merge_strategy(value, strategy)) > git_die_config(key, "unknown notes merge strategy %s", value); > > + free((void*)value); I wonder if the intent would be clearer if you gave 'value' type 'char *' rather than 'const char *', and called git_config_get_string() rather than git_config_get_string_const(). > return 0; > } ``` ## Jeff King, 2016-03-30 01:13 Subject: Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy Message-ID: <20160330011334.GC2237@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160330011334.GC2237%40sigill.intra.peff.net In-Reply-To: <1459298333-21899-4-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 05:38:50PM -0700, Stefan Beller wrote: > `value` is just a temporary scratchpad, so we need to make sure it doesn't > leak. It is xstrdup'd in `git_config_get_string_const` and > `parse_notes_merge_strategy` just compares the string against predefined > values, so no need to keep it around longer. > > Signed-off-by: Stefan Beller > --- > builtin/notes.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/notes.c b/builtin/notes.c > index ed6f222..d6bab17 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key, > if (parse_notes_merge_strategy(value, strategy)) > git_die_config(key, "unknown notes merge strategy %s", value); > > + free((void*)value); > return 0; > } I don't think this is wrong, but would it perhaps be simpler to call git_config_get_value() in the first place, which does not make a copy of the string? -Peff ``` ## Jeff King, 2016-03-30 01:17 Subject: Re: [PATCH 5/6] bundle: don't leak an fd in case of early return Message-ID: <20160330011743.GD2237@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160330011743.GD2237%40sigill.intra.peff.net In-Reply-To: <1459298333-21899-6-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 05:38:52PM -0700, Stefan Beller wrote: > In successful operation `write_pack_data` will close the `bundle_fd`, > but when we exit early, we need to take care of the file descriptor > ourselves. > > Signed-off-by: Stefan Beller > --- > bundle.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/bundle.c b/bundle.c > index 506ac49..04d62af 100644 > --- a/bundle.c > +++ b/bundle.c > @@ -434,8 +434,11 @@ int create_bundle(struct bundle_header *header, const char *path, > init_revisions(&revs, NULL); > > /* write prerequisites */ > - if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) > + if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) { > + if (!bundle_to_stdout) > + close(bundle_fd); > return -1; > + } Makes sense. Should we also be rolling back the lock file? It happens automatically at program exit, of course, but we are in library code here that should not rely on that. > @@ -447,8 +450,11 @@ int create_bundle(struct bundle_header *header, const char *path, > ref_count = write_bundle_refs(bundle_fd, &revs); > if (!ref_count) > die(_("Refusing to create empty bundle.")); > - else if (ref_count < 0) > + else if (ref_count < 0) { > + if (!bundle_to_stdout) > + close(bundle_fd); > return -1; > + } Ditto here, and in the error return from write_pack_data(). There are enough spots that it may be worth setting up a "goto err" path. -Peff ``` ## Jeff King, 2016-03-30 01:20 Subject: Re: [PATCH 6/6] credential-cache, send_request: close fd when done Message-ID: <20160330011959.GE2237@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160330011959.GE2237%40sigill.intra.peff.net In-Reply-To: <1459298333-21899-7-git-send-email-sbeller@google.com> ``` On Tue, Mar 29, 2016 at 05:38:53PM -0700, Stefan Beller wrote: > No need to keep it open any further. > > Signed-off-by: Stefan Beller > --- > credential-cache.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/credential-cache.c b/credential-cache.c > index f4afdc6..86e21de 100644 > --- a/credential-cache.c > +++ b/credential-cache.c > @@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct strbuf *out) > write_or_die(1, in, r); > got_data = 1; > } > + close(fd); > return got_data; > } Looks good. I think nobody ever noticed because credential-cache basically sends off the request and then exits. -Peff ``` ## Eric Sunshine, 2016-03-30 01:30 Subject: Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR%3DZc2QAQTPAqTt4M-nJRw4StCoqT2hYtmHOvi6LseN_A%40mail.gmail.com In-Reply-To: <20160330011111.GB2237@sigill.intra.peff.net> ``` On Tue, Mar 29, 2016 at 9:11 PM, Jeff King wrote: > On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote: >> `split` is of type `struct strbuf **` and just before the new free, >> we release the inner strbufs. Make sure to also release the memory >> containing the pointers to the individual strbufs. >> >> Signed-off-by: Stefan Beller >> --- >> diff --git a/wt-status.c b/wt-status.c >> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line) >> } >> for (i = 0; split[i]; i++) >> strbuf_release(split[i]); >> - >> + free(split); >> } > > I think this can just combine with the for-loop above to become > strbuf_list_free(). The implementation of strbuf_list_free() is this: struct strbuf **s = sbs; while (*s) { strbuf_release(*s); free(*s++); } free(sbs); which means that wt-status.c is leaking not only 'split', but also each element of split[], right? ``` ## Jeff King, 2016-03-30 01:31 Subject: Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory Message-ID: <20160330013127.GA6680@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160330013127.GA6680%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote: > On Tue, Mar 29, 2016 at 9:11 PM, Jeff King wrote: > > On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote: > >> `split` is of type `struct strbuf **` and just before the new free, > >> we release the inner strbufs. Make sure to also release the memory > >> containing the pointers to the individual strbufs. > >> > >> Signed-off-by: Stefan Beller > >> --- > >> diff --git a/wt-status.c b/wt-status.c > >> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line) > >> } > >> for (i = 0; split[i]; i++) > >> strbuf_release(split[i]); > >> - > >> + free(split); > >> } > > > > I think this can just combine with the for-loop above to become > > strbuf_list_free(). > > The implementation of strbuf_list_free() is this: > > struct strbuf **s = sbs; > while (*s) { > strbuf_release(*s); > free(*s++); > } > free(sbs); > > which means that wt-status.c is leaking not only 'split', but also > each element of split[], right? Yeah, I didn't notice that, but I think you're right. -Peff ``` ## Stefan Beller, 2016-03-30 16:41 Subject: Re: [PATCH 1/6] path.c: allocate enough memory for string Message-ID: URL: https://gitlist.dev/e/CAGZ79kb6dsaSxQ%2BHaYODBurnFz3MbsisSNkdrckFZLvSCvS-JQ%40mail.gmail.com In-Reply-To: ``` On Tue, Mar 29, 2016 at 5:57 PM, Eric Sunshine wrote: > On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote: >> `strlen` returns the length of a string without the terminating null byte. >> To make sure enough memory is allocated we need to pass `strlen(..) + 1` >> to the allocation function. >> >> Signed-off-by: Stefan Beller >> --- >> diff --git a/path.c b/path.c >> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value) >> struct trie *new_node = xcalloc(1, sizeof(*new_node)); >> new_node->len = strlen(key); >> if (new_node->len) { >> - new_node->contents = xmalloc(new_node->len); >> + new_node->contents = xmalloc(new_node->len + 1); >> memcpy(new_node->contents, key, new_node->len); > > Huh? This is a trie. It never accesses 'contents' as a NUL-terminated > string. Plus, no NUL is ever even copied, thus this is just > overallocating. How is this an improvement? By using strlen, I assumed it was a standard C string. I missed that, though. > >> } >> new_node->value = value; >> -- ``` ## Junio C Hamano, 2016-03-30 17:06 Subject: Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory Message-ID: URL: https://gitlist.dev/e/xmqqzitfuc7y.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160330013127.GA6680@sigill.intra.peff.net> ``` Jeff King writes: > On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote: > >> The implementation of strbuf_list_free() is this: >> >> struct strbuf **s = sbs; >> while (*s) { >> strbuf_release(*s); >> free(*s++); >> } >> free(sbs); >> >> which means that wt-status.c is leaking not only 'split', but also >> each element of split[], right? > > Yeah, I didn't notice that, but I think you're right. Correct. I suspect that we would be better off in the longer term if we made conscious effort to deprecate and eradicate the use of strbuf_split* API functions. They are easy to write crappy code with, inefficient, and error prone. You would rarely need to have N resulting pieces as strbufs to be easily manipulatable [*1*]. The function can be written by not using the "split and then rebuild" pattern, perhaps like so, and the result is even easier to read and understand, I would say. A sample rewrite is attached at the end. [Footnote] *1* I wouldn't be this harsh if the function were to fill an array of pointers or offsets into the original string. wt-status.c | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/wt-status.c b/wt-status.c index ef74864..4886c35 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1037,35 +1037,30 @@ static int split_commit_in_progress(struct wt_status *s) */ static void abbrev_sha1_in_line(struct strbuf *line) { - struct strbuf **split; - int i; + const char *cp, *ep, *abbrev; + unsigned char sha1[20]; - if (starts_with(line->buf, "exec ") || - starts_with(line->buf, "x ")) + /* the oddball "exec" does not have 40-hex as the second token */ + if (starts_with(line->buf, "exec ") || starts_with(line->buf, "x ")) return; - split = strbuf_split_max(line, ' ', 3); - if (split[0] && split[1]) { - unsigned char sha1[20]; - const char *abbrev; + /* find the second token on the line */ + cp = strchr(line->buf, ' '); + if (!cp) + return; + cp++; + ep = strchr(cp, ' '); + if (!ep) + return; - /* - * strbuf_split_max left a space. Trim it and re-add - * it after abbreviation. - */ - strbuf_trim(split[1]); - if (!get_sha1(split[1]->buf, sha1)) { - abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV); - strbuf_reset(split[1]); - strbuf_addf(split[1], "%s ", abbrev); - strbuf_reset(line); - for (i = 0; split[i]; i++) - strbuf_addf(line, "%s", split[i]->buf); - } - } - for (i = 0; split[i]; i++) - strbuf_release(split[i]); + /* is it 40-hex? */ + if (ep - cp != GIT_SHA1_HEXSZ || get_sha1_hex(cp, sha1)) + return; + /* replace it with its abbreviation */ + abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV); + strbuf_splice(line, cp - line->buf, GIT_SHA1_HEXSZ, + abbrev, strlen(abbrev)); } static void read_rebase_todolist(const char *fname, struct string_list *lines) ``` ## Junio C Hamano, 2016-03-30 17:16 Subject: Re: [PATCH 1/6] path.c: allocate enough memory for string Message-ID: URL: https://gitlist.dev/e/xmqqr3erubs2.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > On Tue, Mar 29, 2016 at 5:57 PM, Eric Sunshine wrote: >> On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller wrote: >>> `strlen` returns the length of a string without the terminating null byte. >>> To make sure enough memory is allocated we need to pass `strlen(..) + 1` >>> to the allocation function. >>> >>> Signed-off-by: Stefan Beller >>> --- >>> diff --git a/path.c b/path.c >>> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void *value) >>> struct trie *new_node = xcalloc(1, sizeof(*new_node)); >>> new_node->len = strlen(key); >>> if (new_node->len) { >>> - new_node->contents = xmalloc(new_node->len); >>> + new_node->contents = xmalloc(new_node->len + 1); >>> memcpy(new_node->contents, key, new_node->len); >> >> Huh? This is a trie. It never accesses 'contents' as a NUL-terminated >> string. Plus, no NUL is ever even copied, thus this is just >> overallocating. How is this an improvement? > > By using strlen, I assumed it was a standard C string. > I missed that, though. You took hint from a wrong place. You are auditing the destination buffer, so the correct place to take hint from is the memcpy() that touches the destination. ``` ## Junio C Hamano, 2016-03-30 17:17 Subject: Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy Message-ID: URL: https://gitlist.dev/e/xmqqmvpfubq8.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160330011334.GC2237@sigill.intra.peff.net> ``` Jeff King writes: > I don't think this is wrong, but would it perhaps be simpler to call > git_config_get_value() in the first place, which does not make a copy of > the string? Yup, I agree. ``` ## Junio C Hamano, 2016-03-30 17:19 Subject: Re: [PATCH 5/6] bundle: don't leak an fd in case of early return Message-ID: URL: https://gitlist.dev/e/xmqqio03ubn6.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160330011743.GD2237@sigill.intra.peff.net> ``` Jeff King writes: >> + if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) { >> + if (!bundle_to_stdout) >> + close(bundle_fd); >> return -1; >> + } > > Makes sense. Should we also be rolling back the lock file? It happens > automatically at program exit, of course, but we are in library code > here that should not rely on that. Yeah, I agree. I suspect that the original wasn't even meant to be used in a "library-ish" fashion, but as long as we are adding this close(), we should also be doing the rollback. Thanks. ``` ## Jeff King, 2016-03-30 17:21 Subject: Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory Message-ID: <20160330172157.GA19675@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160330172157.GA19675%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Mar 30, 2016 at 10:06:41AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote: > > > >> The implementation of strbuf_list_free() is this: > >> > >> struct strbuf **s = sbs; > >> while (*s) { > >> strbuf_release(*s); > >> free(*s++); > >> } > >> free(sbs); > >> > >> which means that wt-status.c is leaking not only 'split', but also > >> each element of split[], right? > > > > Yeah, I didn't notice that, but I think you're right. > > Correct. > > I suspect that we would be better off in the longer term if > we made conscious effort to deprecate and eradicate the use > of strbuf_split* API functions. They are easy to write > crappy code with, inefficient, and error prone. You would > rarely need to have N resulting pieces as strbufs to be > easily manipulatable [*1*]. Yeah, I agree that it is a clunky interface, and I would be happy to see it go away. Many callers would be better off with string_list_split(). When I've looked in the past, though, converting some of the callers was somewhat non-trivial. But in this case... > The function can be written by not using the "split and then > rebuild" pattern, perhaps like so, and the result is even > easier to read and understand, I would say. A sample rewrite > is attached at the end. I agree that the function is much simpler without it. > + /* find the second token on the line */ > + cp = strchr(line->buf, ' '); > + if (!cp) > + return; > + cp++; > + ep = strchr(cp, ' '); > + if (!ep) > + return; Would we ever see "cmd sha1" without a third token? If so, we'd want: ep = strchrnul(cp, ' '); -Peff ```