threads / discuss / 41889

[PATCHv3 0/4] Some cleanups

Subject: [PATCHv3 0/4] Some cleanups

## tl;dr

13 messages between Mar 31, 2016 and Mar 31, 2016.

replies: 12people: 4as markdown or json

Stefan Beller· Mar 31, 2016, 18:04 UTC · lore
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
Stefan Beller· Mar 31, 2016, 18:04 UTC · re: Stefan Beller · lore

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

`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
Eric Sunshine· Mar 31, 2016, 21:08 UTC · re: Stefan Beller · lore

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

On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller <sbeller@google.com> wrote:
Show 16 quoted lines
> `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);
Junio C Hamano· Mar 31, 2016, 22:54 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 30 quoted lines
> 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.
Jeff King· Mar 31, 2016, 23:32 UTC · re: Eric Sunshine · lore

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

On Thu, Mar 31, 2016 at 05:08:30PM -0400, Eric Sunshine wrote:
Show 30 quoted lines
> 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
Stefan Beller· Mar 31, 2016, 18:04 UTC · re: Stefan Beller · lore

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

`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
Stefan Beller· Mar 31, 2016, 18:04 UTC · re: Stefan Beller · lore

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

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
Jeff King· Mar 31, 2016, 19:11 UTC · re: Stefan Beller · lore

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

On Thu, Mar 31, 2016 at 11:04:05AM -0700, Stefan Beller wrote:
Show 33 quoted lines
> 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.

Show 5 quoted lines
> +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
Stefan Beller· Mar 31, 2016, 18:04 UTC · re: Stefan Beller · lore

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

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
Jeff King· Mar 31, 2016, 19:12 UTC · re: Stefan Beller · lore

Re: [PATCHv3 0/4] Some cleanups

On Thu, Mar 31, 2016 at 11:04:02AM -0700, Stefan Beller wrote:
Show 12 quoted lines
> 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
Junio C Hamano· Mar 31, 2016, 19:31 UTC · re: Jeff King · lore

Re: [PATCHv3 0/4] Some cleanups

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.
Jeff King· Mar 31, 2016, 19:33 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 0/4] Some cleanups

On Thu, Mar 31, 2016 at 12:31:34PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Junio C Hamano· Mar 31, 2016, 20:11 UTC · re: Jeff King · lore

Re: [PATCHv3 0/4] Some cleanups

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

← back to recent threads