[libvirt] [PATCH v2 0/4] Final round of build fixes
diff to v1: - Some patches from the original patch set are pushed now - For the other ones, I've managed to work in review suggestions Michal Privoznik (4): virNetDevBridgeGet: Don't require users to virNetDevSetupControl ppc64Compute: Avoid possible NULL dereference virDomainChrGetDomainPtrsInternal: Return an integer virDomainFormatSchedDef: Avoid false positive NULL dereference src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++-------------- src/cpu/cpu_ppc64.c | 12 ++++++------ src/util/virnetdevbridge.c | 35 ++++++++++++----------------------- 3 files changed, 48 insertions(+), 43 deletions(-) -- 2.8.3
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@redhat.com> --- 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
On Thu, Jun 02, 2016 at 12:42:50 +0200, Michal Privoznik wrote:
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@redhat.com> --- src/util/virnetdevbridge.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-)
ACK, this version is much much cleaner. Thanks for fixing this properly.
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@redhat.com> --- 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
On Thu, Jun 02, 2016 at 12:42:51 +0200, Michal Privoznik wrote:
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@redhat.com> --- src/cpu/cpu_ppc64.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
ACK
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@redhat.com> --- 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 Thu, Jun 02, 2016 at 12:42:52 +0200, Michal Privoznik wrote:
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@redhat.com> --- 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;
This doesn't set any error. The VIR_DOMAIN_CHR_DEVICE_TYPE_LAST case should do so and possibly return -1 right away to avoid the ternary.
}
[...]
@@ -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;
So this will report the "unknown error".
return VIR_REALLOC_N(*arrPtr, *cntPtr + 1);
ACK with the error added.
On 02.06.2016 13:36, Peter Krempa wrote:
On Thu, Jun 02, 2016 at 12:42:52 +0200, Michal Privoznik wrote:
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@redhat.com> --- 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;
This doesn't set any error. The VIR_DOMAIN_CHR_DEVICE_TYPE_LAST case should do so and possibly return -1 right away to avoid the ternary.
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.
}
[...]
@@ -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;
So this will report the "unknown error".
Well, in theory yes, but in reality I don't see a way how the error could happen in the first place.
return VIR_REALLOC_N(*arrPtr, *cntPtr + 1);
ACK with the error added.
On Thu, Jun 02, 2016 at 13:58:52 +0200, Michal Privoznik wrote:
On 02.06.2016 13:36, Peter Krempa wrote:
On Thu, Jun 02, 2016 at 12:42:52 +0200, Michal Privoznik wrote:
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@redhat.com> --- 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;
This doesn't set any error. The VIR_DOMAIN_CHR_DEVICE_TYPE_LAST case should do so and possibly return -1 right away to avoid the ternary.
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.
I think you could do something along: *arrPtr = NULL; *cntPtr = NULL; switch (type) { case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: *arrPtr = &vmdef->parallels; *cntPtr = &vmdef->nparallels; return 0; case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: *arrPtr = &vmdef->serials; *cntPtr = &vmdef->nserials; return 0; case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: *arrPtr = &vmdef->consoles; *cntPtr = &vmdef->nconsoles; return 0; case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: *arrPtr = &vmdef->channels; *cntPtr = &vmdef->nchannels; return 0; case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: break; } virReportError(VIR_..., "it's broken"); return -1; ... to basically create a 'default' case outside of the switch. Peter
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@redhat.com> --- 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
On Thu, Jun 02, 2016 at 12:42:53 +0200, Michal Privoznik wrote:
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) ~~~~~^~~~~~~~~~
I think you can trime a few of those.
cc1: all warnings being treated as errors
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
ACK
participants (2)
-
Michal Privoznik -
Peter Krempa