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