git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 07/11] commit.c: use ref transactions for updates

From
Ronnie Sahlberg <sahlberg@google.com>
Date
Apr 21, 2014, 18:45 UTC
Message-ID
<CAL=YDW=5Xf4rq11uxA67zbrK0gGPFhs24SL0NCnny3UObtZAfw@mail.gmail.com>
In-Reply-To
<5352CD14.4060808@alum.mit.edu>
On Sat, Apr 19, 2014 at 12:23 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 70 quoted lines
> 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 <sahlberg@google.com>
>> ---
>>  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

Previous: Michael HaggertyNext: Ronnie Sahlberg
Message 17 of 24 in “Use ref transactions from most callers”
  1. 00/11 Use ref transactions from most callersRonnie Sahlberg, Apr 17, 2014
  2. 01/11 refs.c: constify the sha arguments for ref_transaction_create|delete|updateRonnie Sahlberg, Apr 17, 2014
  3. Michael HaggertyApr 19, 2014
  4. 02/11 refs.c: change ref_transaction_update() to do error checking and return statusRonnie Sahlberg, Apr 17, 2014
  5. Michael HaggertyApr 19, 2014
  6. Jonathan NiederApr 25, 2014
  7. 03/11 refs.c: change ref_transaction_create to do error checking and return statusRonnie Sahlberg, Apr 17, 2014
  8. Michael HaggertyApr 19, 2014
  9. 04/11 refs.c: ref_transaction_delete to check for error and return statusRonnie Sahlberg, Apr 17, 2014
  10. Michael HaggertyApr 19, 2014
  11. 05/11 tag.c: use ref transactions when doing updatesRonnie Sahlberg, Apr 17, 2014
  12. Michael HaggertyApr 19, 2014
  13. 06/11 replace.c: use the ref transaction functions for updatesRonnie Sahlberg, Apr 17, 2014
  14. Michael HaggertyApr 19, 2014
  15. 07/11 commit.c: use ref transactions for updatesRonnie Sahlberg, Apr 17, 2014
  16. Michael HaggertyApr 19, 2014
  17. Ronnie SahlbergApr 21, 2014
  18. 08/11 sequencer.c: use ref transactions for all ref updatesRonnie Sahlberg, Apr 17, 2014
  19. 09/11 fast-import.c: change update_branch to use ref transactionsRonnie Sahlberg, Apr 17, 2014
  20. 10/11 branch.c: use ref transaction for all ref updatesRonnie Sahlberg, Apr 17, 2014
  21. 11/11 walker.c: use ref transaction for ref updatesRonnie Sahlberg, Apr 17, 2014
  22. Michael HaggertyApr 19, 2014
  23. Junio C HamanoApr 21, 2014
  24. Ronnie SahlbergApr 21, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.