[libvirt] [PATCH 00/10] Fix multiple OOM problems
From: "Daniel P. Berrange" <berrange@redhat.com> Running out long ignored OOM injection tests identified a number of problems. This series fixes those that I have found so far, but more are to follow. I'm also rewriting the OOM injection code to make it much, much, much faster to run. Daniel P. Berrange (10): Fix crash on OOM when parsing disk security label Fix crash on OOM in parsing CPU mask in domain XML Fix crash if OOM occurs when creating virConnectPtr Fix crash on OOM in qemuDomainCCWAddressSetCreate() Fix crash on OOM in qemuAddRBDHost Fix allocation of arglist in qemuStringToArgvEnv Fix error checking of qemuParseKeywords return status Fix missing OOM check in qemuParseCommandLine when splitting strings Fix reporting of errors in OOM injection code Don't ignore allocation failure in virCommandAddEnvPassCommon src/conf/domain_conf.c | 5 ++-- src/datatypes.c | 10 +++++--- src/qemu/qemu_command.c | 59 ++++++++++++++++++++++++-------------------- src/qemu/qemu_command.h | 1 + src/qemu/qemu_monitor_json.c | 4 +-- src/util/viralloc.c | 18 ++++++++++++-- src/util/vircommand.c | 5 +++- 7 files changed, 63 insertions(+), 39 deletions(-) -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> If an OOM error occurs in virSecurityDeviceLabelDefParseXML the cleanup code may free an uninitialized pointer, causing a crash Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8953579..73ae0b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4507,7 +4507,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, int nvmSeclabels, xmlXPathContextPtr ctxt, unsigned int flags) { - virSecurityDeviceLabelDefPtr *seclabels; + virSecurityDeviceLabelDefPtr *seclabels = NULL; size_t nseclabels = 0; int n; size_t i, j; -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If an OOM error occurs in virSecurityDeviceLabelDefParseXML the cleanup code may free an uninitialized pointer, causing a crash
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK. Peter
From: "Daniel P. Berrange" <berrange@redhat.com> The virDomainDefParseXML method did not check the return value of the virBitmapNew API call for NULL. This lead to a crash on OOM Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 73ae0b0..240f318 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11188,7 +11188,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_ALLOC(vcpupin) < 0) goto error; - vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); + if (!(vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) + goto error; virBitmapCopy(vcpupin->cpumask, def->cpumask); vcpupin->vcpuid = i; def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The virDomainDefParseXML method did not check the return value of the virBitmapNew API call for NULL. This lead to a crash on OOM
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK. Peter
From: "Daniel P. Berrange" <berrange@redhat.com> If a OOM error occurs in virGetConnect, this may cause the virConnectDispose method to de-reference a NULL pointer, since the close callback will not have been initialized. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/datatypes.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 940d968..161f1b0 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -164,11 +164,13 @@ virConnectDispose(void *obj) virURIFree(conn->uri); - virObjectLock(conn->closeCallback); - conn->closeCallback->callback = NULL; - virObjectUnlock(conn->closeCallback); + if (conn->closeCallback) { + virObjectLock(conn->closeCallback); + conn->closeCallback->callback = NULL; + virObjectUnlock(conn->closeCallback); - virObjectUnref(conn->closeCallback); + virObjectUnref(conn->closeCallback); + } virMutexUnlock(&conn->lock); virMutexDestroy(&conn->lock); -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If a OOM error occurs in virGetConnect, this may cause the virConnectDispose method to de-reference a NULL pointer, since the close callback will not have been initialized.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/datatypes.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
ACK. Peter
From: "Daniel P. Berrange" <berrange@redhat.com> If OOM occurs in qemuDomainCCWAddressSetCreate, it jumps to a cleanup block and frees the partially initialized object. It then mistakenly returns the address of the just free'd pointer instead of NULL. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e6239c9..b20149b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1275,7 +1275,7 @@ qemuDomainCCWAddressSetCreate(void) cleanup: qemuDomainCCWAddressSetFree(addrs); - return addrs; + return NULL; } /* -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If OOM occurs in qemuDomainCCWAddressSetCreate, it jumps to a cleanup block and frees the partially initialized object. It then mistakenly returns the address of the just free'd pointer instead of NULL.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e6239c9..b20149b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1275,7 +1275,7 @@ qemuDomainCCWAddressSetCreate(void)
cleanup:
The label should be then called "error"
qemuDomainCCWAddressSetFree(addrs); - return addrs; + return NULL; }
/*
ACK nevertheless.
From: "Daniel P. Berrange" <berrange@redhat.com> When parsing the RBD hosts, it increments the 'nhosts' counter before increasing the 'hosts' array allocation. If an OOM then occurs when increasing the array allocation, the cleanup block will attempt to access beyond the end of the array. Switch to using VIR_EXPAND_N instead of VIR_REALLOC_N to protect against this mistake Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b20149b..f6e4ace 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3232,9 +3232,8 @@ static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport) size_t skip; char **parts; - disk->nhosts++; - if (VIR_REALLOC_N(disk->hosts, disk->nhosts) < 0) - goto error; + if (VIR_EXPAND_N(disk->hosts, disk->nhosts, 1) < 0) + return -1; if ((port = strchr(hostport, ']'))) { /* ipv6, strip brackets */ -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
When parsing the RBD hosts, it increments the 'nhosts' counter before increasing the 'hosts' array allocation. If an OOM then occurs when increasing the array allocation, the cleanup block will attempt to access beyond the end of the array. Switch to using VIR_EXPAND_N instead of VIR_REALLOC_N to protect against this mistake
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
ACK. Peter
From: "Daniel P. Berrange" <berrange@redhat.com> In commit 41b550567918790cb304378f39c3ba369bcca28e Author: Eric Blake <eblake@redhat.com> Date: Wed Aug 28 15:01:23 2013 -0600 qemu: simplify list cleanup The qemuStringToArgvEnv method was changed to use virStringFreeList to free the 'arglist' array. This method assumes the string list array is NULL terminated, however, qemuStringToArgvEnv was not ensuring this when populating 'arglist'. This caused an out of bounds access by virStringFreeList when OOM occured in the initial loop of qemuStringToArgvEnv Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f6e4ace..73bbe91 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9656,9 +9656,9 @@ static int qemuStringToArgvEnv(const char *args, char ***retargv) { char **arglist = NULL; - int argcount = 0; - int argalloc = 0; - int envend; + size_t argcount = 0; + size_t argalloc = 0; + size_t envend; size_t i; const char *curr = args; const char *start; @@ -9695,15 +9695,13 @@ static int qemuStringToArgvEnv(const char *args, if (next && (*next == '\'' || *next == '"')) next++; - if (argalloc == argcount) { - if (VIR_REALLOC_N(arglist, argalloc+10) < 0) { - VIR_FREE(arg); - goto error; - } - argalloc+=10; + if (VIR_RESIZE_N(arglist, argalloc, argcount, 2) < 0) { + VIR_FREE(arg); + goto error; } arglist[argcount++] = arg; + arglist[argcount] = NULL; while (next && c_isspace(*next)) next++; -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
In
commit 41b550567918790cb304378f39c3ba369bcca28e Author: Eric Blake <eblake@redhat.com> Date: Wed Aug 28 15:01:23 2013 -0600
qemu: simplify list cleanup
The qemuStringToArgvEnv method was changed to use virStringFreeList to free the 'arglist' array. This method assumes the string list array is NULL terminated, however, qemuStringToArgvEnv was not ensuring this when populating 'arglist'. This caused an out of bounds access by virStringFreeList when OOM occured in the initial loop of qemuStringToArgvEnv
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
ACK, Peter
From: "Daniel P. Berrange" <berrange@redhat.com> Most callers of qemuParseKeywords were assigning its return value to a 'size_t' variable. Then then also checked '< 0' for error condition, but this will never be true with the unsigned size_t variable. Rather than using 'ssize_t', change qemuParseKeywords so that the element count is returned via an output parameter, leaving the return value solely as an error indicator. This avoids a crash accessing beyond the end of an error upon OOM. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 33 ++++++++++++++++++++------------- src/qemu/qemu_command.h | 1 + src/qemu/qemu_monitor_json.c | 4 +--- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 73bbe91..76d4e7c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9776,6 +9776,7 @@ int qemuParseKeywords(const char *str, char ***retkeywords, char ***retvalues, + int *retnkeywords, int allowEmptyValue) { int keywordCount = 0; @@ -9788,6 +9789,7 @@ qemuParseKeywords(const char *str, *retkeywords = NULL; *retvalues = NULL; + *retnkeywords = 0; end = start + strlen(str); while (start) { @@ -9857,8 +9859,8 @@ qemuParseKeywords(const char *str, *retkeywords = keywords; *retvalues = values; - - return keywordCount; + *retnkeywords = keywordCount; + return 0; error: for (i = 0; i < keywordCount; i++) { @@ -9893,9 +9895,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, int unitid = -1; int trans = VIR_DOMAIN_DISK_TRANS_DEFAULT; - if ((nkeywords = qemuParseKeywords(val, - &keywords, - &values, 0)) < 0) + if (qemuParseKeywords(val, + &keywords, + &values, + &nkeywords, + 0) < 0) return NULL; if (VIR_ALLOC(def) < 0) @@ -10244,9 +10248,11 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, tmp = strchr(val, ','); if (tmp) { - if ((nkeywords = qemuParseKeywords(tmp+1, - &keywords, - &values, 0)) < 0) + if (qemuParseKeywords(tmp+1, + &keywords, + &values, + &nkeywords, + 0) < 0) return NULL; } else { nkeywords = 0; @@ -10314,9 +10320,11 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, VIR_FREE(values); if (STRPREFIX(nic, "nic,")) { - if ((nkeywords = qemuParseKeywords(nic + strlen("nic,"), - &keywords, - &values, 0)) < 0) { + if (qemuParseKeywords(nic + strlen("nic,"), + &keywords, + &values, + &nkeywords, + 0) < 0) { virDomainNetDefFree(def); def = NULL; goto cleanup; @@ -10820,8 +10828,7 @@ qemuParseCommandLineSmp(virDomainDefPtr dom, char *end; int ret; - nkws = qemuParseKeywords(val, &kws, &vals, 1); - if (nkws < 0) + if (qemuParseKeywords(val, &kws, &vals, &nkws, 1) < 0) return -1; for (i = 0; i < nkws; i++) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2f248eb..0e16a3d 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -296,6 +296,7 @@ int qemuParseKeywords(const char *str, char ***retkeywords, char ***retvalues, + int *retnkeywords, int allowEmptyValue); #endif /* __QEMU_COMMAND_H__*/ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 12f7e69..2d84161 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -533,9 +533,7 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword) if (!(ret = virJSONValueNewObject())) goto error; - nkeywords = qemuParseKeywords(str, &keywords, &values, 1); - - if (nkeywords < 0) + if (qemuParseKeywords(str, &keywords, &values, &nkeywords, 1) < 0) goto error; for (i = 0; i < nkeywords; i++) { -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Most callers of qemuParseKeywords were assigning its return value to a 'size_t' variable. Then then also checked '< 0' for error condition, but this will never be true with the unsigned size_t variable. Rather than using 'ssize_t', change qemuParseKeywords so that the element count is returned via an output parameter, leaving the return value solely as an error indicator.
This avoids a crash accessing beyond the end of an error upon OOM.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 33 ++++++++++++++++++++------------- src/qemu/qemu_command.h | 1 + src/qemu/qemu_monitor_json.c | 4 +--- 3 files changed, 22 insertions(+), 16 deletions(-)
ACK. Peter
From: "Daniel P. Berrange" <berrange@redhat.com> The qemuParseCommandLine method did not check the return value of virStringSplit to see if OOM had occurred. This lead to dereference of a NULL pointer on OOM. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 76d4e7c..733b653 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11387,7 +11387,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, * Set os.machine only if first parameter lacks '=' or * contains explicit type='...' */ WANT_VALUE(); - list = virStringSplit(val, ",", 0); + if (!(list = virStringSplit(val, ",", 0))) + goto error; param = list[0]; if (STRPREFIX(param, "type=")) -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The qemuParseCommandLine method did not check the return value of virStringSplit to see if OOM had occurred. This lead to dereference of a NULL pointer on OOM.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK. Peter
From: "Daniel P. Berrange" <berrange@redhat.com> When the various viralloc.c functions were changed to use the normal error reporting code, the OOM injection code paths were not updated to report errors. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/viralloc.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 8008f33..27ed74a 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -132,6 +132,9 @@ int virAlloc(void *ptrptr, #if TEST_OOM if (virAllocTestFail()) { *(void **)ptrptr = NULL; + if (report) + virReportOOMErrorFull(domcode, filename, funcname, linenr); + errno = ENOMEM; return -1; } #endif @@ -176,6 +179,9 @@ int virAllocN(void *ptrptr, #if TEST_OOM if (virAllocTestFail()) { *(void **)ptrptr = NULL; + if (report) + virReportOOMErrorFull(domcode, filename, funcname, linenr); + errno = ENOMEM; return -1; } #endif @@ -220,8 +226,12 @@ int virReallocN(void *ptrptr, { void *tmp; #if TEST_OOM - if (virAllocTestFail()) + if (virAllocTestFail()) { + if (report) + virReportOOMErrorFull(domcode, filename, funcname, linenr); + errno = ENOMEM; return -1; + } #endif if (xalloc_oversized(count, size)) { @@ -529,8 +539,12 @@ int virAllocVar(void *ptrptr, size_t alloc_size = 0; #if TEST_OOM - if (virAllocTestFail()) + if (virAllocTestFail()) { + if (report) + virReportOOMErrorFull(domcode, filename, funcname, linenr); + errno = ENOMEM; return -1; + } #endif if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
When the various viralloc.c functions were changed to use the normal error reporting code, the OOM injection code paths were not updated to report errors.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/viralloc.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
ACK Peter
From: "Daniel P. Berrange" <berrange@redhat.com> The virCommandAddEnvPassCommon method ignored the failure to pre-allocate the env variable array with VIR_RESIZE_N. While this is harmless, it confuses the test harness which is trying to validate OOM handling of every individual allocation call. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/vircommand.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 95331a6..933028e 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1281,7 +1281,10 @@ virCommandAddEnvPassCommon(virCommandPtr cmd) /* Attempt to Pre-allocate; allocation failure will be detected * later during virCommandAdd*. */ - ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9)); + if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9) < 0) { + cmd->has_error = ENOMEM; + return; + } virCommandAddEnvPair(cmd, "LC_ALL", "C"); -- 1.8.3.1
On 09/23/13 15:23, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The virCommandAddEnvPassCommon method ignored the failure to pre-allocate the env variable array with VIR_RESIZE_N. While this is harmless, it confuses the test harness which is trying to validate OOM handling of every individual allocation call.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/vircommand.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 95331a6..933028e 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1281,7 +1281,10 @@ virCommandAddEnvPassCommon(virCommandPtr cmd)
/* Attempt to Pre-allocate; allocation failure will be detected * later during virCommandAdd*. */
Isn't this comment now obsolete?
- ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9)); + if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9) < 0) { + cmd->has_error = ENOMEM; + return; + }
virCommandAddEnvPair(cmd, "LC_ALL", "C");
ACK. Peter
participants (2)
-
Daniel P. Berrange -
Peter Krempa