So far, this function has just three callers. Two of them call virNetDevSetupControl to create a socket that we can then optionally use for ioctl() to fetch data. However, querying sysfs is preferred. Therefore it doesn't make much sense to require users to set up the socket if they don't even know it will be used in favour of sysfs. We can set up the socket iff we need to. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdevbridge.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index ef1f4cc..ebe9dba 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -169,12 +169,12 @@ static int virNetDevBridgeSet(const char *brname, static int virNetDevBridgeGet(const char *brname, const char *paramname, /* sysfs param name */ - unsigned long *value, /* current value */ - int fd, /* control socket */ - struct ifreq *ifr) /* pre-filled bridge name */ + unsigned long *value) /* current value */ { char *path = NULL; int ret = -1; + int fd = -1; + struct ifreq ifr; if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname, paramname) < 0) return -1; @@ -196,7 +196,11 @@ static int virNetDevBridgeGet(const char *brname, } else { struct __bridge_info info; unsigned long args[] = { BRCTL_GET_BRIDGE_INFO, (unsigned long)&info, 0, 0 }; - ifr->ifr_data = (char*)&args; + + if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) + goto cleanup; + + ifr.ifr_data = (char*)&args; if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) { virReportSystemError(errno, _("Unable to get bridge %s %s"), brname, paramname); @@ -216,6 +220,7 @@ static int virNetDevBridgeGet(const char *brname, ret = 0; cleanup: + VIR_FORCE_CLOSE(fd); VIR_FREE(path); return ret; } @@ -825,20 +830,12 @@ int virNetDevBridgeSetSTPDelay(const char *brname, int virNetDevBridgeGetSTPDelay(const char *brname, int *delayms) { - int fd = -1; int ret = -1; - struct ifreq ifr; unsigned long val; - if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) - goto cleanup; - - ret = virNetDevBridgeGet(brname, "forward_delay", &val, - fd, &ifr); + ret = virNetDevBridgeGet(brname, "forward_delay", &val); *delayms = JIFFIES_TO_MS(val); - cleanup: - VIR_FORCE_CLOSE(fd); return ret; } @@ -885,20 +882,12 @@ int virNetDevBridgeSetSTP(const char *brname, int virNetDevBridgeGetSTP(const char *brname, bool *enabled) { - int fd = -1; int ret = -1; - struct ifreq ifr; unsigned long val; - if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) - goto cleanup; - - ret = virNetDevBridgeGet(brname, "stp_state", &val, - fd, &ifr); + ret = virNetDevBridgeGet(brname, "stp_state", &val); *enabled = val ? true : false; - cleanup: - VIR_FORCE_CLOSE(fd); return ret; } #elif defined(HAVE_BSD_BRIDGE_MGMT) @@ -1000,7 +989,7 @@ virNetDevBridgeGetVlanFiltering(const char *brname, int ret = -1; unsigned long value; - if (virNetDevBridgeGet(brname, "vlan_filtering", &value, -1, NULL) < 0) + if (virNetDevBridgeGet(brname, "vlan_filtering", &value) < 0) goto cleanup; *enable = !!value; -- 2.8.3

cpu/cpu_ppc64.c: In function 'ppc64Compute': cpu/cpu_ppc64.c:620:27: error: potential null pointer dereference [-Werror=null-dereference] if (STRNEQ(guest_model->name, host_model->name)) { ~~~~~~~~~~~^~~ cpu/cpu_ppc64.c:620:9: note: in expansion of macro 'STRNEQ' if (STRNEQ(guest_model->name, host_model->name)) { ^~~~~~ cc1: all warnings being treated as errors Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/cpu/cpu_ppc64.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 799fb8e..09f4596 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -600,23 +600,23 @@ ppc64Compute(virCPUDefPtr host, case VIR_CPU_MODE_HOST_PASSTHROUGH: /* host-model and host-passthrough: * the guest CPU is the same as the host */ - if (!(guest_model = ppc64ModelCopy(host_model))) - goto cleanup; + guest_model = ppc64ModelCopy(host_model); break; case VIR_CPU_MODE_CUSTOM: /* custom: * look up guest CPU information */ - if (!(guest_model = ppc64ModelFromCPU(cpu, map))) - goto cleanup; + guest_model = ppc64ModelFromCPU(cpu, map); break; } } else { /* Other host CPU information */ - if (!(guest_model = ppc64ModelFromCPU(cpu, map))) - goto cleanup; + guest_model = ppc64ModelFromCPU(cpu, map); } + if (!guest_model) + goto cleanup; + if (STRNEQ(guest_model->name, host_model->name)) { VIR_DEBUG("host CPU model does not match required CPU model %s", guest_model->name); -- 2.8.3

There's this problem on the recent gcc-6.1: In file included from conf/domain_conf.c:37:0: conf/domain_conf.c: In function 'virDomainChrPreAlloc': conf/domain_conf.c:14109:35: error: potential null pointer dereference [-Werror=null-dereference] return VIR_REALLOC_N(*arrPtr, *cntPtr + 1); ^~ ./util/viralloc.h:158:73: note: in definition of macro 'VIR_REALLOC_N' # define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count), \ ^~~~~ conf/domain_conf.c: In function 'virDomainChrRemove': conf/domain_conf.c:14133:21: error: potential null pointer dereference [-Werror=null-dereference] for (i = 0; i < *cntPtr; i++) { ^~~~~~~ GCC basically fails to see, that the virDomainChrGetDomainPtrsInternal will never actually return NULL because it's never called over a domain char device with _LAST type. But to make it shut up, lets turn this function into returning an integer and check in the callers if a zero value value was returned. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 568c699..2efe0a3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14038,7 +14038,7 @@ virDomainChrFind(virDomainDefPtr def, /* Return the address within vmdef to be modified when working with a * chrdefptr of the given type. */ -static void +static int ATTRIBUTE_RETURN_CHECK virDomainChrGetDomainPtrsInternal(virDomainDefPtr vmdef, virDomainChrDeviceType type, virDomainChrDefPtr ***arrPtr, @@ -14070,6 +14070,8 @@ virDomainChrGetDomainPtrsInternal(virDomainDefPtr vmdef, *cntPtr = NULL; break; } + + return (*arrPtr && *cntPtr) ? 0 : -1; } @@ -14085,14 +14087,13 @@ virDomainChrGetDomainPtrs(const virDomainDef *vmdef, size_t *cntVar = NULL; /* Cast away const; we add it back in the final assignment. */ - virDomainChrGetDomainPtrsInternal((virDomainDefPtr) vmdef, type, - &arrVar, &cntVar); - if (arrVar) { + if (virDomainChrGetDomainPtrsInternal((virDomainDefPtr) vmdef, type, + &arrVar, &cntVar) < 0) { + *arrPtr = NULL; + *cntPtr = 0; + } else { *arrPtr = (const virDomainChrDef **) *arrVar; *cntPtr = *cntVar; - } else { - *arrPtr = NULL; - *cntPtr = 0; } } @@ -14104,7 +14105,9 @@ virDomainChrPreAlloc(virDomainDefPtr vmdef, virDomainChrDefPtr **arrPtr = NULL; size_t *cntPtr = NULL; - virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); + if (virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, + &arrPtr, &cntPtr) < 0) + return -1; return VIR_REALLOC_N(*arrPtr, *cntPtr + 1); } @@ -14116,7 +14119,9 @@ virDomainChrInsertPreAlloced(virDomainDefPtr vmdef, virDomainChrDefPtr **arrPtr = NULL; size_t *cntPtr = NULL; - virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); + if (virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, + &arrPtr, &cntPtr) < 0) + return; VIR_APPEND_ELEMENT_INPLACE(*arrPtr, *cntPtr, chr); } @@ -14128,7 +14133,9 @@ virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr ret = NULL, **arrPtr = NULL; size_t i, *cntPtr = NULL; - virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); + if (virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, + &arrPtr, &cntPtr) < 0) + return NULL; for (i = 0; i < *cntPtr; i++) { ret = (*arrPtr)[i]; -- 2.8.3

On 02.06.2016 13:36, Peter Krempa wrote: Well, I'm sort of puzzled here. The reason why gcc thinks we might deref NULL is that at the input of this function a chardev with a type other than enumerated in the switch occurred. In which case we don't set any of the passed arguments and thus effectively defer NULL later in the code. This is obviously incorrect as our parser guarantees chardev type within range as expected here. Anyway, if I went by your approach, compiler would think that for CHR_DEVICE_TYPE_YET_UNKNOWN we leave the pointers untouched and return 0 and deref NULL. The same compiler which checks all enum members are enumerated in the switch and therefore there can't be any CHR_DEVICE_TYPE_YET_UNKNOWN. Le sigh. This is actually the reason I want to avoid having 'default' label in the switch. If we ever add new device type it's nice to have compiler identify all^Wa lot of^W^W^Wsome places that need fixing. Well, in theory yes, but in reality I don't see a way how the error could happen in the first place.

Okay, I admit that our code here is complex. It's not easy to spot that NULL deref can't really happen here. So it's no wonder that a dumb compiler fails to see all the connections and produces the following errors: CC conf/libvirt_conf_la-domain_conf.lo conf/domain_conf.c: In function 'virDomainDefFormatInternal': conf/domain_conf.c:22162:22: error: potential null pointer dereference [-Werror=null-dereference] if (sched->policy == i) ~~~~~^~~~~~~~ conf/domain_conf.c:22191:26: error: potential null pointer dereference [-Werror=null-dereference] priority = sched->priority; ~~~~~~~~~^~~~~~~~~~~~~~~~~ conf/domain_conf.c:22197:30: error: potential null pointer dereference [-Werror=null-dereference] if (sched->priority == priority) ~~~~~^~~~~~~~~~ conf/domain_conf.c:22162:22: error: potential null pointer dereference [-Werror=null-dereference] if (sched->policy == i) ~~~~~^~~~~~~~ conf/domain_conf.c:22191:26: error: potential null pointer dereference [-Werror=null-dereference] priority = sched->priority; ~~~~~~~~~^~~~~~~~~~~~~~~~~ conf/domain_conf.c:22197:30: error: potential null pointer dereference [-Werror=null-dereference] if (sched->priority == priority) ~~~~~^~~~~~~~~~ cc1: all warnings being treated as errors Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2efe0a3..e5a0e21 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22147,6 +22147,14 @@ virDomainFormatSchedDef(virDomainDefPtr def, size_t i; int ret = -1; + /* Okay, @func should never return NULL here because it does + * so iff corresponding resource does not exists. But if it + * doesn't we should not have been called in the first place. + * But some compilers fails to see this complex reasoning and + * deduct that this code is buggy. Shut them up by checking + * for return value of sched. Even though we don't need to. + */ + if (!(schedMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)) || !(prioMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) goto cleanup; @@ -22159,7 +22167,7 @@ virDomainFormatSchedDef(virDomainDefPtr def, while ((next = virBitmapNextSetBit(resourceMap, next)) > -1) { sched = func(def, next); - if (sched->policy == i) + if (sched && sched->policy == i) ignore_value(virBitmapSetBit(schedMap, next)); } @@ -22187,14 +22195,15 @@ virDomainFormatSchedDef(virDomainDefPtr def, /* we need to find a subset of vCPUs with the given scheduler * that share the priority */ nextprio = virBitmapNextSetBit(schedMap, -1); - sched = func(def, nextprio); + if (!(sched = func(def, nextprio))) + goto cleanup; + priority = sched->priority; - ignore_value(virBitmapSetBit(prioMap, nextprio)); while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { sched = func(def, nextprio); - if (sched->priority == priority) + if (sched && sched->priority == priority) ignore_value(virBitmapSetBit(prioMap, nextprio)); } -- 2.8.3