git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [RFC v2] refs: strip out not allowed flags from ref_transaction_update
Date: Thu, 21 Sep 2017 10:40:43 +0200	[thread overview]
Message-ID: <4ae6cb35-ecf4-e2a2-302d-95e1442cf101@alum.mit.edu> (raw)
In-Reply-To: <20170912225921.27705-1-t.gummerer@gmail.com>

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@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 :)

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

  reply	other threads:[~2017-09-21  8:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-04 20:05 [PATCH] refs: make sure we never pass NULL to hashcpy Thomas Gummerer
2017-09-06  1:26 ` Junio C Hamano
2017-09-06 20:32   ` Thomas Gummerer
2017-09-07  7:26   ` Michael Haggerty
2017-09-07 20:39     ` Thomas Gummerer
2017-09-08  0:46     ` Junio C Hamano
2017-09-08 15:08       ` Michael Haggerty
2017-09-08 17:15         ` Junio C Hamano
2017-09-12 22:59     ` [RFC v2] refs: strip out not allowed flags from ref_transaction_update Thomas Gummerer
2017-09-21  8:40       ` Michael Haggerty [this message]
2017-09-22  4:23         ` Junio C Hamano
2017-09-24 20:45         ` Thomas Gummerer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4ae6cb35-ecf4-e2a2-302d-95e1442cf101@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=t.gummerer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).