Re: [PATCH] refs: make sure we never pass NULL to hashcpy

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 8, 2017, 15:08 UTC
Message-ID: <ab0c4064-6eaa-c298-a26d-58fc83e4574f@alum.mit.edu>
In-Reply-To: <xmqqvaku10gj.fsf@gitster.mtv.corp.google.com>

On 09/08/2017 02:46 AM, Junio C Hamano wrote:

Show 37 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I did just realize one thing: `ref_transaction_update()` takes `flags`
>> as an argument and alters it using
>>
>>>         flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0);
>>
>> Perhaps gcc is *more* intelligent than we give it credit for, and is
>> actually worried that the `flags` argument passed in by the caller
>> might *already* have one of these bits set. In that case
>> `ref_transaction_add_update()` would indeed be called incorrectly.
>> Does the warning go away if you change that line to
>>
>>>         if (new_sha1)
>>>                 flags |=REF_HAVE_NEW;
>>>         else
>>>                 flags &= ~REF_HAVE_NEW;
>>>         if (old_sha1)
>>>                 flags |=REF_HAVE_OLD;
>>>         else
>>>                 flags &= ~REF_HAVE_OLD;
>>
>> ? This might be a nice change to have anyway, to isolate
>> `ref_transaction_update()` from mistakes by its callers.
> 
> I understand "drop HAVE_NEW bit if new_sha1 is NULL" part, but not
> the other side "add HAVE_NEW if new_SHA1 is not NULL"---doesn't the
> NEW/OLD flag exist exactly because some callers pass the address of
> an embedded oid.hash[] or null_sha1, instead of NULL, when one side 
> does not exist?  So new|old being NULL is a definite signal that we
> need to drop HAVE_NEW|OLD, but the reverse may not be true, no?  Is
> it OK to overwrite null_sha1[] that is passed from some codepaths?
> 
> ref_transaction_create and _delete pass null_sha1 on the missing
> side, while ref_transaction_verify passes NULL, while calling
> _update().  Should this distinction affect how _add_update() gets
> called?

There are two functions under discussion:

* `ref_transaction_add_update()` is the low-level, private function that
uses the `HAVE_{NEW,OLD}` bits to decide what to do.

* `ref_transaction_update()` (like
`ref_transaction_{create,delete,verify}()`) are public functions that
ignore the `HAVE_{NEW,OLD}` bits and base their behavior on whether
`new_sha1` and `old_sha1` are NULL.

Each of these functions has to support three possibilities for its SHA-1 arguments:

1. The SHA-1 is provided and not `null_sha1`—in this case it must match
the old value (if `old_sha1`) or it is the value to be set as the new
value (if `new_sha1`).

2. The SHA-1 is provided and is equal to `null_sha1`—in this case the
reference must not already exist (if `old_sha1` is `null_sha1`) or it
will be deleted (if `new_sha1` is `null_sha1`).

3. The SHA-1 is not provided at all—in this case the old value is
ignored (if `old_sha1` is not provided) or the reference is left
unchanged (if `new_sha1` is not provided).

Much of the current confusion stems because `ref_transaction_add_update()` encodes the third condition using the `REF_HAVE_*` bits, whereas `ref_transaction_update()` and its friends encode the third condition by setting `old_sha1` or `new_sha1` to `NULL`.

So `ref_transaction_update()` *does* need to set or clear the `HAVE_NEW` and `HAVE_OLD` bits as I sketched, to impedance-match between the two conventions.

It's a shame how much time we've wasted discussing this. Maybe the code is trying to be too clever/efficient and needs a rethink.

Michael