Am 16.01.2019 um 10:18 schrieb Carlo Arenas:
> while running HEAD cppcheck against git HEAD got the following error,
> that seem to be in the code all the way to maint:
>
> [refs/files-backend.c:2681] -> [refs.c:1044] -> [cache.h:1075]:
> (error) Null pointer dereference: src
>
> the code that uses NULL as the source OID was introduced with
> b0ca411051 ("files_transaction_prepare(): don't leak flags to packed
> transaction", 2017-11-05) and doesn't seem to be a false positive,
> hence why I am hoping someone else with a better understanding of it
> could come out with a solution
Tl;dr: It's safe.
The statement at line 2681 ff. of refs/files-backend.c is:
ref_transaction_add_update(
packed_transaction, update->refname,
REF_HAVE_NEW | REF_NO_DEREF,
&update->new_oid, NULL,
NULL);
The function is defined in refs/files-backend.c; here are the lines
up to no. 1044:
struct ref_update *ref_transaction_add_update(
struct ref_transaction *transaction,
const char *refname, unsigned int flags,
const struct object_id *new_oid,
const struct object_id *old_oid,
const char *msg)
{
struct ref_update *update;
if (transaction->state != REF_TRANSACTION_OPEN)
BUG("update called for transaction that is not open");
FLEX_ALLOC_STR(update, refname, refname);
ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
transaction->updates[transaction->nr++] = update;
update->flags = flags;
if (flags & REF_HAVE_NEW)
oidcpy(&update->new_oid, new_oid);
if (flags & REF_HAVE_OLD)
oidcpy(&update->old_oid, old_oid);
The "src" in the message "Null pointer dereference: src" refers to
the second parameter of oidcpy() in the line above, i.e. to old_oid.
That's the fifth parameter to ref_transaction_add_update(), and it
is NULL in the invocation mentioned at the top.
oidcopy() is only executed if the flag REF_HAVE_OLD is set, and that
caller passes only REF_HAVE_NEW and REF_NO_DEREF. So let's look at
their values:
$ git grep -E 'define (REF_HAVE_OLD|REF_HAVE_NEW|REF_NO_DEREF)'
refs.h:#define REF_NO_DEREF (1 << 0)
refs/refs-internal.h:#define REF_HAVE_NEW (1 << 2)
refs/refs-internal.h:#define REF_HAVE_OLD (1 << 3)
So these three flags don't overlap; oidcpy() in line 1044 is skipped
by the invocation of ref_transaction_add_update() that offended
cppcheck, i.e. NULL is not actually dereferenced.
I guess the function requires callers to indicate the presence of
non-NULL object ID pointers using flags instead of checking for
NULL directly because the new_oid and old_oid members of struct
ref_update are not nullable, yet they are used as (partial) input
for ref_transaction_add_update().
René
