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

[Eric Blake]

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 Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120425/b7cd76e4/attachment-0001.sig>