refs: make sure we never pass NULL to hashcpy
PATCH 1/1 refs: make sure we never pass NULL to hashcpy
gcc on arch linux (version 7.1.1) warns that a NULL argument is passed as the second parameter of memcpy.
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,
^~~~~~Tracking this error down, we can track it back to ref_transaction_add_update. where the call to hashcpy is however protected by the flags that are passed in.
To make sure there's no code path where the wrong flags are passed in, and to help the compiler realize that no NULL parameter is passed as second argument to hashcpy, add asserts that this is indeed the case.
---
This is based on top of ma/ts-cleanups, as that fixes another compiler warning with gcc 7.1.1.
refs.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
refs.c +6 -2
@@ -896,10 +896,14 @@ struct ref_update *ref_transaction_add_update( update->flags = flags; -if (flags &REF_HAVE_NEW ) +if (flags &REF_HAVE_NEW ) { +assert (new_sha1);hashcpy (update->new_oid.hash, new_sha1); -if (flags &REF_HAVE_OLD ) + } +if (flags &REF_HAVE_OLD ) { +assert (old_sha1);hashcpy (update->old_oid.hash, old_sha1); + } update->msg =xstrdup_or_null (msg);return update; }
Signature · 1 line
2.14.1.480.gb18f417b89
Quoted 23 lines · Thomas Gummerer
gcc on arch linux (version 7.1.1) warns that a NULL argument is passed as the second parameter of memcpy.
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, ^~~~~~...
refs.c +0
-0@@ -896,10 +896,14 @@ struct ref_update *ref_transaction_add_update( update->flags = flags;
refs.c struct ref_update *ref_transaction_add_update( · line 906
Quoted 10 lines
refs.c +6
-2-if (flags &REF_HAVE_NEW ) +if (flags &REF_HAVE_NEW ) { +assert (new_sha1);hashcpy (update->new_oid.hash, new_sha1); -if (flags &REF_HAVE_OLD ) + } +if (flags &REF_HAVE_OLD ) { +assert (old_sha1);hashcpy (update->old_oid.hash, old_sha1); + }
It is hugely annoying to see a halfway-intelligent compiler forces you to add such pointless asserts.
The only way the compiler could error on this is by inferring the fact that new_sha1/old_sha1 could be NULL by looking at the callsite in ref_transaction_update() where these are used as conditionals to set HAVE_NEW/HAVE_OLD that are passed. Even if the compiler were doing the whole-program analysis, the other two callsites of the function pass the address of oid.hash[] in an oid structure so it should know these won't be NULL.
Or is the compiler being really dumb and triggering an error for every use of
memcpy(dst, src, size);
that must now be written as
assert(src); memcpy(dst, src, size);
??? That would be doubly annoying.
I wonder if REF_HAVE_NEW/REF_HAVE_OLD are really needed in these codepaths, though. Perhaps we can instead declare !!new_sha1 means we have the new side and rewrite the above part to
if (new_sha1) hashcpy(update->new_oid.hash, new_sha1);
without an extra and totally pointless assert()?
Quoted 3 lines
refs.c +0
-0update->msg =xstrdup_or_null (msg);return update; }
Quoted 50 lines · Junio C Hamano
Quoted 23 lines · Thomas Gummerer
gcc on arch linux (version 7.1.1) warns that a NULL argument is passed as the second parameter of memcpy.
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, ^~~~~~...
refs.c +0
-0@@ -896,10 +896,14 @@ struct ref_update *ref_transaction_add_update( update->flags = flags;refs.c struct ref_update *ref_transaction_add_update( · line 906
Quoted 10 lines
refs.c +6
-2-if (flags &REF_HAVE_NEW ) +if (flags &REF_HAVE_NEW ) { +assert (new_sha1);hashcpy (update->new_oid.hash, new_sha1); -if (flags &REF_HAVE_OLD ) + } +if (flags &REF_HAVE_OLD ) { +assert (old_sha1);hashcpy (update->old_oid.hash, old_sha1); + }It is hugely annoying to see a halfway-intelligent compiler forces you to add such pointless asserts.
The only way the compiler could error on this is by inferring the fact that new_sha1/old_sha1 could be NULL by looking at the callsite in
ref_transaction_update()where these are used as conditionals to set HAVE_NEW/HAVE_OLD that are passed. Even if the compiler were doing the whole-program analysis, the other two callsites of the function pass the address of oid.hash[] in an oid structure so it should know these won't be NULL.Or is the compiler being really dumb and triggering an error for every use of
Quoted 8 lines
memcpy(dst, src, size);that must now be written as
assert(src); memcpy(dst, src, size);??? That would be doubly annoying
No, I think it can't quite deal with the flags that are passed in. I'm on a different
machine today, so I can't actually check, but that's what I would expect at least.
Quoted 8 lines
I wonder if REF_HAVE_NEW/REF_HAVE_OLD are really needed in these codepaths, though. Perhaps we can instead declare !!new_sha1 means we have the new side and rewrite the above part to
if (new_sha1) hashcpy(update->new_oid.hash, new_sha1);without an extra and totally pointless
assert()?
Yeah, that seems much nicer. I'll try that and send a new a patch (though I won't
get to it before tomorrow). Thanks for the review.
Quoted 3 lines
Quoted 3 lines
refs.c +0
-0update->msg =xstrdup_or_null (msg);return update; }
Quoted 19 lines · Junio C Hamano
Quoted 3 lines · Thomas Gummerer
gcc on arch linux (version 7.1.1) warns that a NULL argument is passed as the second parameter of memcpy.
[...]It is hugely annoying to see a halfway-intelligent compiler forces you to add such pointless asserts.
The only way the compiler could error on this is by inferring the fact that new_sha1/old_sha1 could be NULL by looking at the callsite in
ref_transaction_update()where these are used as conditionals to set HAVE_NEW/HAVE_OLD that are passed. Even if the compiler were doing the whole-program analysis, the other two callsites of the function pass the address of oid.hash[] in an oid structure so it should know these won't be NULL.[...]
Quoted 8 lines
I wonder if REF_HAVE_NEW/REF_HAVE_OLD are really needed in these codepaths, though. Perhaps we can instead declare !!new_sha1 means we have the new side and rewrite the above part to
if (new_sha1) hashcpy(update->new_oid.hash, new_sha1);without an extra and totally pointless
assert()?
The ultimate reason for those flags is that struct ref_update embeds new_oid and old_oid directly in the struct, so there is no way to set it to "NULL". (The is_null_sha1 value is used for a different purpose.) So those flags keep track of whether the corresponding value is specified or absent.
Four of the five callers of ref_transaction_add_update() are constructing a new ref_update from an old one. They currently don't have to look into flags; they just pass it on (possibly changing a bit or two). Implementing your proposal would oblige those callers to change from something like
Quoted 5 lines
new_update= ref_transaction_add_update(transaction, "HEAD", update->flags | REF_LOG_ONLY | REF_NODEREF, update->new_oid.hash, update->old_oid.hash, update->msg);
to
Quoted 6 lines
new_update= ref_transaction_add_update(transaction, "HEAD", update->flags | REF_LOG_ONLY | REF_NODEREF, (update->flags & REF_HAVE_NEW) ? update->new_oid.hash : NULL, (update->flags & REF_HAVE_OLD) ? update->old_oid.hash : NULL, update->msg);
It's not the end of the world, but it's annoying. ref_transaction_add_update() was meant to be a low-level, low-overhead way of allocating a struct ref_update and add it to a transaction.
Another solution (also annoying, but maybe a tad less so) would be to change the one iffy caller, ref_transaction_update(), to pass in a pointer to the null OID for new_sha1 and old_sha1 when the corresponding flags are turned off. That value would never be looked at, but it would hopefully reassure gcc.
I did just realize one thing: ref_transaction_update() takes flags as an argument and alters it using
Quoted 1 line
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
Quoted 8 lines
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. 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:
Quoted 1 line
flags &=
REF_ALLOWED_FLAGS; /* value would need to be determined */
Michael
Quoted 35 lines · Michael Haggerty
Quoted 19 lines · Junio C Hamano
Quoted 3 lines · Thomas Gummerer
gcc on arch linux (version 7.1.1) warns that a NULL argument is passed as the second parameter of memcpy.
[...]It is hugely annoying to see a halfway-intelligent compiler forces you to add such pointless asserts.
The only way the compiler could error on this is by inferring the fact that new_sha1/old_sha1 could be NULL by looking at the callsite in
ref_transaction_update()where these are used as conditionals to set HAVE_NEW/HAVE_OLD that are passed. Even if the compiler were doing the whole-program analysis, the other two callsites of the function pass the address of oid.hash[] in an oid structure so it should know these won't be NULL.[...]
Quoted 8 lines
I wonder if REF_HAVE_NEW/REF_HAVE_OLD are really needed in these codepaths, though. Perhaps we can instead declare !!new_sha1 means we have the new side and rewrite the above part to
if (new_sha1) hashcpy(update->new_oid.hash, new_sha1);without an extra and totally pointless
assert()?The ultimate reason for those flags is that
struct ref_updateembedsnew_oidandold_oiddirectly in the struct, so there is no way to set it to "NULL". (Theis_null_sha1value is used for a different purpose.) So those flags keep track of whether the corresponding value is specified or absent.
Quoted 5 lines
Four of the five callers of
ref_transaction_add_update()are constructing a newref_updatefrom an old one. They currently don't have to look intoflags; they just pass it on (possibly changing a bit or two). Implementing your proposal would oblige those callers to change from something like
Thanks for the explanation!
Quoted 46 lines
Quoted 5 lines
new_update= ref_transaction_add_update(transaction, "HEAD", update->flags | REF_LOG_ONLY | REF_NODEREF, update->new_oid.hash, update->old_oid.hash, update->msg);to
Quoted 6 lines
new_update= ref_transaction_add_update(transaction, "HEAD", update->flags | REF_LOG_ONLY | REF_NODEREF, (update->flags & REF_HAVE_NEW) ? update->new_oid.hash : NULL, (update->flags & REF_HAVE_OLD) ? update->old_oid.hash : NULL, update->msg);It's not the end of the world, but it's annoying.
ref_transaction_add_update()was meant to be a low-level, low-overhead way of allocating astruct ref_updateand add it to a transaction.Another solution (also annoying, but maybe a tad less so) would be to change the one iffy caller,
ref_transaction_update(), to pass in a pointer to the null OID fornew_sha1andold_sha1when the corresponding flags are turned off. That value would never be looked at, but it would hopefully reassure gcc.I did just realize one thing:
ref_transaction_update()takesflagsas an argument and alters it usingQuoted 1 line
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
flagsargument passed in by the caller might *already* have one of these bits set. In that caseref_transaction_add_update()would indeed be called incorrectly. Does the warning go away if you change that line toQuoted 8 lines
if (new_sha1) flags |=REF_HAVE_NEW; else flags &= ~REF_HAVE_NEW; if (old_sha1) flags |=REF_HAVE_OLD; else flags &= ~REF_HAVE_OLD;
Quoted 1 line
?
Indeed that fixes it, great catch! gcc is indeed smarter than we gave it credit for, this makes a lot of sense.
Interestingly stripping away the flags fixes the compiler warning:
refs.c +3 -0
@@ -921,6 +921,9 @@ int ref_transaction_update(struct ref_transaction *transaction,return -1 ; } + flags &= ~REF_HAVE_NEW ; + flags &= ~REF_HAVE_OLD ; + flags |= (new_sha1 ?REF_HAVE_NEW :0 ) | (old_sha1 ?REF_HAVE_OLD :0 ); ref_transaction_add_update(transaction, refname, flags,
while checking that the flags are not there in the first place still leaves the compiler warning (whether I use "die()" or just "return -1" doesn't matter in this case):
refs.c +3 -0
@@ -921,6 +921,9 @@ int ref_transaction_update(struct ref_transaction *transaction,return -1 ; } +if ((flags &REF_HAVE_NEW ) !=0 || (flags &REF_HAVE_OLD ) !=0 ) +die ("BUG: passed invalid flag to ref_transaction_update" ); + flags |= (new_sha1 ?REF_HAVE_NEW :0 ) | (old_sha1 ?REF_HAVE_OLD :0 ); ref_transaction_add_update(transaction, refname, flags,
Quoted 7 lines
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 theflagsargument toref_transaction_update()'s callers:Quoted 1 line
flags &=
REF_ALLOWED_FLAGS; /* value would need to be determined */
Interesting idea, would we just strip out the flags in this case, or should we check that the flags are not there in the first place and error out if they are?
I guess with the second option we would make sure that every caller cleans up the flags before calling the function in the first place, but also have a small risk of missing something if we don't test a particular codepath. I stripping out the flags might actually fix the compiler warning as well, so that might be the better option here. I'm happy to try and patch this based on the suggestion above, but most likely it won't happen before next week, as I'm traveling over the weekend :)
Quoted 1 line
Michael
Quoted 20 lines · Michael Haggerty
I did just realize one thing:
ref_transaction_update()takesflagsas an argument and alters it usingQuoted 1 line
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
flagsargument passed in by the caller might *already* have one of these bits set. In that caseref_transaction_add_update()would indeed be called incorrectly. Does the warning go away if you change that line toQuoted 8 lines
if (new_sha1) flags |=REF_HAVE_NEW; else flags &= ~REF_HAVE_NEW; if (old_sha1) flags |=REF_HAVE_OLD; else flags &= ~REF_HAVE_OLD;
Quoted 2 lines
? 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?
Quoted 33 lines · Junio C Hamano
Quoted 20 lines · Michael Haggerty
I did just realize one thing:
ref_transaction_update()takesflagsas an argument and alters it usingQuoted 1 line
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
flagsargument passed in by the caller might *already* have one of these bits set. In that caseref_transaction_add_update()would indeed be called incorrectly. Does the warning go away if you change that line toQuoted 8 lines
if (new_sha1) flags |=REF_HAVE_NEW; else flags &= ~REF_HAVE_NEW; if (old_sha1) flags |=REF_HAVE_OLD; else flags &= ~REF_HAVE_OLD;Quoted 2 lines
? This might be a nice change to have anyway, to isolate
ref_transaction_update()from mistakes by its callers.I understand "drop
HAVE_NEWbit ifnew_sha1is NULL" part, but not the other side "addHAVE_NEWifnew_SHA1is not NULL"---doesn't the NEW/OLD flag exist exactly because some callers pass the address of an embedded oid.hash[] ornull_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?
Quoted 4 lines
ref_transaction_createand_deletepassnull_sha1on the missing side, whileref_transaction_verifypasses 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
Quoted 3 lines · Michael Haggerty
So
ref_transaction_update()*does* need to set or clear theHAVE_NEWandHAVE_OLDbits as I sketched, to impedance-match between the two conventions.
OK, so ignoring HAVE_NEW/HAVE_OLD bits that the callers of ref_transaction_update() may set in flags, and having ref_transaction_update() compute these bits based on new/old_sha1 pointers from scratch, would be the right thing to do.
IOW
flags &= ~(REF_HAVE_NEW|REF_HAVE_OLD); if (new_sha1) flags |= REF_HAVE_NEW; if (old_sha1) flags |= REF_HAVE_OLD;
and your earlier "Does the warning go away if you change the line to" does essentially the same thing.
Quoted 2 lines
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.
It might be the case, but I do not know what to blame is "the two conventions", an over-eager compiler, or a confused commenter on the thread (that's me), though ;-).
RFC v2 1/1 refs: strip out not allowed flags from ref_transaction_update
In reply to Michael HaggertyCallers 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.
---
Quoted 7 lines
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 theflagsargument toref_transaction_update()'s callers:Quoted 1 line
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(+)
refs.c +2 -0
@@ -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,
refs.h +8 -0
@@ -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. */
Signature · 1 line
2.14.1.480.gb18f417b89
Re: [RFC v2] refs: strip out not allowed flags from ref_transaction_update
Quoted 15 lines · Thomas Gummerer
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,
Quoted 7 lines
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_updateis protected by the
s/hascpy/hashcpy/
Quoted 22 lines
passed in flags, but as we only add flags there, gcc notices
REF_HAVE_NEWorREF_HAVE_OLDflags 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 <[email protected]>Signed-off-by Thomas Gummerer <[email protected]>---
Quoted 7 lines
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 theflagsargument toref_transaction_update()'s callers:Quoted 1 line
flags &=
REF_ALLOWED_FLAGS; /* value would need to be determined */Here's my attempt at doing this.
Quoted 8 lines
The odd flag out as the flag that's advertised as internal but can't stripped out is
REF_ISPRUNING.REF_ISPRUNINGis 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.
Quoted 9 lines
refs.c | 2 ++ refs.h | 8 ++++++++ 2 files changed, 10 insertions(+)refs.c +0
-0
refs.c int ref_transaction_update(struct ref_transaction *transaction, · line 925
Quoted 5 lines
refs.c +2
-0@@ -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?
Quoted 13 lines
refs.c +0
-0flags |= (new_sha1 ?REF_HAVE_NEW :0 ) | (old_sha1 ?REF_HAVE_OLD :0 ); ref_transaction_add_update(transaction, refname, flags,refs.h +1
-0@@ -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 +/*
refs.h int refs_pack_refs(struct ref_store *refs, unsigned int flags); · line 367
Quoted 10 lines
refs.h +7
-0+* 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
Re: [RFC v2] refs: strip out not allowed flags from ref_transaction_update
Quoted 15 lines · Michael Haggerty
I wouldn't worry too much about this anomaly.
REF_ISPRUNINGis an ugly internal kludge, but allowing it in the mask doesn't make anything worse.Quoted 12 lines
refs.c | 2 ++ refs.h | 8 ++++++++ 2 files changed, 10 insertions(+)refs.c +0
-0@@ -921,6 +921,8 @@ int ref_transaction_update(struct ref_transaction *transaction,return -1 ; }
Quoted 8 lines
refs.c int ref_transaction_update(struct ref_transaction *transaction, · line 925
Quoted 2 lines
refs.c +2
-0+ 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);
It sounds like a sensible thing to do. Thanks.
Re: [RFC v2] refs: strip out not allowed flags from ref_transaction_update
In reply to Michael HaggertyQuoted 22 lines · Michael Haggerty
Quoted 21 lines · Thomas Gummerer
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, ^~~~~~
Quoted 3 lines
Quoted 1 line
The call to hascpy in
ref_transaction_add_updateis protected by thes/hascpy/hashcpy/
Argh thanks, good catch. I see Junio got my original version already in next, so I guess fixing it now is not really an option anymore :(
Quoted 26 lines
Quoted 26 lines
passed in flags, but as we only add flags there, gcc notices
REF_HAVE_NEWorREF_HAVE_OLDflags 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 <[email protected]>Signed-off-by Thomas Gummerer <[email protected]>---
Quoted 7 lines
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 theflagsargument toref_transaction_update()'s callers:Quoted 1 line
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_ISPRUNINGis passed in as argument to 'ref_transaction_delete()' in 'prune_ref()'.
Quoted 7 lines
Quoted 4 lines
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_ISPRUNINGis an ugly internal kludge, but allowing it in the mask doesn't make anything worse.
Makes sense, thanks!
Quoted 18 lines
Quoted 9 lines
refs.c | 2 ++ refs.h | 8 ++++++++ 2 files changed, 10 insertions(+)refs.c +0
-0refs.c int ref_transaction_update(struct ref_transaction *transaction, · line 925
Quoted 5 lines
refs.c +2
-0@@ -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:
Quoted 4 lines
if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
BUG("illegal flags %x in ref_transaction_update", flags);Would this also squelch the compiler warning?
I thought about this, and I agree that it would be nicer to do this way. However unfortunately the compiler is not smart enough to realize that this is gives the same guarantees as stripping the flags out, and thus still emits the warning. This is why I went with just stripping them out.
I'm not sure if it's worth additionally adding this check? I think it would read a bit weirdly to check whether the flags are there and to then "strip" them out when we already know they are not there.
Quoted 11 lines
Quoted 11 lines
refs.c +0
-0flags |= (new_sha1 ?REF_HAVE_NEW :0 ) | (old_sha1 ?REF_HAVE_OLD :0 ); ref_transaction_add_update(transaction, refname, flags,refs.h +0
-0@@ -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
Quoted 14 lines
refs.h int refs_pack_refs(struct ref_store *refs, unsigned int flags); · line 367
Quoted 12 lines
refs.h +8
-0+/* +* 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.
Thanks for the review!
Quoted 1 line
Michael