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