Re: null pointer dereference in refs/file-backend

From: René Scharfe <l.s.r@web.de>
Date: Jan 16, 2019, 15:49 UTC
Message-ID: <410a950b-3cbf-1936-aea6-0d4894206a20@web.de>
In-Reply-To: <CAPUEspiz3RxwRsEJW2MwbVVEQh55Q9eA264=RPjtjkx8T-m7iw@mail.gmail.com>

Am 16.01.2019 um 10:18 schrieb Carlo Arenas:

Show 11 quoted lines

> 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é