Web lists-archives.com

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




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

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`).
Most of your tests (t5318-commit-graph.sh) segfaulted, but presumably
you're on a more forgiving compiler/platform/options. I compiled with
-O0 -g on clang 4.0.1-8 + Debian testing.

I appreciate the extra platform testing. I'm using GCC on Ubuntu (gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0).


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?
Sure that works too. We'd be doing the init when we don't need it, but
it's not like this part is performance critical or anything...