Re: [libvirt] [PATCH] virnetdev: Resolve some Coverity issues
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(a)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