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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 6, 2018, 12:11 UTC
Message-ID: <87h8mgyv0f.fsf@evledraar.gmail.com>
In-Reply-To: <20180606113611.87822-19-dstolee@microsoft.com>

On Wed, Jun 06 2018, Derrick Stolee wrote:

Show 6 quoted lines

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/commit-graph.c | 39 +++++++++++++--------------------------
>  commit-graph.c         | 15 +++++++--------
>  commit-graph.h         |  7 +++----
>  3 files changed, 23 insertions(+), 38 deletions(-)

Show 63 quoted lines

> 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().

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.