[PATCHv4 0/4] Some cleanups

[PATCHv4 0/4] Some cleanups

[Stefan Beller]

Thanks for the reviews!
Thanks Jeff, Eric, Junio!

This replaces sb/misc-cleanups.

* Revert the builtin/notes fix to the first version as git_config_get_value
  is dangerous, and the memory allocation and free is just a small overhead here
* the bundle code integrates all of the suggestions (i.e. rollback_lock_file
  conditioned on (!bundle_to_stdout).
  
I hope I got everything by now. I will head out for today and tomorrow I'll
be traveling to NY, where there is the Git Merge conference on Mon, Tues and some
vacation afterwards. I'll be online in a very limited fashion. (I plan on taking
my phone only, no laptop), so in case I missed a thing this series will halt
for a while or someone else picks it up.

Thanks,
Stefan

diff to remotes/origin/sb/misc-cleanups (which doesn't contain the bundle fix):
diff --git a/builtin/notes.c b/builtin/notes.c
index 386402d..afcfa8f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -741,13 +741,14 @@ static int merge_commit(struct notes_merge_options *o)
 static int git_config_get_notes_strategy(const char *key,
 					 enum notes_merge_strategy *strategy)
 {
-	const char *value;
+	char *value;
 
-	if (git_config_get_value(key, &value))
+	if (git_config_get_string(key, &value))
 		return 1;
 	if (parse_notes_merge_strategy(value, strategy))
 		git_die_config(key, "unknown notes merge strategy %s", value);
 
+	free(value);
 	return 0;
 }
 
diff --git a/bundle.c b/bundle.c
index 506ac49..08e32ca 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,23 @@ 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)


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    |  5 +++--
 bundle.c           | 18 +++++++++++++-----
 credential-cache.c |  1 +
 wt-status.c        |  4 +---
 4 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.5.0.264.gc776916.dirty

[PATCHv4 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.

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

diff --git a/builtin/notes.c b/builtin/notes.c
index 52aa9af..afcfa8f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -741,13 +741,14 @@ static int merge_commit(struct notes_merge_options *o)
 static int git_config_get_notes_strategy(const char *key,
 					 enum notes_merge_strategy *strategy)
 {
-	const char *value;
+	char *value;
 
-	if (git_config_get_string_const(key, &value))
+	if (git_config_get_string(key, &value))
 		return 1;
 	if (parse_notes_merge_strategy(value, strategy))
 		git_die_config(key, "unknown notes merge strategy %s", value);
 
+	free(value);
 	return 0;
 }
 
-- 
2.5.0.264.gc776916.dirty

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

[Eric Sunshine]

On Thu, Mar 31, 2016 at 8:35 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.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 52aa9af..afcfa8f 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -741,13 +741,14 @@ static int merge_commit(struct notes_merge_options *o)
>  static int git_config_get_notes_strategy(const char *key,
>                                          enum notes_merge_strategy *strategy)
>  {
> -       const char *value;
> +       char *value;
>
> -       if (git_config_get_string_const(key, &value))
> +       if (git_config_get_string(key, &value))
>                 return 1;

Meh. Rather than reverting the git_config_get_value(), it would have
been just as easy and safer (less chance of a future change
re-introducing a leak) if you had just inserted the necessary check
here:

    if (!value)
        return  config_error_nonbool(key);

But, perhaps it's not worth the patch churn at this point...

>         if (parse_notes_merge_strategy(value, strategy))
>                 git_die_config(key, "unknown notes merge strategy %s", value);
>
> +       free(value);
>         return 0;
>  }
>
> --
> 2.5.0.264.gc776916.dirty

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Meh. Rather than reverting the git_config_get_value(), it would have
> been just as easy and safer (less chance of a future change
> re-introducing a leak) if you had just inserted the necessary check
> here:
>
>     if (!value)
>         return  config_error_nonbool(key);

Yup, sounds much more sensible fix that is useful in the longer term
(and avoids one unnecessary strdup()).

>
> But, perhaps it's not worth the patch churn at this point...
>
>>         if (parse_notes_merge_strategy(value, strategy))
>>                 git_die_config(key, "unknown notes merge strategy %s", value);
>>
>> +       free(value);
>>         return 0;
>>  }
>>
>> --
>> 2.5.0.264.gc776916.dirty

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Meh. Rather than reverting the git_config_get_value(), it would have
>> been just as easy and safer (less chance of a future change
>> re-introducing a leak) if you had just inserted the necessary check
>> here:
>>
>>     if (!value)
>>         return  config_error_nonbool(key);
>
> Yup, sounds much more sensible fix that is useful in the longer term
> (and avoids one unnecessary strdup()).

Perhaps like this?

-- >8 --
From: Stefan Beller <sbeller@google.com>
Date: Thu, 31 Mar 2016 11:04:03 -0700
Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy

This function asks for the value of a configuration and
after using the value does not have to retain ownership
of the value.  git_config_get_string_const() however is
a function to get a copy of the value, but we forget to
free it before we return.

Because we only need to peek the value without retaining
a pointer to it, use git_config_get_value() to peek into
the value cached in the config API layer.

As git_config_get_value() does not insist the value to be
a string, we'd need to do the "nonbool" check ourselves.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/notes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 52aa9af..c1265eb 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -743,8 +743,10 @@ 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 (!value)
+		return config_error_nonbool(key);
 	if (parse_notes_merge_strategy(value, strategy))
 		git_die_config(key, "unknown notes merge strategy %s", value);
 
-- 
2.8.0-225-g297c00e

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

[Jeff King]

On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:

> -- >8 --
> From: Stefan Beller <sbeller@google.com>
> Date: Thu, 31 Mar 2016 11:04:03 -0700
> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy
> 
> This function asks for the value of a configuration and
> after using the value does not have to retain ownership
> of the value.  git_config_get_string_const() however is
> a function to get a copy of the value, but we forget to
> free it before we return.
> 
> Because we only need to peek the value without retaining
> a pointer to it, use git_config_get_value() to peek into
> the value cached in the config API layer.
> 
> As git_config_get_value() does not insist the value to be
> a string, we'd need to do the "nonbool" check ourselves.

Unfortunately, I don't think this is quite right. In the original, we
relied on git_config_get_string_const to notice a non-string value, at
which point it would die:

  $ git -c notes.mergeStrategy notes merge whatever
  error: Missing value for 'notes.mergeStrategy'
  fatal: unable to parse 'notes.mergeStrategy' from command-line config

But in your patch:

> @@ -743,8 +743,10 @@ 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 (!value)
> +		return config_error_nonbool(key);
>  	if (parse_notes_merge_strategy(value, strategy))
>  		git_die_config(key, "unknown notes merge strategy %s", value);

We just return an error from git_config_get_notes_strategy(). If this
were a callback to git_config(), that would be fine (as we would
auto-die then in the caller), but it's not. It is called directly for a
specific key. One of the callers treats a non-zero return as "we don't
have that variable", and the other ignores the return value completely.

So I think you'd want something more like:

  if (!value) {
	config_error_nonbool(key);
	git_die_config(key);
  }

That keeps the original message intact (though it is a bit verbose in
the first place).

This is why I wondered if the minor "do not allocate" tweak was worth
the trouble, when git_config_get_string() just handles this for us.

-Peff

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

[Eric Sunshine]

On Fri, Apr 1, 2016 at 1:14 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:
>> From: Stefan Beller <sbeller@google.com>
>> Date: Thu, 31 Mar 2016 11:04:03 -0700
>> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy
>>
>> This function asks for the value of a configuration and
>> after using the value does not have to retain ownership
>> of the value.  git_config_get_string_const() however is
>> a function to get a copy of the value, but we forget to
>> free it before we return.
>>
>> Because we only need to peek the value without retaining
>> a pointer to it, use git_config_get_value() to peek into
>> the value cached in the config API layer.
>>
>> As git_config_get_value() does not insist the value to be
>> a string, we'd need to do the "nonbool" check ourselves.

Nicer commit message.

> Unfortunately, I don't think this is quite right. In the original, we
> relied on git_config_get_string_const to notice a non-string value, at
> which point it would die:
>
>   $ git -c notes.mergeStrategy notes merge whatever
>   error: Missing value for 'notes.mergeStrategy'
>   fatal: unable to parse 'notes.mergeStrategy' from command-line config
>
> But in your patch:
>
>> +     if (!value)
>> +             return config_error_nonbool(key);
>
> We just return an error from git_config_get_notes_strategy(). If this
> were a callback to git_config(), that would be fine (as we would
> auto-die then in the caller), but it's not. It is called directly for a
> specific key. One of the callers treats a non-zero return as "we don't
> have that variable", and the other ignores the return value completely.
>
> So I think you'd want something more like:
>
>   if (!value) {
>         config_error_nonbool(key);
>         git_die_config(key);
>   }

Yep, I had noted the bit about having to die() manually when reviewing
the earlier patch, but it slipped from memory when composing the reply
to the current version of the patch.

> This is why I wondered if the minor "do not allocate" tweak was worth
> the trouble, when git_config_get_string() just handles this for us.

Which again suggests a new function which does this work but doesn't
copy the string (despite the already quite large set of similar
functions).

[PATCHv4 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>
---
 wt-status.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index bba2596..9f4be33 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.gc776916.dirty

[PATCHv4 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 | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index 506ac49..08e32ca 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,23 @@ 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.gc776916.dirty

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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>
> ---

Thanks.  I think this turned out to be the trickiest one to get
right among the four and my reading of this round tells me that
it addresses all the trickiness pointed out in the reviews.

Will replace.

>  bundle.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 506ac49..08e32ca 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,23 @@ 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)

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

[Jeff King]

On Fri, Apr 01, 2016 at 10:05:49AM -0700, Junio C Hamano wrote:

> Stefan Beller <sbeller@google.com> writes:
> 
> > 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>
> > ---
> 
> Thanks.  I think this turned out to be the trickiest one to get
> right among the four and my reading of this round tells me that
> it addresses all the trickiness pointed out in the reviews.

You didn't see my response to the other patch yet. :)

> >  	/* write pack */
> >  	if (write_pack_data(bundle_fd, &revs))
> > -		return -1;
> > +		goto err;

This does still suffer from the double-close I mentioned earlier. I'm
not sure if we want to address that or not.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You didn't see my response to the other patch yet. :)
>
>> >  	/* write pack */
>> >  	if (write_pack_data(bundle_fd, &revs))
>> > -		return -1;
>> > +		goto err;
>
> This does still suffer from the double-close I mentioned earlier. I'm
> not sure if we want to address that or not.

Something like this on top?

 bundle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 08e32ca..bbf4efa 100644
--- a/bundle.c
+++ b/bundle.c
@@ -453,8 +453,10 @@ int create_bundle(struct bundle_header *header, const char *path,
 		goto err;
 
 	/* write pack */
-	if (write_pack_data(bundle_fd, &revs))
+	if (write_pack_data(bundle_fd, &revs)) {
+		bundle_fd = -1; /* already closed by the above call */
 		goto err;
+	}
 
 	if (!bundle_to_stdout) {
 		if (commit_lock_file(&lock))
@@ -463,7 +465,8 @@ int create_bundle(struct bundle_header *header, const char *path,
 	return 0;
 err:
 	if (!bundle_to_stdout) {
-		close(bundle_fd);
+		if (0 <= bundle_fd)
+			close(bundle_fd);
 		rollback_lock_file(&lock);
 	}
 	return -1;

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

[Jeff King]

On Fri, Apr 01, 2016 at 10:29:51AM -0700, Junio C Hamano wrote:

> > This does still suffer from the double-close I mentioned earlier. I'm
> > not sure if we want to address that or not.
> 
> Something like this on top?
> 
>  bundle.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/bundle.c b/bundle.c
> index 08e32ca..bbf4efa 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -453,8 +453,10 @@ int create_bundle(struct bundle_header *header, const char *path,
>  		goto err;
>  
>  	/* write pack */
> -	if (write_pack_data(bundle_fd, &revs))
> +	if (write_pack_data(bundle_fd, &revs)) {
> +		bundle_fd = -1; /* already closed by the above call */
>  		goto err;
> +	}
>  
>  	if (!bundle_to_stdout) {
>  		if (commit_lock_file(&lock))
> @@ -463,7 +465,8 @@ int create_bundle(struct bundle_header *header, const char *path,
>  	return 0;
>  err:
>  	if (!bundle_to_stdout) {
> -		close(bundle_fd);
> +		if (0 <= bundle_fd)
> +			close(bundle_fd);
>  		rollback_lock_file(&lock);

Yep, that looks fine.

-Peff

[PATCHv4 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.gc776916.dirty