[PATCHv4 0/4] Some cleanups

Subsystems: the rest

6 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCHv4 0/4] Some cleanups

From: Stefan Beller <hidden>
Date: 2016-06-15 23:09:09

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

From: Stefan Beller <hidden>
Date: 2016-06-15 23:09:09

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

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

From: Stefan Beller <hidden>
Date: 2016-06-15 23:09:09

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 [off-list ref]
Signed-off-by: Stefan Beller <redacted>
---
 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

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

From: Stefan Beller <hidden>
Date: 2016-06-15 23:09:09

`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 [off-list ref]
Signed-off-by: Stefan Beller <redacted>
---
 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 4/4] credential-cache, send_request: close fd when done

From: Stefan Beller <hidden>
Date: 2016-06-15 23:09:09

No need to keep it open any further.

Signed-off-by: Stefan Beller <redacted>
---
 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

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

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:09:09

On Thu, Mar 31, 2016 at 8:35 PM, Stefan Beller [off-list ref] wrote:

quoted hunk

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

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help