[libvirt] [PATCH v2 0/3] virnetdev: Resolve some Coverity issues
A reposting of a patch into 3 separate patches for each of the Coverity issues found from commit id 'cc0e8c24'. The changes were already ACK'd, but I figured I'd repost anyway. v1: http://www.redhat.com/archives/libvir-list/2014-October/msg01012.html John Ferlan (3): virnetdev: Resolve Coverity DEADCODE virnetdev: Resolve Coverity FORWARD_NULL virnetdev: Resolve Coverity RESOURCE_LEAK src/util/virnetdev.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) -- 1.9.3
Coverity complains that because the for loop is from 0 to 5 (max tokens) and the impending switch/case statements used each of the #define values that the 'default' wouldn't reachable. This patch will convert the #define's into enum's and add the obligatory dead_error_begin marker for these type situations. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 127bfb2..36e84d7 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -59,15 +59,19 @@ 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 #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 +} virMCastType; + typedef struct _virNetDevMcastEntry virNetDevMcastEntry; typedef virNetDevMcastEntry *virNetDevMcastEntryPtr; struct _virNetDevMcastEntry { @@ -2076,7 +2080,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) char *saveptr; char *endptr; - for (ifindex = 0, next = buf; ifindex < VIR_MCAST_NUM_TOKENS; ifindex++, + for (ifindex = 0, next = buf; ifindex < VIR_MCAST_TYPE_LAST; ifindex++, next = NULL) { token = strtok_r(next, VIR_MCAST_TOKEN_DELIMS, &saveptr); @@ -2087,8 +2091,8 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) return -1; } - 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, _("Failed to parse interface index from '%s'"), @@ -2098,7 +2102,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) } mcast->index = num; break; - case VIR_MCAST_NAME_TOKEN_IDX: + case VIR_MCAST_TYPE_NAME_TOKEN: if (virStrncpy(mcast->name, token, strlen(token), VIR_MCAST_NAME_LEN) == NULL) { virReportSystemError(EINVAL, @@ -2107,7 +2111,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) return -1; } break; - case VIR_MCAST_USERS_TOKEN_IDX: + case VIR_MCAST_TYPE_USERS_TOKEN: if (virStrToLong_i(token, &endptr, 10, &num) < 0) { virReportSystemError(EINVAL, _("Failed to parse users from '%s'"), @@ -2117,7 +2121,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) } mcast->users = num; break; - case VIR_MCAST_GLOBAL_TOKEN_IDX: + case VIR_MCAST_TYPE_GLOBAL_TOKEN: if (virStrToLong_i(token, &endptr, 10, &num) < 0) { virReportSystemError(EINVAL, _("Failed to parse users from '%s'"), @@ -2127,7 +2131,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) } mcast->global = num; break; - case VIR_MCAST_ADDR_TOKEN_IDX: + case VIR_MCAST_TYPE_ADDR_TOKEN: if (virMacAddrParseHex((const char*)token, &mcast->macaddr) < 0) { virReportSystemError(EINVAL, @@ -2135,7 +2139,9 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) buf); } break; - default: + + /* coverity[dead_error_begin] */ + case VIR_MCAST_TYPE_LAST: break; } } -- 1.9.3
The complaint is that if cleanup is called when virFileReadAll fails, then mcast->entries is NULL and could be dereferenced in the clear function. After following the code some - I saw that the caller to the function (virNetDevGetMulticastTable) will also call virNetDevMcastListClear if this function returns -1, so this isn't necessary, so I removed the call. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 36e84d7..4fea4cb 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2200,9 +2200,6 @@ static int virNetDevGetMcastList(const char *ifname, ret = 0; cleanup: - if (ret < 0) - virNetDevMcastListClear(mcast); - VIR_FREE(entry); return ret; -- 1.9.3
virFileReadAll returns a chunk of memory that needs to be free'd when done Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 4fea4cb..0c9c1f9 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2200,6 +2200,7 @@ static int virNetDevGetMcastList(const char *ifname, ret = 0; cleanup: + VIR_FREE(buf); VIR_FREE(entry); return ret; -- 1.9.3
On 10/29/2014 04:50 PM, John Ferlan wrote:
A reposting of a patch into 3 separate patches for each of the Coverity issues found from commit id 'cc0e8c24'. The changes were already ACK'd, but I figured I'd repost anyway.
Thanks; looks better, and good for 1.2.10.
v1:
http://www.redhat.com/archives/libvir-list/2014-October/msg01012.html
John Ferlan (3): virnetdev: Resolve Coverity DEADCODE virnetdev: Resolve Coverity FORWARD_NULL virnetdev: Resolve Coverity RESOURCE_LEAK
src/util/virnetdev.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
John Ferlan