4:34 p.m.
Updated to a more recent Coverity checker and it found some new
issues. The last one perhaps could be a false positive, I don't
really know for sure, but there's enough instances that I figured
it was worth the patch.
John Ferlan (3):
conf: Alter when ctxt->node is set
lxc: Resolve memory leak
tests: Resolve possible overrun
src/conf/domain_conf.c | 8 +--
src/lxc/lxc_native.c | 110 +++++++++++++++++++++++------------------
tests/vircgroupmock.c | 21 ++++----
3 files changed, 79 insertions(+), 60 deletions(-)
--
2.17.1
4:34 p.m.
New subject: [libvirt] [PATCH 1/3] conf: Alter when ctxt->node is set
In virDomainMemoryDefParseXML and virDomainVideoDefParseXML if
the VIR_ALLOC fails and NULL is returned, then the alteration
to ctxt->node isn't reversed.
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1ee43950ae..9911d56130 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15089,11 +15089,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
char *vgamem = NULL;
char *primary = NULL;
- ctxt->node = node;
-
if (!(def = virDomainVideoDefNew()))
return NULL;
+ ctxt->node = node;
+
cur = node->children;
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE) {
@@ -15830,11 +15830,11 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
virDomainMemoryDefPtr def;
int val;
- ctxt->node = memdevNode;
-
if (VIR_ALLOC(def) < 0)
return NULL;
+ ctxt->node = memdevNode;
+
if (!(tmp = virXMLPropString(memdevNode, "model"))) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing memory model"));
--
2.17.1
4:34 p.m.
New subject: [libvirt] [PATCH 2/3] lxc: Resolve memory leak
Commit 40b5c99a modified the virConfGetValue callers to use
virConfGetValueString. However, using the virConfGetValueString
resulted in leaking the returned @value string in each case.
So, let's modify each instance to use the VIR_AUTOFREE(char *)
syntax. In some instances changing the variable name since
@value was used more than once.
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/lxc/lxc_native.c | 110 +++++++++++++++++++++++++------------------
1 file changed, 63 insertions(+), 47 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index e1992fd1f9..6c5640536b 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -198,7 +198,7 @@ lxcSetRootfs(virDomainDefPtr def,
virConfPtr properties)
{
int type = VIR_DOMAIN_FS_TYPE_MOUNT;
- char *value = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
if (virConfGetValueString(properties, "lxc.rootfs", &value) <= 0)
return -1;
@@ -679,7 +679,7 @@ lxcConvertNetworkSettings(virDomainDefPtr def, virConfPtr properties)
static int
lxcCreateConsoles(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
int nbttys = 0;
virDomainChrDefPtr console;
size_t i;
@@ -756,14 +756,20 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr value, void
*data)
static int
lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) hard_limit = NULL;
+ VIR_AUTOFREE(char *) soft_limit = NULL;
+ VIR_AUTOFREE(char *) swap_hard_limit = NULL;
unsigned long long size = 0;
if (virConfGetValueString(properties,
"lxc.cgroup.memory.limit_in_bytes",
- &value) > 0) {
- if (lxcConvertSize(value, &size) < 0)
- goto error;
+ &hard_limit) > 0) {
+ if (lxcConvertSize(hard_limit, &size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ hard_limit);
+ return -1;
+ }
size = size / 1024;
virDomainDefSetMemoryTotal(def, size);
def->mem.hard_limit = virMemoryLimitTruncate(size);
@@ -771,76 +777,85 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
if (virConfGetValueString(properties,
"lxc.cgroup.memory.soft_limit_in_bytes",
- &value) > 0) {
- if (lxcConvertSize(value, &size) < 0)
- goto error;
+ &soft_limit) > 0) {
+ if (lxcConvertSize(soft_limit, &size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ soft_limit);
+ return -1;
+ }
def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
}
if (virConfGetValueString(properties,
"lxc.cgroup.memory.memsw.limit_in_bytes",
- &value) > 0) {
- if (lxcConvertSize(value, &size) < 0)
- goto error;
+ &swap_hard_limit) > 0) {
+ if (lxcConvertSize(swap_hard_limit, &size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ swap_hard_limit);
+ return -1;
+ }
def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
}
return 0;
-
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), value);
- return -1;
-
}
static int
lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) shares = NULL;
+ VIR_AUTOFREE(char *) quota = NULL;
+ VIR_AUTOFREE(char *) period = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
- &value) > 0) {
- if (virStrToLong_ull(value, NULL, 10, &def->cputune.shares) < 0)
- goto error;
+ &shares) > 0) {
+ if (virStrToLong_ull(shares, NULL, 10, &def->cputune.shares) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
shares);
+ return -1;
+ }
def->cputune.sharesSpecified = true;
}
if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_quota_us",
- &value) > 0) {
- if (virStrToLong_ll(value, NULL, 10, &def->cputune.quota) < 0)
- goto error;
+ "a) > 0) {
+ if (virStrToLong_ll(quota, NULL, 10, &def->cputune.quota) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"), quota);
+ return -1;
+ }
}
if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_period_us",
- &value) > 0) {
- if (virStrToLong_ull(value, NULL, 10, &def->cputune.period) < 0)
- goto error;
+ &period) > 0) {
+ if (virStrToLong_ull(period, NULL, 10, &def->cputune.period) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
period);
+ return -1;
+ }
}
return 0;
-
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), value);
- return -1;
}
static int
lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) cpus = NULL;
+ VIR_AUTOFREE(char *) mems = NULL;
virBitmapPtr nodeset = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
- &value) > 0) {
- if (virBitmapParse(value, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
+ &cpus) > 0) {
+ if (virBitmapParse(cpus, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
return -1;
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC;
}
if (virConfGetValueString(properties, "lxc.cgroup.cpuset.mems",
- &value) > 0) {
- if (virBitmapParse(value, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
+ &mems) > 0) {
+ if (virBitmapParse(mems, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
return -1;
if (virDomainNumatuneSet(def->numa,
def->placement_mode ==
@@ -949,7 +964,7 @@ lxcBlkioDeviceWalkCallback(const char *name, virConfValuePtr value,
void *data)
static int
lxcSetBlkioTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.blkio.weight",
&value) > 0) {
@@ -969,7 +984,7 @@ lxcSetBlkioTune(virDomainDefPtr def, virConfPtr properties)
static void
lxcSetCapDrop(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
char **toDrop = NULL;
const char *capString;
size_t i;
@@ -996,7 +1011,8 @@ lxcParseConfigString(const char *config,
{
virDomainDefPtr vmdef = NULL;
virConfPtr properties = NULL;
- char *value = NULL;
+ VIR_AUTOFREE(char *) lxcarch = NULL;
+ VIR_AUTOFREE(char *) lxcutsname = NULL;
if (!(properties = virConfReadString(config, VIR_CONF_FLAG_LXC_FORMAT)))
return NULL;
@@ -1029,11 +1045,11 @@ lxcParseConfigString(const char *config,
vmdef->nfss = 0;
vmdef->os.type = VIR_DOMAIN_OSTYPE_EXE;
- if (virConfGetValueString(properties, "lxc.arch", &value) > 0) {
- virArch arch = virArchFromString(value);
- if (arch == VIR_ARCH_NONE && STREQ(value, "x86"))
+ if (virConfGetValueString(properties, "lxc.arch", &lxcarch) > 0) {
+ virArch arch = virArchFromString(lxcarch);
+ if (arch == VIR_ARCH_NONE && STREQ(lxcarch, "x86"))
arch = VIR_ARCH_I686;
- else if (arch == VIR_ARCH_NONE && STREQ(value, "amd64"))
+ else if (arch == VIR_ARCH_NONE && STREQ(lxcarch, "amd64"))
arch = VIR_ARCH_X86_64;
vmdef->os.arch = arch;
}
@@ -1041,8 +1057,8 @@ lxcParseConfigString(const char *config,
if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
goto error;
- if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0
||
- VIR_STRDUP(vmdef->name, value) < 0)
+ if (virConfGetValueString(properties, "lxc.utsname", &lxcutsname) <=
0 ||
+ VIR_STRDUP(vmdef->name, lxcutsname) < 0)
goto error;
if (!vmdef->name && (VIR_STRDUP(vmdef->name, "unnamed") <
0))
goto error;
--
2.17.1
3:43 a.m.
New subject: [libvirt] [PATCH 2/3] lxc: Resolve memory leak
On Thu, Sep 20, 2018 at 05:34:37PM -0400, John Ferlan wrote:
Commit 40b5c99a modified the virConfGetValue callers to use
virConfGetValueString. However, using the virConfGetValueString
resulted in leaking the returned @value string in each case.
So, let's modify each instance to use the VIR_AUTOFREE(char *)
syntax. In some instances changing the variable name since
@value was used more than once.
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
...
static int
lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) hard_limit = NULL;
+ VIR_AUTOFREE(char *) soft_limit = NULL;
+ VIR_AUTOFREE(char *) swap_hard_limit = NULL;
unsigned long long size = 0;
This is just a matter of taste, but I'd probably went with VIR_AUTOFREE(char *)
value within each of the 'if' blocks.
if (virConfGetValueString(properties,
"lxc.cgroup.memory.limit_in_bytes",
- &value) > 0) {
- if (lxcConvertSize(value, &size) < 0)
- goto error;
+ &hard_limit) > 0) {
+ if (lxcConvertSize(hard_limit, &size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ hard_limit);
lxcConvertSize already reports an error: "failed to convert size: '%s'".
And
since the one you added provides essentially the same amount of information, we
might as well go with that one. Potentially, we could also change it so that it
matches the wording of all the other ones for consistency reasons, i.e. the one
you're proposing here.
Moreover, I don't really like copying the same error message, having a goto
error label is IMHO justified in case like these, although, we can drop it
specifically for this function, it's different for the functions below though...
+ return -1;
+ }
size = size / 1024;
virDomainDefSetMemoryTotal(def, size);
def->mem.hard_limit = virMemoryLimitTruncate(size);
@@ -771,76 +777,85 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
if (virConfGetValueString(properties,
"lxc.cgroup.memory.soft_limit_in_bytes",
- &value) > 0) {
- if (lxcConvertSize(value, &size) < 0)
- goto error;
+ &soft_limit) > 0) {
+ if (lxcConvertSize(soft_limit, &size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ soft_limit);
+ return -1;
+ }
def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
}
if (virConfGetValueString(properties,
"lxc.cgroup.memory.memsw.limit_in_bytes",
- &value) > 0) {
- if (lxcConvertSize(value, &size) < 0)
- goto error;
+ &swap_hard_limit) > 0) {
+ if (lxcConvertSize(swap_hard_limit, &size) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ swap_hard_limit);
+ return -1;
+ }
def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
}
return 0;
-
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), value);
- return -1;
we can definitely drop it ^here...Alternatively, we could drop the message in
lxcConvertSize and leave the error label here as well, again, for consistency
reasons, it's perhaps also the better way to let it behave like just like the
virStrToX helpers.
-
}
static int
lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) shares = NULL;
+ VIR_AUTOFREE(char *) quota = NULL;
+ VIR_AUTOFREE(char *) period = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
- &value) > 0) {
again, matter of taste, but I think that having a helper VIR_AUTOFREE(char *)
value that gets destroyed after each block looks more clean than having three
different function local helper variables just to match the structure members
we're filling in.
- if (virStrToLong_ull(value, NULL, 10,
&def->cputune.shares) < 0)
- goto error;
+ &shares) > 0) {
+ if (virStrToLong_ull(shares, NULL, 10, &def->cputune.shares) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
shares);
+ return -1;
+ }
def->cputune.sharesSpecified = true;
}
if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_quota_us",
- &value) > 0) {
- if (virStrToLong_ll(value, NULL, 10, &def->cputune.quota) < 0)
- goto error;
+ "a) > 0) {
+ if (virStrToLong_ll(quota, NULL, 10, &def->cputune.quota) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
quota);
+ return -1;
+ }
}
if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_period_us",
- &value) > 0) {
- if (virStrToLong_ull(value, NULL, 10, &def->cputune.period) < 0)
- goto error;
+ &period) > 0) {
+ if (virStrToLong_ull(period, NULL, 10, &def->cputune.period) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
period);
+ return -1;
+ }
}
return 0;
-
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), value);
- return -1;
I actually prefer the error label here, because we don't have to duplicate the
same error message at multiple spots.
}
static int
lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) cpus = NULL;
+ VIR_AUTOFREE(char *) mems = NULL;
virBitmapPtr nodeset = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
- &value) > 0) {
same comments apply here too...additionally (just a note out of sheer rant), we
should do something about the error within virBitmapParse, since that one isn't
going to help literally anyone, so there's no way for the user to know why
their XML failed to parse.
- if (virBitmapParse(value, &def->cpumask,
VIR_DOMAIN_CPUMASK_LEN) < 0)
+ &cpus) > 0) {
+ if (virBitmapParse(cpus, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
return -1;
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC;
}
if (virConfGetValueString(properties, "lxc.cgroup.cpuset.mems",
- &value) > 0) {
- if (virBitmapParse(value, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
+ &mems) > 0) {
+ if (virBitmapParse(mems, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
return -1;
if (virDomainNumatuneSet(def->numa,
def->placement_mode ==
@@ -949,7 +964,7 @@ lxcBlkioDeviceWalkCallback(const char *name, virConfValuePtr value,
void *data)
static int
lxcSetBlkioTune(virDomainDefPtr def, virConfPtr properties)
{
- char *value = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.blkio.weight",
&value) > 0) {
the comments further onward would be simply the same, you get the picture, but
those are just tiny nitpicks based on personal preference I'd say (except for
copying the same error messages all over again), with that addressed:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
6:46 a.m.
New subject: [libvirt] [PATCH 2/3] lxc: Resolve memory leak
On 09/21/2018 04:43 AM, Erik Skultety wrote:
On Thu, Sep 20, 2018 at 05:34:37PM -0400, John Ferlan wrote:
> Commit 40b5c99a modified the virConfGetValue callers to use
> virConfGetValueString. However, using the virConfGetValueString
> resulted in leaking the returned @value string in each case.
> So, let's modify each instance to use the VIR_AUTOFREE(char *)
> syntax. In some instances changing the variable name since
> @value was used more than once.
>
> Found by Coverity
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
...
> static int
> lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) hard_limit = NULL;
> + VIR_AUTOFREE(char *) soft_limit = NULL;
> + VIR_AUTOFREE(char *) swap_hard_limit = NULL;
> unsigned long long size = 0;
This is just a matter of taste, but I'd probably went with VIR_AUTOFREE(char *)
value within each of the 'if' blocks.
In this instance, how?
>
> if (virConfGetValueString(properties,
> "lxc.cgroup.memory.limit_in_bytes",
> - &value) > 0) {
^^^ hard_limit instead of value, 4 spaces in for the "if"... This
repeats for each one. They're all top level.
> - if (lxcConvertSize(value, &size) < 0)
> - goto error;
> + &hard_limit) > 0) {
> + if (lxcConvertSize(hard_limit, &size) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
> + hard_limit);
lxcConvertSize already reports an error: "failed to convert size:
'%s'". And
since the one you added provides essentially the same amount of information, we
might as well go with that one. Potentially, we could also change it so that it
matches the wording of all the other ones for consistency reasons, i.e. the one
you're proposing here.
Moreover, I don't really like copying the same error message, having a goto
error label is IMHO justified in case like these, although, we can drop it
specifically for this function, it's different for the functions below though...
Generally speaking - I agree about the common error label; however, in
this case without looking at the lxcConvertSize because the previous
code was:
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), value);
and @value was being replaced by 3 new variables each unique, using a
common label was not viable.
So there's really two patches here. The first one should remove those
error: labels because the lxcConvertSize already generates one. Then
the second patch would be to convert to VIR_AUTOFREE.
> + return -1;
> + }
> size = size / 1024;
> virDomainDefSetMemoryTotal(def, size);
> def->mem.hard_limit = virMemoryLimitTruncate(size);
> @@ -771,76 +777,85 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
>
> if (virConfGetValueString(properties,
> "lxc.cgroup.memory.soft_limit_in_bytes",
> - &value) > 0) {
> - if (lxcConvertSize(value, &size) < 0)
> - goto error;
> + &soft_limit) > 0) {
> + if (lxcConvertSize(soft_limit, &size) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
> + soft_limit);
> + return -1;
> + }
> def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
> }
>
> if (virConfGetValueString(properties,
> "lxc.cgroup.memory.memsw.limit_in_bytes",
> - &value) > 0) {
> - if (lxcConvertSize(value, &size) < 0)
> - goto error;
> + &swap_hard_limit) > 0) {
> + if (lxcConvertSize(swap_hard_limit, &size) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
> + swap_hard_limit);
> + return -1;
> + }
> def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
> }
> return 0;
> -
> - error:
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed to parse integer: '%s'"), value);
> - return -1;
we can definitely drop it ^here...Alternatively, we could drop the message in
lxcConvertSize and leave the error label here as well, again, for consistency
reasons, it's perhaps also the better way to let it behave like just like the
virStrToX helpers.
> -
> }
>
> static int
> lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) shares = NULL;
> + VIR_AUTOFREE(char *) quota = NULL;
> + VIR_AUTOFREE(char *) period = NULL;
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
> - &value) > 0) {
again, matter of taste, but I think that having a helper VIR_AUTOFREE(char *)
value that gets destroyed after each block looks more clean than having three
different function local helper variables just to match the structure members
we're filling in.
Oh, so you want a rewrite of the logic to call some local that will
return some value, but then we create a local for handling when @value
is a virStrToLong_i, virStrToLong_ui, virStrToLong_ull, or
virBitmapParse. That would mean another pre-patch.
Or maybe I'm being dense and just missing your point
> - if (virStrToLong_ull(value, NULL, 10,
&def->cputune.shares) < 0)
> - goto error;
> + &shares) > 0) {
> + if (virStrToLong_ull(shares, NULL, 10, &def->cputune.shares) < 0)
{
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
shares);
> + return -1;
> + }
> def->cputune.sharesSpecified = true;
> }
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_quota_us",
> - &value) > 0) {
> - if (virStrToLong_ll(value, NULL, 10, &def->cputune.quota) < 0)
> - goto error;
> + "a) > 0) {
> + if (virStrToLong_ll(quota, NULL, 10, &def->cputune.quota) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
quota);
> + return -1;
> + }
> }
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_period_us",
> - &value) > 0) {
> - if (virStrToLong_ull(value, NULL, 10, &def->cputune.period) < 0)
> - goto error;
> + &period) > 0) {
> + if (virStrToLong_ull(period, NULL, 10, &def->cputune.period) < 0)
{
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
period);
> + return -1;
> + }
> }
>
> return 0;
> -
> - error:
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed to parse integer: '%s'"), value);
> - return -1;
I actually prefer the error label here, because we don't have to duplicate the
same error message at multiple spots.
Again, maybe I'm missing something subtle in your comment.
> }
>
> static int
> lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) cpus = NULL;
> + VIR_AUTOFREE(char *) mems = NULL;
> virBitmapPtr nodeset = NULL;
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
> - &value) > 0) {
same comments apply here too...additionally (just a note out of sheer rant), we
should do something about the error within virBitmapParse, since that one isn't
going to help literally anyone, so there's no way for the user to know why
their XML failed to parse.
Oh so you want me to step into the cpuset="" argument ;-) No thanks!
> - if (virBitmapParse(value, &def->cpumask,
VIR_DOMAIN_CPUMASK_LEN) < 0)
> + &cpus) > 0) {
> + if (virBitmapParse(cpus, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) <
0)
> return -1;
> def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC;
> }
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpuset.mems",
> - &value) > 0) {
> - if (virBitmapParse(value, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
> + &mems) > 0) {
> + if (virBitmapParse(mems, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
> return -1;
> if (virDomainNumatuneSet(def->numa,
> def->placement_mode ==
> @@ -949,7 +964,7 @@ lxcBlkioDeviceWalkCallback(const char *name, virConfValuePtr
value, void *data)
> static int
> lxcSetBlkioTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) value = NULL;
>
> if (virConfGetValueString(properties, "lxc.cgroup.blkio.weight",
> &value) > 0) {
the comments further onward would be simply the same, you get the picture, but
those are just tiny nitpicks based on personal preference I'd say (except for
copying the same error messages all over again), with that addressed:
Nope, the picture wasn't clear, but I can/should check my coffee to see
if it's strong enough.
John
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
7 a.m.
New subject: [libvirt] [PATCH 2/3] lxc: Resolve memory leak
On Fri, Sep 21, 2018 at 07:46:47AM -0400, John Ferlan wrote:
On 09/21/2018 04:43 AM, Erik Skultety wrote:
> On Thu, Sep 20, 2018 at 05:34:37PM -0400, John Ferlan wrote:
>> Commit 40b5c99a modified the virConfGetValue callers to use
>> virConfGetValueString. However, using the virConfGetValueString
>> resulted in leaking the returned @value string in each case.
>> So, let's modify each instance to use the VIR_AUTOFREE(char *)
>> syntax. In some instances changing the variable name since
>> @value was used more than once.
>>
>> Found by Coverity
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
> ...
>
>
>> static int
>> lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
>> {
>> - char *value = NULL;
>> + VIR_AUTOFREE(char *) hard_limit = NULL;
>> + VIR_AUTOFREE(char *) soft_limit = NULL;
>> + VIR_AUTOFREE(char *) swap_hard_limit = NULL;
>> unsigned long long size = 0;
>
> This is just a matter of taste, but I'd probably went with VIR_AUTOFREE(char *)
> value within each of the 'if' blocks.
>
In this instance, how?
>>
>> if (virConfGetValueString(properties,
>> "lxc.cgroup.memory.limit_in_bytes",
>> - &value) > 0) {
^^^ hard_limit instead of value, 4 spaces in for the "if"... This
repeats for each one. They're all top level.
>> - if (lxcConvertSize(value, &size) < 0)
>> - goto error;
>> + &hard_limit) > 0) {
>> + if (lxcConvertSize(hard_limit, &size) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to parse integer:
'%s'"),
>> + hard_limit);
>
> lxcConvertSize already reports an error: "failed to convert size:
'%s'". And
> since the one you added provides essentially the same amount of information, we
> might as well go with that one. Potentially, we could also change it so that it
> matches the wording of all the other ones for consistency reasons, i.e. the one
> you're proposing here.
> Moreover, I don't really like copying the same error message, having a goto
> error label is IMHO justified in case like these, although, we can drop it
> specifically for this function, it's different for the functions below
though...
>
Generally speaking - I agree about the common error label; however, in
this case without looking at the lxcConvertSize because the previous
code was:
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), value);
and @value was being replaced by 3 new variables each unique, using a
common label was not viable.
So there's really two patches here. The first one should remove those
error: labels because the lxcConvertSize already generates one. Then
the second patch would be to convert to VIR_AUTOFREE.
>> + return -1;
>> + }
>> size = size / 1024;
>> virDomainDefSetMemoryTotal(def, size);
>> def->mem.hard_limit = virMemoryLimitTruncate(size);
>> @@ -771,76 +777,85 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
>>
>> if (virConfGetValueString(properties,
>>
"lxc.cgroup.memory.soft_limit_in_bytes",
>> - &value) > 0) {
>> - if (lxcConvertSize(value, &size) < 0)
>> - goto error;
>> + &soft_limit) > 0) {
>> + if (lxcConvertSize(soft_limit, &size) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to parse integer:
'%s'"),
>> + soft_limit);
>> + return -1;
>> + }
>> def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
>> }
>>
>> if (virConfGetValueString(properties,
>>
"lxc.cgroup.memory.memsw.limit_in_bytes",
>> - &value) > 0) {
>> - if (lxcConvertSize(value, &size) < 0)
>> - goto error;
>> + &swap_hard_limit) > 0) {
>> + if (lxcConvertSize(swap_hard_limit, &size) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to parse integer:
'%s'"),
>> + swap_hard_limit);
>> + return -1;
>> + }
>> def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
>> }
>> return 0;
>> -
>> - error:
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("failed to parse integer: '%s'"),
value);
>> - return -1;
>
> we can definitely drop it ^here...Alternatively, we could drop the message in
> lxcConvertSize and leave the error label here as well, again, for consistency
> reasons, it's perhaps also the better way to let it behave like just like the
> virStrToX helpers.
>
>> -
>> }
>>
>> static int
>> lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
>> {
>> - char *value = NULL;
>> + VIR_AUTOFREE(char *) shares = NULL;
>> + VIR_AUTOFREE(char *) quota = NULL;
>> + VIR_AUTOFREE(char *) period = NULL;
>>
>> if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
>> - &value) > 0) {
>
> again, matter of taste, but I think that having a helper VIR_AUTOFREE(char *)
> value that gets destroyed after each block looks more clean than having three
> different function local helper variables just to match the structure members
> we're filling in.
>
Oh, so you want a rewrite of the logic to call some local that will
return some value, but then we create a local for handling when @value
is a virStrToLong_i, virStrToLong_ui, virStrToLong_ull, or
virBitmapParse. That would mean another pre-patch.
Or maybe I'm being dense and just missing your point
>> - if (virStrToLong_ull(value, NULL, 10, &def->cputune.shares) <
0)
>> - goto error;
>> + &shares) > 0) {
>> + if (virStrToLong_ull(shares, NULL, 10, &def->cputune.shares)
< 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to parse integer:
'%s'"), shares);
>> + return -1;
>> + }
>> def->cputune.sharesSpecified = true;
>> }
>>
>> if (virConfGetValueString(properties,
"lxc.cgroup.cpu.cfs_quota_us",
>> - &value) > 0) {
>> - if (virStrToLong_ll(value, NULL, 10, &def->cputune.quota) <
0)
>> - goto error;
>> + "a) > 0) {
>> + if (virStrToLong_ll(quota, NULL, 10, &def->cputune.quota) <
0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to parse integer:
'%s'"), quota);
>> + return -1;
>> + }
>> }
>>
>> if (virConfGetValueString(properties,
"lxc.cgroup.cpu.cfs_period_us",
>> - &value) > 0) {
>> - if (virStrToLong_ull(value, NULL, 10, &def->cputune.period) <
0)
>> - goto error;
>> + &period) > 0) {
>> + if (virStrToLong_ull(period, NULL, 10, &def->cputune.period)
< 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("failed to parse integer:
'%s'"), period);
>> + return -1;
>> + }
>> }
>>
>> return 0;
>> -
>> - error:
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("failed to parse integer: '%s'"),
value);
>> - return -1;
>
> I actually prefer the error label here, because we don't have to duplicate the
> same error message at multiple spots.
>
Again, maybe I'm missing something subtle in your comment.
>> }
>>
>> static int
>> lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties)
>> {
>> - char *value = NULL;
>> + VIR_AUTOFREE(char *) cpus = NULL;
>> + VIR_AUTOFREE(char *) mems = NULL;
>> virBitmapPtr nodeset = NULL;
>>
>> if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
>> - &value) > 0) {
>
> same comments apply here too...additionally (just a note out of sheer rant), we
> should do something about the error within virBitmapParse, since that one isn't
> going to help literally anyone, so there's no way for the user to know why
> their XML failed to parse.
>
Oh so you want me to step into the cpuset="" argument ;-) No thanks!
>> - if (virBitmapParse(value, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN)
< 0)
>> + &cpus) > 0) {
>> + if (virBitmapParse(cpus, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN)
< 0)
>> return -1;
>> def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC;
>> }
>>
>> if (virConfGetValueString(properties, "lxc.cgroup.cpuset.mems",
>> - &value) > 0) {
>> - if (virBitmapParse(value, &nodeset, VIR_DOMAIN_CPUMASK_LEN) <
0)
>> + &mems) > 0) {
>> + if (virBitmapParse(mems, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
>> return -1;
>> if (virDomainNumatuneSet(def->numa,
>> def->placement_mode ==
>> @@ -949,7 +964,7 @@ lxcBlkioDeviceWalkCallback(const char *name, virConfValuePtr
value, void *data)
>> static int
>> lxcSetBlkioTune(virDomainDefPtr def, virConfPtr properties)
>> {
>> - char *value = NULL;
>> + VIR_AUTOFREE(char *) value = NULL;
>>
>> if (virConfGetValueString(properties, "lxc.cgroup.blkio.weight",
>> &value) > 0) {
>
> the comments further onward would be simply the same, you get the picture, but
> those are just tiny nitpicks based on personal preference I'd say (except for
> copying the same error messages all over again), with that addressed:
Nope, the picture wasn't clear, but I can/should check my coffee to see
if it's strong enough.
Well, I was a bit too fast with my thinking, missing some obvious problems
which I'd have spotted right away had I tried to write the code myself, so now
I see that it couldn't have worked using my way, but for completeness, you can
find a diff that more-or-less summarizes my original points below (also in the
attachment for easy application), we can potentially discuss that, but at this
point I'm more convinced by your version that by mine. Anyway, let me know
what your thoughts are, otherwise:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 6c5640536b..8c9d736d5c 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -219,7 +219,7 @@ lxcConvertSize(const char *size, unsigned long long *value)
/* Split the string into value and unit */
if (virStrToLong_ull(size, &unit, 10, value) < 0)
- goto error;
+ return -1;
if (STREQ(unit, "%")) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -228,16 +228,10 @@ lxcConvertSize(const char *size, unsigned long long *value)
return -1;
} else {
if (virScaleInteger(value, unit, 1, ULLONG_MAX) < 0)
- goto error;
+ return -1;
}
return 0;
-
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to convert size: '%s'"),
- size);
- return -1;
}
static int
@@ -275,8 +269,12 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab)
for (i = 0; options[i]; i++) {
if ((sizeStr = STRSKIP(options[i], "size="))) {
- if (lxcConvertSize(sizeStr, &usage) < 0)
+ if (lxcConvertSize(sizeStr, &usage) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ sizeStr);
goto cleanup;
+ }
break;
}
}
@@ -756,106 +754,103 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr value, void
*data)
static int
lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
{
- VIR_AUTOFREE(char *) hard_limit = NULL;
- VIR_AUTOFREE(char *) soft_limit = NULL;
- VIR_AUTOFREE(char *) swap_hard_limit = NULL;
unsigned long long size = 0;
+ VIR_AUTOFREE(char *) value = NULL;
if (virConfGetValueString(properties,
"lxc.cgroup.memory.limit_in_bytes",
- &hard_limit) > 0) {
- if (lxcConvertSize(hard_limit, &size) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"),
- hard_limit);
- return -1;
- }
+ &value) > 0) {
+ if (lxcConvertSize(value, &size) < 0)
+ goto error;
+
size = size / 1024;
virDomainDefSetMemoryTotal(def, size);
def->mem.hard_limit = virMemoryLimitTruncate(size);
+ VIR_FREE(value);
}
if (virConfGetValueString(properties,
"lxc.cgroup.memory.soft_limit_in_bytes",
- &soft_limit) > 0) {
- if (lxcConvertSize(soft_limit, &size) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"),
- soft_limit);
- return -1;
- }
+ &value) > 0) {
+ if (lxcConvertSize(value, &size) < 0)
+ goto error;
+
def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
+ VIR_FREE(value);
}
if (virConfGetValueString(properties,
"lxc.cgroup.memory.memsw.limit_in_bytes",
- &swap_hard_limit) > 0) {
- if (lxcConvertSize(swap_hard_limit, &size) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"),
- swap_hard_limit);
- return -1;
- }
+ &value) > 0) {
+ if (lxcConvertSize(value, &size) < 0)
+ goto error;
+
def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
}
+
return 0;
+
+ error:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"),
+ value);
+ return 1;
}
static int
lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
{
- VIR_AUTOFREE(char *) shares = NULL;
- VIR_AUTOFREE(char *) quota = NULL;
- VIR_AUTOFREE(char *) period = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
- &shares) > 0) {
- if (virStrToLong_ull(shares, NULL, 10, &def->cputune.shares) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"),
shares);
- return -1;
- }
+ &value) > 0) {
+ if (virStrToLong_ull(value, NULL, 10, &def->cputune.shares) < 0)
+ goto error;
+
def->cputune.sharesSpecified = true;
+ VIR_FREE(value);
}
if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_quota_us",
- "a) > 0) {
- if (virStrToLong_ll(quota, NULL, 10, &def->cputune.quota) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), quota);
- return -1;
- }
+ &value) > 0) {
+ if (virStrToLong_ll(value, NULL, 10, &def->cputune.quota) < 0)
+ goto error;
+
+ VIR_FREE(value);
}
if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_period_us",
- &period) > 0) {
- if (virStrToLong_ull(period, NULL, 10, &def->cputune.period) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"),
period);
- return -1;
- }
+ &value) > 0) {
+ if (virStrToLong_ull(value, NULL, 10, &def->cputune.period) < 0)
+ goto error;
}
return 0;
+
+ error:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse integer: '%s'"), value);
+ return -1;
}
static int
lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties)
{
- VIR_AUTOFREE(char *) cpus = NULL;
- VIR_AUTOFREE(char *) mems = NULL;
virBitmapPtr nodeset = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
- &cpus) > 0) {
- if (virBitmapParse(cpus, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
+ &value) > 0) {
+ if (virBitmapParse(value, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
return -1;
+
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC;
+ VIR_FREE(value);
}
if (virConfGetValueString(properties, "lxc.cgroup.cpuset.mems",
- &mems) > 0) {
- if (virBitmapParse(mems, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
+ &value) > 0) {
+ if (virBitmapParse(value, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
return -1;
if (virDomainNumatuneSet(def->numa,
def->placement_mode ==
@@ -1011,8 +1006,7 @@ lxcParseConfigString(const char *config,
{
virDomainDefPtr vmdef = NULL;
virConfPtr properties = NULL;
- VIR_AUTOFREE(char *) lxcarch = NULL;
- VIR_AUTOFREE(char *) lxcutsname = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
if (!(properties = virConfReadString(config, VIR_CONF_FLAG_LXC_FORMAT)))
return NULL;
@@ -1045,20 +1039,21 @@ lxcParseConfigString(const char *config,
vmdef->nfss = 0;
vmdef->os.type = VIR_DOMAIN_OSTYPE_EXE;
- if (virConfGetValueString(properties, "lxc.arch", &lxcarch) > 0) {
- virArch arch = virArchFromString(lxcarch);
- if (arch == VIR_ARCH_NONE && STREQ(lxcarch, "x86"))
+ if (virConfGetValueString(properties, "lxc.arch", &value) > 0) {
+ virArch arch = virArchFromString(value);
+ if (arch == VIR_ARCH_NONE && STREQ(value, "x86"))
arch = VIR_ARCH_I686;
- else if (arch == VIR_ARCH_NONE && STREQ(lxcarch, "amd64"))
+ else if (arch == VIR_ARCH_NONE && STREQ(value, "amd64"))
arch = VIR_ARCH_X86_64;
vmdef->os.arch = arch;
+ VIR_FREE(value);
}
if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
goto error;
- if (virConfGetValueString(properties, "lxc.utsname", &lxcutsname) <=
0 ||
- VIR_STRDUP(vmdef->name, lxcutsname) < 0)
+ if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0
||
+ VIR_STRDUP(vmdef->name, value) < 0)
goto error;
if (!vmdef->name && (VIR_STRDUP(vmdef->name, "unnamed") <
0))
goto error;
7:47 a.m.
New subject: [libvirt] [PATCH 2/3] lxc: Resolve memory leak
[...]
>> the comments further onward would be simply the same, you get
the picture, but
>> those are just tiny nitpicks based on personal preference I'd say (except
for
>> copying the same error messages all over again), with that addressed:
>
> Nope, the picture wasn't clear, but I can/should check my coffee to see
> if it's strong enough.
Well, I was a bit too fast with my thinking, missing some obvious problems
which I'd have spotted right away had I tried to write the code myself, so now
I see that it couldn't have worked using my way, but for completeness, you can
find a diff that more-or-less summarizes my original points below (also in the
attachment for easy application), we can potentially discuss that, but at this
point I'm more convinced by your version that by mine. Anyway, let me know
what your thoughts are, otherwise:
Ahhh - now I see... Since I hadn't followed all the VIR_AUTOFREE stuff
that closely - I figured perhaps from the few examples that I'd seen
that each virConfGetValueString would have a uniquely named autofree
pointer rather than using a common name and using VIR_FREE "as needed"
and then reloading. Having a unique name just didn't seem that "costly".
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 6c5640536b..8c9d736d5c 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -219,7 +219,7 @@ lxcConvertSize(const char *size, unsigned long long *value)
/* Split the string into value and unit */
if (virStrToLong_ull(size, &unit, 10, value) < 0)
- goto error;
+ return -1;
if (STREQ(unit, "%")) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -228,16 +228,10 @@ lxcConvertSize(const char *size, unsigned long long *value)
return -1;
} else {
if (virScaleInteger(value, unit, 1, ULLONG_MAX) < 0)
- goto error;
+ return -1;
}
return 0;
-
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to convert size: '%s'"),
- size);
- return -1;
I actually figured this was a better model rather than the other way, so
what I did was create a "pre" patch to this one that removed the error
message from lxcSetMemTune and just "return -1;" directly.
I can just send a v2 with the lxc_native.c changes - it should take all
of about 10 seconds to look at ;-)
John
}
[...]
4:34 p.m.
New subject: [libvirt] [PATCH 3/3] tests: Resolve possible overrun
Coverity noted that each of the fmemopen called used the strlen value
in order to allocate space, but that neglected space for terminating
null string. So just add 1 to the strlen.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/vircgroupmock.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
index d512417789..6587fb3c7e 100644
--- a/tests/vircgroupmock.c
+++ b/tests/vircgroupmock.c
@@ -466,12 +466,13 @@ FILE *fopen(const char *path, const char *mode)
if (STREQ(mode, "r")) {
if (allinone)
return fmemopen((void *)procmountsallinone,
- strlen(procmountsallinone), mode);
+ strlen(procmountsallinone) + 1, mode);
else if (logind)
return fmemopen((void *)procmountslogind,
- strlen(procmountslogind), mode);
+ strlen(procmountslogind) + 1, mode);
else
- return fmemopen((void *)procmounts, strlen(procmounts), mode);
+ return fmemopen((void *)procmounts,
+ strlen(procmounts) + 1, mode);
} else {
errno = EACCES;
return NULL;
@@ -481,12 +482,13 @@ FILE *fopen(const char *path, const char *mode)
if (STREQ(mode, "r")) {
if (allinone)
return fmemopen((void *)proccgroupsallinone,
- strlen(proccgroupsallinone), mode);
+ strlen(proccgroupsallinone) + 1, mode);
else if (logind)
return fmemopen((void *)proccgroupslogind,
- strlen(proccgroupslogind), mode);
+ strlen(proccgroupslogind) + 1, mode);
else
- return fmemopen((void *)proccgroups, strlen(proccgroups), mode);
+ return fmemopen((void *)proccgroups,
+ strlen(proccgroups) + 1, mode);
} else {
errno = EACCES;
return NULL;
@@ -496,12 +498,13 @@ FILE *fopen(const char *path, const char *mode)
if (STREQ(mode, "r")) {
if (allinone)
return fmemopen((void *)procselfcgroupsallinone,
- strlen(procselfcgroupsallinone), mode);
+ strlen(procselfcgroupsallinone) + 1, mode);
else if (logind)
return fmemopen((void *)procselfcgroupslogind,
- strlen(procselfcgroupslogind), mode);
+ strlen(procselfcgroupslogind) + 1, mode);
else
- return fmemopen((void *)procselfcgroups, strlen(procselfcgroups), mode);
+ return fmemopen((void *)procselfcgroups,
+ strlen(procselfcgroups) + 1, mode);
} else {
errno = EACCES;
return NULL;
--
2.17.1
2:51 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Alter when ctxt->node is set
On Thu, Sep 20, 2018 at 05:34:36PM -0400, John Ferlan wrote:
In virDomainMemoryDefParseXML and virDomainVideoDefParseXML if
the VIR_ALLOC fails and NULL is returned, then the alteration
to ctxt->node isn't reversed.
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
3:47 a.m.
New subject: [libvirt] [PATCH 3/3] tests: Resolve possible overrun
On Thu, Sep 20, 2018 at 05:34:38PM -0400, John Ferlan wrote:
Coverity noted that each of the fmemopen called used the strlen
value
in order to allocate space, but that neglected space for terminating
null string. So just add 1 to the strlen.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
Although, I'm wondering whether it's worth having an internal wrapper like the
STREQ stuff we have to mitigate the off-by-1 errors in cases like these, but I
guess there would be cases, where that might be undesirable.
2251
days inactive
2255
days old
9 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan