[libvirt] [PATCH] virnetdev: Resolve some Coverity issues

[John Ferlan]

On 10/29/2014 05:44 PM, Eric Blake wrote:
> On 10/29/2014 03:28 PM, John Ferlan wrote:
>> Coverity discovered a few issues with recent changes in this module
>> from commit id 'cc0e8c24'.  This patches fixes those.
> 
> Might be nice to summarize what those errors were.
> 
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virnetdev.c | 37 +++++++++++++++++++++----------------
>>  1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 127bfb2..8bd1af4 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -59,15 +59,20 @@ VIR_LOG_INIT("util.netdev");
>>  #define PROC_NET_DEV_MCAST "/proc/net/dev_mcast"
>>  #define MAX_MCAST_SIZE 50*14336
>>  #define VIR_MCAST_NAME_LEN (IFNAMSIZ + 1)
>> -#define VIR_MCAST_INDEX_TOKEN_IDX 0
>> -#define VIR_MCAST_NAME_TOKEN_IDX 1
>> -#define VIR_MCAST_USERS_TOKEN_IDX 2
>> -#define VIR_MCAST_GLOBAL_TOKEN_IDX 3
>> -#define VIR_MCAST_ADDR_TOKEN_IDX 4
>>  #define VIR_MCAST_NUM_TOKENS 5
> 
> Is VIR_MCAST_NUM_TOKENS still used after this patch?  Or can you delete
> it in favor of...
> 

yeah - forgot to go back and remove that line.

>>  #define VIR_MCAST_TOKEN_DELIMS " \n"
>>  #define VIR_MCAST_ADDR_LEN (VIR_MAC_HEXLEN + 1)
>>  
>> +typedef enum {
>> +    VIR_MCAST_TYPE_INDEX_TOKEN,
>> +    VIR_MCAST_TYPE_NAME_TOKEN,
>> +    VIR_MCAST_TYPE_USERS_TOKEN,
>> +    VIR_MCAST_TYPE_GLOBAL_TOKEN,
>> +    VIR_MCAST_TYPE_ADDR_TOKEN,
>> +
>> +    VIR_MCAST_TYPE_LAST
> 
> ...this?
> 
> 
>>  
>> -        switch (ifindex) {
>> -            case VIR_MCAST_INDEX_TOKEN_IDX:
>> +        switch ((virMCastType)ifindex) {
>> +            case VIR_MCAST_TYPE_INDEX_TOKEN:
>>                  if (virStrToLong_i(token, &endptr, 10, &num) < 0) {
>>                      virReportSystemError(EINVAL,
> 
>> @@ -2135,7 +2140,9 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast)
>>                                           buf);
>>                  }
>>                  break;
>> -            default:
>> +
>> +            /* coverity[dead_error_begin] */
>> +            case VIR_MCAST_TYPE_LAST:
>>                  break;
> 
> Okay, I can see what you did so far (convert from #defines to an enum to
> let the compiler warn if we miss any cases, including a way to silence
> Coverity about the known-dead branch of the switch).
> 

Correct - issue #1 (of 4)

>>          }
>>      }
>> @@ -2194,9 +2201,7 @@ static int virNetDevGetMcastList(const char *ifname,
>>  
>>      ret = 0;
>>   cleanup:
>> -    if (ret < 0)
>> -        virNetDevMcastListClear(mcast);
>> -
>> +    VIR_FREE(buf);
> 
> But this one doesn't have any context in the commit message at why it is
> needed.  However, in looking at the function itself and the referenced
> commit that introduced the problem, I agree with the fix.  ACK, although
> it might be better to do this as two separate commits.
> 

OK - I'll separate these into separate patches... These are coverity
issues 2, 3, & 4.  There's a followup to the Extend NIC_RX_* series that
I made just before this that pointed out that there's a path where
calling virNetDevMcastListClear in this context if we jumped here after
failing the virFileReadAll, then there'd be a NULL deref on
mcast->entries. When I looked at the code - the calling function to this
will make the same Clear call on failure.

The final 2 Coverity issues were resource leaks on buf, but Coverity
also says "cur" is leaked resulting in a twofer.

Even though you've ACK'd, I'll split things up and repost anyway for
clarity.

John