[libvirt] [PATCH] util: portably check for unchanged uid
We've already scrubbed for comparisons of 'uid_t == -1' (which fail on platforms where uid_t is a u16), but another one snuck in. * src/util/virutil.c (virSetUIDGIDWithCaps): Correct uid comparison. * cfg.mk (sc_prohibit_risky_id_promotion): New rule. --- cfg.mk | 6 ++++++ src/util/virutil.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index b95a90b..394521e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -389,6 +389,12 @@ sc_prohibit_setuid: halt='use virSetUIDGID, not raw set*id' \ $(_sc_search_regexp) +# Don't compare *id_t against raw -1. +sc_prohibit_risky_id_promotion: + @prohibit='\b(user|group|[ug]id) *[=!]= *-' \ + halt='cast -1 to ([ug]id_t) before comparing against id' \ + $(_sc_search_regexp) + # Use snprintf rather than s'printf, even if buffer is provably large enough, # since gnulib has more guarantees for snprintf portability sc_prohibit_sprintf: diff --git a/src/util/virutil.c b/src/util/virutil.c index a0d1530..42b4295 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3011,7 +3011,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits, * change the capabilities bounding set. */ - if (clearExistingCaps || (uid != -1 && uid != 0)) + if (clearExistingCaps || (uid != (uid_t)-1 && uid != 0)) capng_clear(CAPNG_SELECT_BOTH); for (ii = 0; ii <= CAP_LAST_CAP; ii++) { -- 1.8.1.4
On 03/14/2013 06:35 PM, Eric Blake wrote:
We've already scrubbed for comparisons of 'uid_t == -1' (which fail on platforms where uid_t is a u16), but another one snuck in.
ACK (and sorry for the botched "bugfix" :-/)
* src/util/virutil.c (virSetUIDGIDWithCaps): Correct uid comparison. * cfg.mk (sc_prohibit_risky_id_promotion): New rule. --- cfg.mk | 6 ++++++ src/util/virutil.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index b95a90b..394521e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -389,6 +389,12 @@ sc_prohibit_setuid: halt='use virSetUIDGID, not raw set*id' \ $(_sc_search_regexp)
+# Don't compare *id_t against raw -1. +sc_prohibit_risky_id_promotion: + @prohibit='\b(user|group|[ug]id) *[=!]= *-' \ + halt='cast -1 to ([ug]id_t) before comparing against id' \ + $(_sc_search_regexp) +
As we discussed on IRC, I'm slightly concerned about false positives when user, group, or [gu]id is used as something other than [gu]id_t, but the most common case of this would be for a char*, and I doubt we would ever be comparing a char* with !=, so I think it's okay (certainly better than the alternative of pushing a release only to find that we don't compile on some platform).
On 03/15/2013 10:47 AM, Laine Stump wrote:
On 03/14/2013 06:35 PM, Eric Blake wrote:
We've already scrubbed for comparisons of 'uid_t == -1' (which fail on platforms where uid_t is a u16), but another one snuck in.
ACK (and sorry for the botched "bugfix" :-/)
+# Don't compare *id_t against raw -1. +sc_prohibit_risky_id_promotion: + @prohibit='\b(user|group|[ug]id) *[=!]= *-' \ + halt='cast -1 to ([ug]id_t) before comparing against id' \ + $(_sc_search_regexp) +
As we discussed on IRC, I'm slightly concerned about false positives when user, group, or [gu]id is used as something other than [gu]id_t, but the most common case of this would be for a char*, and I doubt we would ever be comparing a char* with !=,
Well, there's always 'group == NULL' or 'group != NULL'. But neither of those patterns match the '-' of '-1' on the other side of the comparison, so they won't flag as a syntax error.
so I think it's okay (certainly better than the alternative of pushing a release only to find that we don't compile on some platform).
Actually, it's worse than not compiling - it's a silent difference in behavior. Omitting the cast has a valid compilation regardless of the size of uid_t, it's just that the rules for how C99 requires it to behave are not intuitive to a naive programmer who doesn't realize that the size of uid_t plays a role in what behavior results, and that the behavior is different for 32-bit vs. 16-bit types. If we could actually make it a compiler error (such as with a -Wfoo -Werror combination), that would be nicer than a syntax check, but as far as I know, gcc doesn't have anything to help us out on that front. One thing I did not check for is yoda-isms like '-1 == uid'; but as we already actively discourage that coding style, I don't think we have to worry about missing any of those violations. Thanks for the review, and pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump