[PATCH 0/6] Some cleanups

[PATCH 0/6] Some cleanups

[Stefan Beller]

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

[PATCH 1/6] path.c: allocate enough memory for string

[Stefan Beller]

`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 <sbeller@google.com>
---
 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

Re: [PATCH 1/6] path.c: allocate enough memory for string

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> ---
>  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 <len,ptr> 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;

Re: [PATCH 1/6] path.c: allocate enough memory for string

[Eric Sunshine]

On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> ---
> 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;
> --

Re: [PATCH 1/6] path.c: allocate enough memory for string

[Stefan Beller]

On Tue, Mar 29, 2016 at 5:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
>> ---
>> 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;
>> --

Re: [PATCH 1/6] path.c: allocate enough memory for string

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Mar 29, 2016 at 5:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
>>> ---
>>> 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.

[PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string

[Stefan Beller]

`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 <sbeller@google.com>
---
 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

Re: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string

[Eric Sunshine]

On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> ---
>  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)
> --

Re: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string

[Jeff King]

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 <sbeller@google.com>
> ---
>  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

[PATCH 3/6] 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 | 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

Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy

[Eric Sunshine]

On Tue, Mar 29, 2016 at 8:38 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
> @@ -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;
>  }

Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy

[Jeff King]

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 <sbeller@google.com>
> ---
>  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

Re: [PATCH 3/6] notes: don't leak memory in git_config_get_notes_strategy

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

[PATCH 4/6] abbrev_sha1_in_line: don't leak memory

[Stefan Beller]

`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 <sbeller@google.com>
---
 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

Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory

[Jeff King]

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 <sbeller@google.com>
> ---
>  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

Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:11 PM, Jeff King <peff@peff.net> 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 <sbeller@google.com>
>> ---
>> 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?

Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory

[Jeff King]

On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote:

> On Tue, Mar 29, 2016 at 9:11 PM, Jeff King <peff@peff.net> 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 <sbeller@google.com>
> >> ---
> >> 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

Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory

[Junio C Hamano]

Jeff King <peff@peff.net> 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)

Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory

[Jeff King]

On Wed, Mar 30, 2016 at 10:06:41AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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

[PATCH 5/6] 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
ourselves.

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

Re: [PATCH 5/6] bundle: don't leak an fd in case of early return

[Jeff King]

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 <sbeller@google.com>
> ---
>  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

Re: [PATCH 5/6] bundle: don't leak an fd in case of early return

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

[PATCH 6/6] 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.8.0.8.g27a27a6.dirty

Re: [PATCH 6/6] credential-cache, send_request: close fd when done

[Jeff King]

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

Looks good. I think nobody ever noticed because credential-cache
basically sends off the request and then exits.

-Peff