Re: [libvirt] [PATCH] util: fix crash when starting macvtap interfaces

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.