[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]