On 04/26/2012 02:12 AM, Alex Jia wrote: > On 04/26/2012 04:46 AM, Eric Blake wrote: >> On 04/25/2012 02:01 PM, Laine Stump wrote: >>> This patch resolves https://bugzilla.redhat.com/show_bug.cgi?id=815270 >>> >>> The function virNetDevMacVLanVPortProfileRegisterCallback() takes an >>> arg "virtPortProfile", and was checking it for non-NULL before using >>> it. However, the prototype for >>> virNetDevMacVLanPortProfileRegisterCallback had marked that arg with >>> ATTRIBUTE_NONNULL(). Contrary to what one may think, >>> ATTRIBUTE_NONNULL() does not provide any guarantee that an arg marked >>> as such really is always non-null; the only effect to the code >>> generated by gcc, is that gcc *assumes* it is non-NULL; this results >>> in, for example, the check for a non-NULL value being optimized out. >>> >>> (Unfortunately, this code removal only occurs when optimization is >>> enabled, and I am in the habit of doing local builds with optimization >>> off to ease debugging, so the bug didn't show up in my earlier local >>> testing). >>> >>> In general, virPortProfile might always be NULL, so it shouldn't be >>> marked as ATTRIBUTE_NONNULL. One other function prototype made this >>> same error, so this patch fixes it as well. >> Might be worth linking to >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 >> >>> --- >>> src/util/virnetdevmacvlan.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >> ACK. What an insidious bug. >> >> As Laine and I discussed on IRC, I'm half wondering if we should just do: >> >> #ifdef STATIC_ANALYSIS && /* attributes supported */ >> # define ATTRIBUTE_NONNULL(n) __attribute__((__nonnull__(n))) >> #else >> # define ATTRIBUTE_NONNULL(n) /* empty, due to gcc lameness */ >> #endif >> >> so that our code will be pessimized under normal compiles, but _at >> least_ places where we have bugs with improper use of the attribute >> won't cause gcc to miscompile things; but still let us get NULL checking >> when running clang or Coverity. >> >> I also wonder if this has been detected by Coverity (checking a nonnull >> parameter for NULL is dead code, which Coverity does tend to flag), and >> we just haven't been following Coverity closely enough to notice. > Eric, I ran Coverity on current commit 'f78024b util: fix crash when > starting macvtap interfaces', > Coverity hasn't complain this issue, although I also enabled > '--security' checkers in Coverity. The interesting run would be *before* this commit. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list