Re: [RFC v2] refs: strip out not allowed flags from ref_transaction_update

> 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

> 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@alum.mit.edu>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> 
>> 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 :)

>  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;
> +

>  	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.
>   */
>