On Wed, Feb 05, 2014 at 03:10:02PM +0100, Cédric Bosdonnat wrote:
> Tmpfs relative size and default 50% size values aren't supported as
> we have no idea of the available memory at the conversion time.
> ---
> src/lxc/lxc_container.c | 2 +-
> src/lxc/lxc_container.h | 2 +
> src/lxc/lxc_native.c | 251 +++++++++++++++++++++++-
> tests/lxcconf2xmldata/lxcconf2xml-fstab.config | 37 ++++
> tests/lxcconf2xmldata/lxcconf2xml-simple.config | 4 +-
> tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 9 +
> tests/lxcconf2xmltest.c | 60 +++---
> 7 files changed, 336 insertions(+), 29 deletions(-)
> create mode 100644 tests/lxcconf2xmldata/lxcconf2xml-fstab.config
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index c6bdc8c..f08dbc2 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -744,7 +744,7 @@ static const virLXCBasicMountInfo lxcBasicMounts[] = {
> };
>
>
> -static bool lxcIsBasicMountLocation(const char *path)
> +bool lxcIsBasicMountLocation(const char *path)
> {
> size_t i;
>
> diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h
> index e74a7d7..67292ab 100644
> --- a/src/lxc/lxc_container.h
> +++ b/src/lxc/lxc_container.h
> @@ -71,4 +71,6 @@ virArch lxcContainerGetAlt32bitArch(virArch arch);
>
> int lxcContainerChown(virDomainDefPtr def, const char *path);
>
> +bool lxcIsBasicMountLocation(const char *path);
> +
> #endif /* LXC_CONTAINER_H */
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index 86d0dd3..acd68db 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -21,10 +21,13 @@
> */
>
> #include <config.h>
> +#include <stdio.h>
>
> #include "internal.h"
> +#include "lxc_container.h"
> #include "lxc_native.h"
> #include "util/viralloc.h"
> +#include "util/virfile.h"
> #include "util/virlog.h"
> #include "util/virstring.h"
> #include "util/virconf.h"
> @@ -33,7 +36,11 @@
>
>
> static virDomainFSDefPtr
> -lxcCreateFSDef(int type, char *src, char* dst)
> +lxcCreateFSDef(int type,
> + char *src,
> + char* dst,
> + bool readonly,
> + unsigned long long usage)
> {
> virDomainFSDefPtr def;
>
> @@ -44,16 +51,124 @@ lxcCreateFSDef(int type, char *src, char* dst)
> def->accessmode = VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH;
> def->src = src;
> def->dst = dst;
> + def->readonly = readonly;
> + def->usage = usage;
>
> return def;
> }
>
> +typedef struct _lxcFstab lxcFstab;
> +typedef lxcFstab *lxcFstabPtr;
> +struct _lxcFstab {
> + lxcFstabPtr next;
> + char *src;
> + char *dst;
> + char *type;
> + char *options;
> +};
> +
> +static void
> +lxcFstabFree(lxcFstabPtr fstab)
> +{
> + while (fstab) {
> + lxcFstabPtr next = NULL;
> + next = fstab->next;
> +
> + VIR_FREE(fstab->src);
> + VIR_FREE(fstab->dst);
> + VIR_FREE(fstab->type);
> + VIR_FREE(fstab->options);
> + VIR_FREE(fstab);
> +
> + fstab = next;
> + }
> +}
> +
> +static char ** lxcStringSplit(const char *string)
> +{
> + char *tmp;
> + size_t i;
> + size_t ntokens = 0;
> + char **parts;
> + char **result = NULL;
> +
> + if (VIR_STRDUP(tmp, string) < 0)
> + return NULL;
> +
> + /* Replace potential \t by a space */
> + for (i = 0; tmp[i]; i++) {
> + if (tmp[i] == '\t')
> + tmp[i] = ' ';
> + }
> +
> + parts = virStringSplit(tmp, " ", 0);
Not checking parts for NULL
> + for (i = 0; parts[i]; i++) {
> + if (STREQ(parts[i], ""))
> + continue;
> +
> + if (VIR_EXPAND_N(result, ntokens, 1) < 0)
> + goto error;
> +
> + if (VIR_STRDUP(result[ntokens-1], parts[i]) < 0)
> + goto error;
> + }
> +
> + /* Append NULL element */
> + if (VIR_EXPAND_N(result, ntokens, 1) < 0)
> + goto error;
A really subtle problem here - we need results to be
NULL terminated at all times, not only when the loop
is finished. If the VIR_EXPAND_N or VIR_STRDUP calls
in the loop body fail, we jump to error where we
call virStringFreeList which expects NULL termination.
As it stands we have array out of bounds access on OOM.
> +
> + VIR_FREE(tmp);
> + virStringFreeList(parts);
> + return result;
> +
> +error:
> + VIR_FREE(tmp);
> + virStringFreeList(parts);
> + virStringFreeList(result);
> + return NULL;
> +}
> +
> +static lxcFstabPtr
> +lxcParseFstabLine(char *fstabLine)
> +{
> + lxcFstabPtr fstab = NULL;
> + char **parts;
> +
> + if (!fstabLine || VIR_ALLOC(fstab) < 0)
> + return NULL;
> +
> + parts = lxcStringSplit(fstabLine);
Not checking for NULL
> +
> + if (!parts[0] || !parts[1] || !parts[2] || !parts[3])
> + goto error;
> +
> + if (VIR_STRDUP(fstab->src, parts[0]) < 0 ||
> + VIR_STRDUP(fstab->dst, parts[1]) < 0 ||
> + VIR_STRDUP(fstab->type, parts[2]) < 0 ||
> + VIR_STRDUP(fstab->options, parts[3]) < 0)
> + goto error;
> +
> + virStringFreeList(parts);
> +
> + return fstab;
> +
> +error:
> + lxcFstabFree(fstab);
> + virStringFreeList(parts);
> + return NULL;
> +}
> +
> static int
> -lxcAddFSDef(virDomainDefPtr def, int type, char *src, char *dst)
> +lxcAddFSDef(virDomainDefPtr def,
> + int type,
> + char *src,
> + char *dst,
> + bool readonly,
> + unsigned long long usage)
> {
> virDomainFSDefPtr fsDef = NULL;
>
> - if (!(fsDef = lxcCreateFSDef(type, src, dst)))
> + if (!(fsDef = lxcCreateFSDef(type, src, dst, readonly, usage)))
> goto error;
>
> if (VIR_EXPAND_N(def->fss, def->nfss, 1) < 0)
> @@ -86,7 +201,7 @@ lxcSetRootfs(virDomainDefPtr def,
> type = VIR_DOMAIN_FS_TYPE_BLOCK;
>
>
> - if (lxcAddFSDef(def, type, fssrc, fsdst) < 0)
> + if (lxcAddFSDef(def, type, fssrc, fsdst, false, 0) < 0)
> goto error;
>
> return 0;
> +static int
> +lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab)
> +{
> + char *src = NULL;
> + char *dst = NULL;
> + char **options = virStringSplit(fstab->options, ",", 0);
> + bool readonly;
> + int type = VIR_DOMAIN_FS_TYPE_MOUNT;
> + unsigned long long usage = 0;
This method leaks 'options' array.
> +
> + if (fstab->dst[0] != '/') {
> + if (virAsprintf(&dst, "/%s", fstab->dst) < 0)
> + goto error;
> + } else {
> + if (VIR_STRDUP(dst, fstab->dst) < 0)
> + goto error;
> + }
> +
> + /* Check that we don't add basic mounts */
> + if (lxcIsBasicMountLocation(dst)) {
> + VIR_FREE(dst);
> + return 0;
> + }
> +
> + if (STREQ(fstab->type, "tmpfs")) {
> + char *sizeStr = NULL;
> + size_t i;
> + type = VIR_DOMAIN_FS_TYPE_RAM;
> +
> + for (i = 0; options[i]; i++) {
> + if ((sizeStr = STRSKIP(options[i], "size="))) {
> + if (lxcConvertSize(sizeStr, &usage) < 0)
> + goto error;
> + break;
> + }
> + }
> + if (!sizeStr) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing tmpfs size, set the size option"));
> + goto error;
> + }
> + } else {
> + if (VIR_STRDUP(src, fstab->src) < 0)
> + goto error;
> + }
> +
> + /* Do we have ro in options? */
> + readonly = virStringArrayHasString(options, "ro");
> +
> + if (lxcAddFSDef(def, type, src, dst, readonly, usage) < 0)
> + goto error;
> +
> +
> + return 1;
> +
> +error:
> + VIR_FREE(dst);
> + VIR_FREE(src);
> + virStringFreeList(options);
> + return -1;
> +}
ACK, I'm going to push and squash in 2 fixes
For the memory leaks:
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index acd68db..c740e9f 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -249,19 +249,20 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab)
bool readonly;
int type = VIR_DOMAIN_FS_TYPE_MOUNT;
unsigned long long usage = 0;
+ int ret = -1;
if (fstab->dst[0] != '/') {
if (virAsprintf(&dst, "/%s", fstab->dst) < 0)
- goto error;
+ goto cleanup;
} else {
if (VIR_STRDUP(dst, fstab->dst) < 0)
- goto error;
+ goto cleanup;
}
/* Check that we don't add basic mounts */
if (lxcIsBasicMountLocation(dst)) {
- VIR_FREE(dst);
- return 0;
+ ret = 0;
+ goto cleanup;
}
if (STREQ(fstab->type, "tmpfs")) {
@@ -272,34 +273,34 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab)
for (i = 0; options[i]; i++) {
if ((sizeStr = STRSKIP(options[i], "size="))) {
if (lxcConvertSize(sizeStr, &usage) < 0)
- goto error;
+ goto cleanup;
break;
}
}
if (!sizeStr) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing tmpfs size, set the size option"));
- goto error;
+ goto cleanup;
}
} else {
if (VIR_STRDUP(src, fstab->src) < 0)
- goto error;
+ goto cleanup;
}
/* Do we have ro in options? */
readonly = virStringArrayHasString(options, "ro");
if (lxcAddFSDef(def, type, src, dst, readonly, usage) < 0)
- goto error;
-
+ goto cleanup;
- return 1;
+ src = dst = NULL;
+ ret = 1;
-error:
+ cleanup:
VIR_FREE(dst);
VIR_FREE(src);
virStringFreeList(options);
- return -1;
+ return ret;
}
static int
And then for the OOM fixes
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 3e6c2bc..66be361 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -107,7 +107,13 @@ static char ** lxcStringSplit(const char *string)
tmp[i] = ' ';
}
- parts = virStringSplit(tmp, " ", 0);
+ if (!(parts = virStringSplit(tmp, " ", 0)))
+ goto error;
+
+ /* Append NULL element */
+ if (VIR_EXPAND_N(result, ntokens, 1) < 0)
+ goto error;
+
for (i = 0; parts[i]; i++) {
if (STREQ(parts[i], ""))
continue;
@@ -115,14 +121,10 @@ static char ** lxcStringSplit(const char *string)
if (VIR_EXPAND_N(result, ntokens, 1) < 0)
goto error;
- if (VIR_STRDUP(result[ntokens-1], parts[i]) < 0)
+ if (VIR_STRDUP(result[ntokens-2], parts[i]) < 0)
goto error;
}
- /* Append NULL element */
- if (VIR_EXPAND_N(result, ntokens, 1) < 0)
- goto error;
-
VIR_FREE(tmp);
virStringFreeList(parts);
return result;
@@ -143,7 +145,8 @@ lxcParseFstabLine(char *fstabLine)
if (!fstabLine || VIR_ALLOC(fstab) < 0)
return NULL;
- parts = lxcStringSplit(fstabLine);
+ if (!(parts = lxcStringSplit(fstabLine)))
+ goto error;
if (!parts[0] || !parts[1] || !parts[2] || !parts[3])
goto error;
@@ -250,6 +253,9 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab)
unsigned long long usage = 0;
int ret = -1;
+ if (!options)
+ return -1;
+
if (fstab->dst[0] != '/') {
if (virAsprintf(&dst, "/%s", fstab->dst) < 0)
goto cleanup;
@@ -291,7 +297,6 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab)
if (lxcAddFSDef(def, type, src, dst, readonly, usage) < 0)
goto cleanup;
- dst = NULL;
ret = 1;
cleanup:
--
|: Http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list