On 09/13/2017 12:59 AM, Thomas Gummerer wrote:
> Callers are only allowed to pass certain flags into
> ref_transaction_update, other flags are internal to it. To prevent
> mistakes from the callers, strip the internal only flags out before
> continuing.
>
> This was noticed because of a compiler warning gcc 7.1.1 issued about
> passing a NULL parameter as second parameter to memcpy (through
> hashcpy):
>
> In file included from refs.c:5:0:
> refs.c: In function ‘ref_transaction_verify’:
> cache.h:948:2: error: argument 2 null where non-null expected [-Werror=nonnull]
> memcpy(sha_dst, sha_src, GIT_SHA1_RAWSZ);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from git-compat-util.h:165:0,
> from cache.h:4,
> from refs.c:5:
> /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared here
> extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
> ^~~~~~
>
> The call to hascpy in ref_transaction_add_update is protected by the
s/hascpy/hashcpy/
> passed in flags, but as we only add flags there, gcc notices
> REF_HAVE_NEW or REF_HAVE_OLD flags could be passed in from the outside,
> which would potentially result in passing in NULL as second parameter to
> memcpy.
>
> Fix both the compiler warning, and make the interface safer for its
> users by stripping the internal flags out.
>
> Suggested-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
> Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>
> ---
>
>> This might be a nice change to have anyway, to isolate
>> `ref_transaction_update()` from mistakes by its callers. For that
>> matter, one might want to be even more selective about what bits are
>> allowed in the `flags` argument to `ref_transaction_update()`'s
>> callers:
>>
>>> flags &= REF_ALLOWED_FLAGS; /* value would need to be determined */
>
> Here's my attempt at doing this.
>
> The odd flag out as the flag that's advertised as internal but can't
> stripped out is REF_ISPRUNING. REF_ISPRUNING is passed in as argument
> to 'ref_transaction_delete()' in 'prune_ref()'.
>
> Maybe this flag should be public, or maybe I'm missing something here?
> Having only this internal flags as part of the allowed flags feels a
> bit ugly, but I'm also unfamiliar with the refs code, hence the RFC.
> If someone has more suggestions they would be very welcome :)
I wouldn't worry too much about this anomaly. `REF_ISPRUNING` is an ugly
internal kludge, but allowing it in the mask doesn't make anything worse.
> refs.c | 2 ++
> refs.h | 8 ++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index ba22f4acef..fad61be1da 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -921,6 +921,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
> return -1;
> }
>
> + flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
> +
I would advocate considering it a bug if the caller passes in options
that we are going to ignore anyway:
if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
BUG("illegal flags %x in ref_transaction_update", flags);
Would this also squelch the compiler warning?
> flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0);
>
> ref_transaction_add_update(transaction, refname, flags,
> diff --git a/refs.h b/refs.h
> index 6daa78eb50..4d75c207e1 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -354,6 +354,14 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags);
> #define REF_NODEREF 0x01
> #define REF_FORCE_CREATE_REFLOG 0x40
>
> +/*
> + * Flags that can be passed in to ref_transaction_update
> + */
> +#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
> + REF_ISPRUNING | \
> + REF_FORCE_CREATE_REFLOG | \
> + REF_NODEREF
> +
> /*
> * Setup reflog before using. Fill in err and return -1 on failure.
> */
>
Thanks for working on this.
Michael
