From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 06 Jun 2018 12:15:04 GMT
Subject: Re: [PATCH v5 18/21] commit-graph: use string-list API for input
Message-ID: <aa4c3a93-0c63-f424-db0b-0e451bbaa91e@gmail.com>
In-Reply-To: <87h8mgyv0f.fsf@evledraar.gmail.com>

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@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