{"thread":{"id":"50238","subject":"null pointer dereference in refs/file-backend","startedAt":"2019-01-16T09:18:36Z","lastAt":"2019-01-16T15:50:02Z","messageCount":2,"participants":["Carlo Arenas","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"366798","messageId":"CAPUEspiz3RxwRsEJW2MwbVVEQh55Q9eA264=RPjtjkx8T-m7iw@mail.gmail.com","threadId":"50238","inReplyTo":null,"subject":"null pointer dereference in refs/file-backend","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-01-16T09:18:23Z","receivedAt":"2019-01-16T09:18:36Z","isPatch":false,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"while running HEAD cppcheck against git HEAD got the following error,\nthat seem to be in the code all the way to maint:\n\n[refs/files-backend.c:2681] -> [refs.c:1044] -> [cache.h:1075]:\n(error) Null pointer dereference: src\n\nthe code that uses NULL as the source OID was introduced with\nb0ca411051 (\"files_transaction_prepare(): don't leak flags to packed\ntransaction\", 2017-11-05) and doesn't seem to be a false positive,\nhence why I am hoping someone else with a better understanding of it\ncould come out with a solution\n\nCarlo\n"},{"id":"366855","messageId":"410a950b-3cbf-1936-aea6-0d4894206a20@web.de","threadId":"50238","inReplyTo":"CAPUEspiz3RxwRsEJW2MwbVVEQh55Q9eA264=RPjtjkx8T-m7iw@mail.gmail.com","subject":"Re: null pointer dereference in refs/file-backend","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-01-16T15:49:44Z","receivedAt":"2019-01-16T15:50:02Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.01.2019 um 10:18 schrieb Carlo Arenas:\n> while running HEAD cppcheck against git HEAD got the following error,\n> that seem to be in the code all the way to maint:\n>\n> [refs/files-backend.c:2681] -> [refs.c:1044] -> [cache.h:1075]:\n> (error) Null pointer dereference: src\n>\n> the code that uses NULL as the source OID was introduced with\n> b0ca411051 (\"files_transaction_prepare(): don't leak flags to packed\n> transaction\", 2017-11-05) and doesn't seem to be a false positive,\n> hence why I am hoping someone else with a better understanding of it\n> could come out with a solution\n\nTl;dr: It's safe.\n\n\nThe statement at line 2681 ff. of refs/files-backend.c is:\n\n\tref_transaction_add_update(\n\t\t\tpacked_transaction, update->refname,\n\t\t\tREF_HAVE_NEW | REF_NO_DEREF,\n\t\t\t&update->new_oid, NULL,\n\t\t\tNULL);\n\nThe function is defined in refs/files-backend.c; here are the lines\nup to no. 1044:\n\n\tstruct ref_update *ref_transaction_add_update(\n\t\t\tstruct ref_transaction *transaction,\n\t\t\tconst char *refname, unsigned int flags,\n\t\t\tconst struct object_id *new_oid,\n\t\t\tconst struct object_id *old_oid,\n\t\t\tconst char *msg)\n\t{\n\t\tstruct ref_update *update;\n\n\t\tif (transaction->state != REF_TRANSACTION_OPEN)\n\t\t\tBUG(\"update called for transaction that is not open\");\n\n\t\tFLEX_ALLOC_STR(update, refname, refname);\n\t\tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n\t\ttransaction->updates[transaction->nr++] = update;\n\n\t\tupdate->flags = flags;\n\n\t\tif (flags & REF_HAVE_NEW)\n\t\t\toidcpy(&update->new_oid, new_oid);\n\t\tif (flags & REF_HAVE_OLD)\n\t\t\toidcpy(&update->old_oid, old_oid);\n\nThe \"src\" in the message \"Null pointer dereference: src\" refers to\nthe second parameter of oidcpy() in the line above, i.e. to old_oid.\nThat's the fifth parameter to ref_transaction_add_update(), and it\nis NULL in the invocation mentioned at the top.\n\noidcopy() is only executed if the flag REF_HAVE_OLD is set, and that\ncaller passes only REF_HAVE_NEW and REF_NO_DEREF.  So let's look at\ntheir values:\n\n\t$ git grep -E 'define (REF_HAVE_OLD|REF_HAVE_NEW|REF_NO_DEREF)'\n\trefs.h:#define REF_NO_DEREF (1 << 0)\n\trefs/refs-internal.h:#define REF_HAVE_NEW (1 << 2)\n\trefs/refs-internal.h:#define REF_HAVE_OLD (1 << 3)\n\nSo these three flags don't overlap; oidcpy() in line 1044 is skipped\nby the invocation of ref_transaction_add_update() that offended\ncppcheck, i.e. NULL is not actually dereferenced.\n\n\nI guess the function requires callers to indicate the presence of\nnon-NULL object ID pointers using flags instead of checking for\nNULL directly because the new_oid and old_oid members of struct\nref_update are not nullable, yet they are used as (partial) input\nfor ref_transaction_add_update().\n\nRené\n"}]}