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

[Laine Stump]

On 04/25/2012 04:46 PM, 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

Oops. I pushed before I noticed this comment.


>
>> ---
>>  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.

Or the patch that will be in the next reply to your mail?
(STATIC_ANALYSIS is always defined, but could be 0 or 1)

> 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.

It's a fairly recent change, so very likely nobody has run Coverity
against it yet.