Web lists-archives.com

Re: [PATCH v5 18/21] commit-graph: use string-list API for input




On 6/6/2018 8:11 AM, Ævar Arnfjörð Bjarmason wrote:
On Wed, Jun 06 2018, Derrick Stolee wrote:

Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
---
  builtin/commit-graph.c | 39 +++++++++++++--------------------------
  commit-graph.c         | 15 +++++++--------
  commit-graph.h         |  7 +++----
  3 files changed, 23 insertions(+), 38 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3079cde6f9..d8eb8278b3 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -118,13 +118,9 @@ static int graph_read(int argc, const char **argv)

  static int graph_write(int argc, const char **argv)
  {
-	const char **pack_indexes = NULL;
-	int packs_nr = 0;
-	const char **commit_hex = NULL;
-	int commits_nr = 0;
-	const char **lines = NULL;
-	int lines_nr = 0;
-	int lines_alloc = 0;
+	struct string_list *pack_indexes = NULL;
+	struct string_list *commit_hex = NULL;
+	struct string_list lines;

  	static struct option builtin_commit_graph_write_options[] = {
  		OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -150,32 +146,23 @@ static int graph_write(int argc, const char **argv)

  	if (opts.stdin_packs || opts.stdin_commits) {
  		struct strbuf buf = STRBUF_INIT;
-		lines_nr = 0;
-		lines_alloc = 128;
-		ALLOC_ARRAY(lines, lines_alloc);
-
-		while (strbuf_getline(&buf, stdin) != EOF) {
-			ALLOC_GROW(lines, lines_nr + 1, lines_alloc);
-			lines[lines_nr++] = strbuf_detach(&buf, NULL);
-		}
-
-		if (opts.stdin_packs) {
-			pack_indexes = lines;
-			packs_nr = lines_nr;
-		}
-		if (opts.stdin_commits) {
-			commit_hex = lines;
-			commits_nr = lines_nr;
-		}
+		string_list_init(&lines, 0);
+
+		while (strbuf_getline(&buf, stdin) != EOF)
+			string_list_append(&lines, strbuf_detach(&buf, NULL));
+
+		if (opts.stdin_packs)
+			pack_indexes = &lines;
+		if (opts.stdin_commits)
+			commit_hex = &lines;
  	}

  	write_commit_graph(opts.obj_dir,
  			   pack_indexes,
-			   packs_nr,
  			   commit_hex,
-			   commits_nr,
  			   opts.append);

+	string_list_clear(&lines, 0);
  	return 0;
  }
This results in an invalid free() & segfault because you're freeing
&lines which may not have been allocated by string_list_init().

Good point. Did my tests not catch this? (seems it requires calling `git commit-graph write` with no `--stdin-packs` or `--stdin-commits`).


Monkeypatch on top which I used to fix it:

     diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
     index 76423b3fa5..c7eb68aa3a 100644
     --- a/builtin/commit-graph.c
     +++ b/builtin/commit-graph.c
     @@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
             struct string_list *pack_indexes = NULL;
             struct string_list *commit_hex = NULL;
             struct string_list lines;
     +       int free_lines = 0;

             static struct option builtin_commit_graph_write_options[] = {
                     OPT_STRING(0, "object-dir", &opts.obj_dir,
     @@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
             if (opts.stdin_packs || opts.stdin_commits) {
                     struct strbuf buf = STRBUF_INIT;
                     string_list_init(&lines, 0);
     +               free_lines = 1;

                     while (strbuf_getline(&buf, stdin) != EOF)
                             string_list_append(&lines, strbuf_detach(&buf, NULL));
     @@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
                                commit_hex,
                                opts.append);

     -       string_list_clear(&lines, 0);
     +       if (free_lines)
     +               string_list_clear(&lines, 0);
             return 0;
      }

But probably having a pointer to the struct which is NULL etc. is
better.

Wouldn't the easiest fix be to call `string_list_init(&lines, 0)` outside of any conditional?

Thanks,
-Stolee