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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list