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

[Alex Jia]

On 04/26/2012 02:57 PM, Laine Stump wrote:
> 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.
I reran coverity on commit 'bae1312 build: fix bootstrap on RHEL', however,
Coverity hasn't also complain the issue, for details, please refer to 
attachment.
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: tmp.bN9X001Ui2
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120426/35da5eca/attachment-0001.ksh>