From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 06 Jun 2018 12:45:00 GMT
Subject: Re: [PATCH v5 18/21] commit-graph: use string-list API for input
Message-ID: <548f9285-da68-478c-1423-a9b9b5021d97@gmail.com>
In-Reply-To: <87efhkyubq.fsf@evledraar.gmail.com>

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@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`).
> 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...