[PATCHv3 0/4] Some cleanups

[PATCHv3 0/4] Some cleanups

[Stefan Beller]

v3:
Thanks Eric, Jeff, Junio for discussion!
* use git_config_get_value instead of git_config_get_string in patch 1
* Improve commit message to explain why strbuf_list_free frees more memory
  (hence is the right thing to do)
* the bundle code doesn't have a dedicated return variable,
  but the error path always returns -1  
* removed a duplicate of
        +	if (!bundle_to_stdout)
        +		close(bundle_fd);
  in the bundle patch.
* This applies on v2.8.0.

v2:
Thanks Eric, Jeff, Junio for discussion, I picked up all hints and here
is a v2.

* drop the overallocation patches (1&2)
* use git_config_get_string instead of its _const equivalent, such that
  we don't need a cast when freeing in git_config_get_notes_strategy
* Use strbuf_list_free instead of cooking our own.
* have a dedicated error exit path in bundle.c, create_bundle

v1:
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 (4):
  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    |  2 +-
 bundle.c           | 17 ++++++++++++-----
 credential-cache.c |  1 +
 wt-status.c        |  4 +---
 4 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.5.0.264.g4004fdc.dirty

[PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Stefan Beller]

`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. Instead of using
`git_config_get_string_const`, use `git_config_get_value`, which doesn't
return a copy.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..fa21f1a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,
 {
 	const char *value;
 
-	if (git_config_get_string_const(key, &value))
+	if (git_config_get_value(key, &value))
 		return 1;
 	if (parse_notes_merge_strategy(value, strategy))
 		git_die_config(key, "unknown notes merge strategy %s", value);
-- 
2.5.0.264.g4004fdc.dirty

[PATCHv3 2/4] abbrev_sha1_in_line: don't leak memory

[Stefan Beller]

`split` is of type `struct strbuf **`, and currently we are leaking split
itself as well as each element in split[i]. We have a dedicated free
function for `struct strbuf **`, which takes care of freeing all
related memory.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    Feel free to drop this patch and use the
    the proposal, which drops `**split` entirely
    http://thread.gmane.org/gmane.comp.version-control.git/290232/focus=290318

 wt-status.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index ef74864..1ea2ebe 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
 				strbuf_addf(line, "%s", split[i]->buf);
 		}
 	}
-	for (i = 0; split[i]; i++)
-		strbuf_release(split[i]);
-
+	strbuf_list_free(split);
 }
 
 static void read_rebase_todolist(const char *fname, struct string_list *lines)
-- 
2.5.0.264.g4004fdc.dirty

[PATCHv3 3/4] bundle: don't leak an fd in case of early return

[Stefan Beller]

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
as well as the lock file ourselves. The lock file may be deleted at the
end of running the program, but we are in library code, so we should
not rely on that.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 bundle.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index 506ac49..31ae1da 100644
--- a/bundle.c
+++ b/bundle.c
@@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	/* write prerequisites */
 	if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))
-		return -1;
+		goto err;
 
 	argc = setup_revisions(argc, argv, &revs, NULL);
 
-	if (argc > 1)
-		return error(_("unrecognized argument: %s"), argv[1]);
+	if (argc > 1) {
+		error(_("unrecognized argument: %s"), argv[1]);
+		goto err;
+	}
 
 	object_array_remove_duplicates(&revs.pending);
 
@@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const char *path,
 	if (!ref_count)
 		die(_("Refusing to create empty bundle."));
 	else if (ref_count < 0)
-		return -1;
+		goto err;
 
 	/* write pack */
 	if (write_pack_data(bundle_fd, &revs))
-		return -1;
+		goto err;
 
 	if (!bundle_to_stdout) {
 		if (commit_lock_file(&lock))
 			die_errno(_("cannot create '%s'"), path);
 	}
 	return 0;
+err:
+	if (!bundle_to_stdout)
+		close(bundle_fd);
+	rollback_lock_file(&lock);
+	return -1;
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd, int flags)
-- 
2.5.0.264.g4004fdc.dirty

[PATCHv3 4/4] credential-cache, send_request: close fd when done

[Stefan Beller]

No need to keep it open any further.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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.5.0.264.g4004fdc.dirty

Re: [PATCHv3 3/4] bundle: don't leak an fd in case of early return

[Jeff King]

On Thu, Mar 31, 2016 at 11:04:05AM -0700, Stefan Beller wrote:

> diff --git a/bundle.c b/bundle.c
> index 506ac49..31ae1da 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const char *path,
>  
>  	/* write prerequisites */
>  	if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))
> -		return -1;
> +		goto err;
>  
>  	argc = setup_revisions(argc, argv, &revs, NULL);
>  
> -	if (argc > 1)
> -		return error(_("unrecognized argument: %s"), argv[1]);
> +	if (argc > 1) {
> +		error(_("unrecognized argument: %s"), argv[1]);
> +		goto err;
> +	}
>  
>  	object_array_remove_duplicates(&revs.pending);
>  
> @@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const char *path,
>  	if (!ref_count)
>  		die(_("Refusing to create empty bundle."));
>  	else if (ref_count < 0)
> -		return -1;
> +		goto err;
>  
>  	/* write pack */
>  	if (write_pack_data(bundle_fd, &revs))
> -		return -1;
> +		goto err;

Sorry for not noticing this before, but if we assume write_pack_data
always closes bundle_fd, even on error (and I think it does), then the
close() in the "err" code path is redundant from this goto, right?

I guess it is harmless, as nobody could have opened a new descriptor in
the interim, so our close(bundle_fd) will just get EBADF. But I guess we
could also do:

  if (write_pack_data(bundle_fd, &revs)) {
	bundle_fd = -1;
	goto err;
  }

or even:

  ret = write_pack_data(bundle_fd, &revs);
  bundle_fd = -1; /* closed by write_pack_data */
  if (ret)
	goto err;

to ensure that nobody (including the non-error code paths) uses it
again.

Like I said, I don't think it's buggy in the current code, but it does
seem a little fragile.

> +err:
> +	if (!bundle_to_stdout)
> +		close(bundle_fd);
> +	rollback_lock_file(&lock);
> +	return -1;

Similarly, I think the rollback_lock_file() call is redundant if
bundle_to_stdout is set (because we don't have created a lockfile in the
first place). I think this is more explicitly OK, because the lockfile
keeps an "am I initialized" flag, but perhaps sticking inside the "if
(!bundle_to_stdout)" condition makes it more clear that it's not an
error (especially because the "commit_lock_file" call above is inside
one).

-Peff

Re: [PATCHv3 0/4] Some cleanups

[Jeff King]

On Thu, Mar 31, 2016 at 11:04:02AM -0700, Stefan Beller wrote:

> v3:
> Thanks Eric, Jeff, Junio for discussion!
> * use git_config_get_value instead of git_config_get_string in patch 1
> * Improve commit message to explain why strbuf_list_free frees more memory
>   (hence is the right thing to do)
> * the bundle code doesn't have a dedicated return variable,
>   but the error path always returns -1  
> * removed a duplicate of
>         +	if (!bundle_to_stdout)
>         +		close(bundle_fd);
>   in the bundle patch.
> * This applies on v2.8.0.

With the exception of the comments on patch 3, these all look good. I'll
leave it to Junio to decide whether he wants to polish up his "get rid
of strbuf_split" patch for patch 2. Certainly yours is a strict
improvement over what is there.

-Peff

Re: [PATCHv3 0/4] Some cleanups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> With the exception of the comments on patch 3, these all look good. I'll
> leave it to Junio to decide whether he wants to polish up his "get rid
> of strbuf_split" patch for patch 2. Certainly yours is a strict
> improvement over what is there.

I do not think there were anything further to be polished up.  Other
than that, I agree with all of the above.

Thanks for reviewing.

Re: [PATCHv3 0/4] Some cleanups

[Jeff King]

On Thu, Mar 31, 2016 at 12:31:34PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > With the exception of the comments on patch 3, these all look good. I'll
> > leave it to Junio to decide whether he wants to polish up his "get rid
> > of strbuf_split" patch for patch 2. Certainly yours is a strict
> > improvement over what is there.
> 
> I do not think there were anything further to be polished up.  Other
> than that, I agree with all of the above.

Well, by polish up, I meant "write a commit message for". :)

The patch itself looked fine to me.

-Peff

Re: [PATCHv3 0/4] Some cleanups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Well, by polish up, I meant "write a commit message for". :)
>
> The patch itself looked fine to me.

I'll throw it in my "leftover bits" list.  After queuing 2/4, it now
needs a trivial adjustment to the patch text, which would be a good
spice for a new contributor who is looking for something a bit more
than "somebody did all the work and I can just resend it".

Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Eric Sunshine]

On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller <sbeller@google.com> 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. Instead of using
> `git_config_get_string_const`, use `git_config_get_value`, which doesn't
> return a copy.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,
>  {
>         const char *value;
>
> -       if (git_config_get_string_const(key, &value))
> +       if (git_config_get_value(key, &value))

Hmm, doesn't this introduce a rather severe regression? Unless I'm
misreading the code (possible), with the original, if 'key' was
boolean (lacked a value in the config file), then it would complain:

    Missing value for 'floop.blork'

but, with this change, it will dereference NULL and crash.

(My understanding was that Peff's suggestion to use
git_config_get_value() implied a bit of work beyond the simple textual
substitution of 'git_config_get_value' for
'git_config_get_string_const'.)

>                 return 1;
>         if (parse_notes_merge_strategy(value, strategy))
>                 git_die_config(key, "unknown notes merge strategy %s", value);

Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller <sbeller@google.com> 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. Instead of using
>> `git_config_get_string_const`, use `git_config_get_value`, which doesn't
>> return a copy.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,
>>  {
>>         const char *value;
>>
>> -       if (git_config_get_string_const(key, &value))
>> +       if (git_config_get_value(key, &value))
>
> Hmm, doesn't this introduce a rather severe regression? Unless I'm
> misreading the code (possible), with the original, if 'key' was
> boolean (lacked a value in the config file), then it would complain:
>
>     Missing value for 'floop.blork'
>
> but, with this change, it will dereference NULL and crash.
>
> (My understanding was that Peff's suggestion to use
> git_config_get_value() implied a bit of work beyond the simple textual
> substitution of 'git_config_get_value' for
> 'git_config_get_string_const'.)

Yup, thanks for spelling it out.

Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Jeff King]

On Thu, Mar 31, 2016 at 05:08:30PM -0400, Eric Sunshine wrote:

> On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller <sbeller@google.com> 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. Instead of using
> > `git_config_get_string_const`, use `git_config_get_value`, which doesn't
> > return a copy.
> >
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,
> >  {
> >         const char *value;
> >
> > -       if (git_config_get_string_const(key, &value))
> > +       if (git_config_get_value(key, &value))
> 
> Hmm, doesn't this introduce a rather severe regression? Unless I'm
> misreading the code (possible), with the original, if 'key' was
> boolean (lacked a value in the config file), then it would complain:
> 
>     Missing value for 'floop.blork'
> 
> but, with this change, it will dereference NULL and crash.
> 
> (My understanding was that Peff's suggestion to use
> git_config_get_value() implied a bit of work beyond the simple textual
> substitution of 'git_config_get_value' for
> 'git_config_get_string_const'.)

Ah, yeah, I didn't even think about that case. I was thinking that
wouldn't it be nice if we had:

  const char *git_config_get_string(const char *key);

which would be a much more natural interface. But the reason we don't is
that we have to represent the "NULL as boolean true" case in the first
place.

So I dunno. Getting the NULL-handling for free is rather nice, and maybe
worth using the normal git_config_get_string(). It's too bad there's not
a variant that just returns a non-allocated pointer, but given that
there is already a confusing proliferation of functions to retrieve a
config string, it's hard to justify adding another.

-Peff