On Sat, Apr 19, 2014 at 12:23 PM, Michael Haggerty <[email protected]> wrote:
> On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
>> Change commit.c to use ref transactions for all ref updates.
>>
>> Signed-off-by: Ronnie Sahlberg <[email protected]>
>> ---
>> builtin/commit.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index d9550c5..b8e4389 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1541,11 +1541,11 @@ int cmd_commit(int argc, const char **argv, const
>> char *prefix)
>> const char *index_file, *reflog_msg;
>> char *nl;
>> unsigned char sha1[20];
>> - struct ref_lock *ref_lock;
>> struct commit_list *parents = NULL, **pptr = &parents;
>> struct stat statbuf;
>> struct commit *current_head = NULL;
>> struct commit_extra_header *extra = NULL;
>> + struct ref_transaction *transaction;
>>
>> if (argc == 2 && !strcmp(argv[1], "-h"))
>> usage_with_options(builtin_commit_usage,
>> builtin_commit_options);
>> @@ -1667,12 +1667,6 @@ int cmd_commit(int argc, const char **argv, const
>> char *prefix)
>> strbuf_release(&author_ident);
>> free_commit_extra_headers(extra);
>>
>> - ref_lock = lock_any_ref_for_update("HEAD",
>> - !current_head
>> - ? NULL
>> - : current_head->object.sha1,
>> - 0, NULL);
>
> The old version, above, contemplates that current_head might be NULL...
>
>> -
>> nl = strchr(sb.buf, '\n');
>> if (nl)
>> strbuf_setlen(&sb, nl + 1 - sb.buf);
>> @@ -1681,14 +1675,22 @@ int cmd_commit(int argc, const char **argv, const
>> char *prefix)
>> strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>> strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>>
>> - if (!ref_lock) {
>> + transaction = ref_transaction_begin();
>> + if (!transaction) {
>> rollback_index_files();
>> - die(_("cannot lock HEAD ref"));
>> + die(_("HEAD: cannot start transaction"));
>> }
>> - if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
>> + if (ref_transaction_update(transaction, "HEAD", sha1,
>> + current_head->object.sha1,
>> + 0, !!current_head)) {
>
> ...but here you dereference current_head without checking it first.
>
> It upsets me that the test suite didn't catch this NULL pointer
> dereference. Either
>
> 1. current_head cannot in fact be NULL, in which case the commit message
> should explain that fact and the code should be simplified
>
> or
>
> 2. the test suite is incomplete. If so, it would be great if you would
> add a test that exercises this branch of the code (and catches your
> error), and then fix the error.
Current_head can in fact be NULL here but we never actually
dereference any pointer in this case since !!current_head is 0.
current_head->object.sha1 just computes current_head +
offsetof(commit, object) + offsetof(object, sha1)
so we compute and pass a non-NULL garbage pointer into ref_transaction_update()
But since !!current_head is 0 this mean we never actually dereference
this pointer.
Way to subtle for its own good.
I will change ref_transaction_update and friends to use an additional
test to detect some subset of this kind of bug :
if (!have_old && old_sha1)
die("have_old is false but old_sha1 is not NULL");
regards
ronnie sahlberg
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
