Re: [PATCH] bisect: save heap memory. allocate only the required amount

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 25, 2014, 14:06 UTC
Message-ID: <CAP8UFD2FAfg5GenJXOkOsjU9vmCO3R3Difp6-mrP_cp4zXQENg@mail.gmail.com>
In-Reply-To: <20140825133550.GE17288@peff.net>

On Mon, Aug 25, 2014 at 3:35 PM, Jeff King <peff@peff.net> wrote:

Show 17 quoted lines

> On Sun, Aug 24, 2014 at 07:47:24PM +0530, Arjun Sreedharan wrote:
>
>> diff --git a/bisect.c b/bisect.c
>> index d6e851d..c96aab0 100644
>> --- a/bisect.c
>> +++ b/bisect.c
>> @@ -215,10 +215,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
>>       }
>>       qsort(array, cnt, sizeof(*array), compare_commit_dist);
>>       for (p = list, i = 0; i < cnt; i++) {
>> -             struct name_decoration *r = xmalloc(sizeof(*r) + 100);
>> +             char name[100];
>> +             sprintf(name, "dist=%d", array[i].distance);
>> +             int name_len = strlen(name);
>> +             struct name_decoration *r = xmalloc(sizeof(*r) + name_len);
>
> This allocation should be name_len + 1 for the NUL-terminator, no?

I wondered about that too, but as struct name_decoration is defined like this:

struct name_decoration {
        struct name_decoration *next;
        int type;
        char name[1];
};

the .name field of this struct already has one char, so the allocation above should be ok.

> It looks like add_name_decoration in log-tree already handles half of
> what you are adding here. Can we just make that available globally (it
> is manipulating the already-global "struct decoration name_decoration")?

Yeah, it looks like it should be better.

Note that add_name_decoration() does:

int nlen = strlen(name); struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + nlen);

so it also relies on the fact that .name contains one char.

Show 21 quoted lines

> I also notice that we do not set r->type at all, meaning the decoration
> lookup code in log-tree will access uninitialized memory (worse, it will
> use it as a pointer offset into the color list; I got a segfault when I
> tried to run "git rev-list --bisect-all v1.8.0..v1.9.0").
>
> I think we need this:
>
> diff --git a/bisect.c b/bisect.c
> index d6e851d..e2a7682 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -219,6 +219,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
>                 struct object *obj = &(array[i].commit->object);
>
>                 sprintf(r->name, "dist=%d", array[i].distance);
> +               r->type = 0;
>                 r->next = add_decoration(&name_decoration, obj, r);
>                 p->item = array[i].commit;
>                 p = p->next;
>
> at a minimum.

Yeah if we don't use add_name_decoration() we would need that. Thanks for noticing.

> It looks like this was a regression caused by eb3005e (commit.h: add
> 'type' to struct name_decoration, 2010-06-19). Which makes me wonder if
> anybody actually _uses_ --bisect-all (which AFAICT is the only way to
> trigger the problem), but since it's public, I guess we should keep it.

Yeah, we should probably keep it.

Show 5 quoted lines

> I think the sane thing here is to stop advertising name_decoration as a
> global, and make all callers use add_name_decoration. That makes it
> easier for callers like this one, and would have caught the regression
> caused be eb3005e (the compiler would have noticed that we were not
> passing a type parameter to the function).

I agree.

Thanks, Christian.