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

[Derrick Stolee]

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 <dsto...@microsoft.com>
---
  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