2011/11/17 Ramkumar Ramachandra <artagnon@xxxxxxxxx>:
> Hi Nguyễn,
>
> Nguyễn Thái Ngọc Duy wrote:
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index 2b8e73b..6efb1cf 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -696,15 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>> {
>> int ret = 0;
>> struct branch_info old;
>> + char *path;
>> unsigned char rev[20];
>> int flag;
>> memset(&old, 0, sizeof(old));
>> - old.path = xstrdup(resolve_ref_unsafe("HEAD", rev, 0, &flag));
>> + old.path = path = resolve_ref("HEAD", rev, 0, &flag);
>> old.commit = lookup_commit_reference_gently(rev, 1);
>> - if (!(flag & REF_ISSYMREF)) {
>> - free((char *)old.path);
>> + if (!(flag & REF_ISSYMREF))
>> old.path = NULL;
>> - }
>>
>> if (old.path && !prefixcmp(old.path, "refs/heads/"))
>> old.name = old.path + strlen("refs/heads/");
>
> This caught my eye immediately: you introduced a new "path" variable.
> Let's scroll ahead and see why.
>
>> @@ -718,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>> }
>>
>> ret = merge_working_tree(opts, &old, new);
>> - if (ret)
>> + if (ret) {
>> + free(path);
>> return ret;
>> + }
>>
>> if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
>> orphaned_commit_warning(old.commit);
>> @@ -727,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>> update_refs_for_switch(opts, &old, new);
>>
>> ret = post_checkout_hook(old.commit, new->commit, 1);
>> - free((char *)old.path);
>> + free(path);
>> return ret || opts->writeout_error;
>> }
>
> Before application of your patch, if !(flag & REF_ISSYMREF) then
> old.path is set to NULL and this free() would've read free(NULL). Was
> this codepath ever reached, and did you fix a bug by introducing the
> new "path" variable, or was it never reached but you introduced the
> new variable for clarity anyway? Either case is worth mentioning in
> the commit message, I think.
free(NULL) is OK if I remember correctly, so it's not really a bug.
Although I do plug a memory leak when merge_working_tree() returns
non-zero.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html