thr3ads.net - Xen devel - [Xen-devel] [PATCH]: xl: introduce xlu_cfg_get_(string|long)

If this information is useful, please help other people find it:
Share via:

Gianni Tedesco

2010-Aug-31 15:42 UTC

[Xen-devel] [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

The function init_dm_info() in xl_cmdimpl.c is used to initialise a
libxl_device_model_info structure with some default values. After being
called, some of those values are overridden. For the string values which
are not overwritten we either end up with a double free or attempt to
free a string literal resulting in a segfault. This type of usage model
would be complex to fix by adding strdup()''s in the init function and
then checking and freeing when over-writing.

My proposed solution is to add default versions of xlu_cfg_get_string
and xlu_cfg_get_long. This way both double-free''s and leaks are avoided
and arguably the intention of the code is clearer.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r d323c34ec982 -r aa90461b1d04 tools/libxl/libxlu_cfg.c
--- a/tools/libxl/libxlu_cfg.c	Tue Aug 31 13:30:21 2010 +0100
+++ b/tools/libxl/libxlu_cfg.c	Tue Aug 31 14:01:07 2010 +0100
@@ -151,7 +151,23 @@ int xlu_cfg_get_string(const XLU_Config 
     *value_r= set->values[0];
     return 0;
 }
-        
+
+int xlu_cfg_get_string_default(const XLU_Config *cfg, const char *n,
+                       const char **value_r, const char *def) {
+    XLU_ConfigSetting *set;
+    int e;
+
+    e= find_atom(cfg,n,&set);
+    if (e) {
+        *value_r = strdup(def);
+        if ( def && NULL == *value_r )
+            return ENOMEM;
+        return 0;
+    }
+    *value_r= set->values[0];
+    return 0;
+}
+
 int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
                      long *value_r) {
     long l;
@@ -181,6 +197,35 @@ int xlu_cfg_get_long(const XLU_Config *c
     return 0;
 }
         
+int xlu_cfg_get_long_default(const XLU_Config *cfg, const char *n,
+                     long *value_r, long def) {
+    long l;
+    XLU_ConfigSetting *set;
+    int e;
+    char *ep;
+
+    e= find_atom(cfg,n,&set);  if (e) { *value_r = def; return 0; }
+    errno= 0; l= strtol(set->values[0], &ep, 0);
+    e= errno;
+    if (errno) {
+        e= errno;
+        assert(e==EINVAL || e==ERANGE);
+        fprintf(cfg->report,
+                "%s:%d: warning: parameter `%s'' could not be
parsed"
+                " as a number: %s\n",
+                cfg->filename, set->lineno, n, strerror(e));
+        *value_r = def;
+        return e;
+    }
+    if (*ep || ep==set->values[0]) {
+        fprintf(cfg->report,
+                "%s:%d: warning: parameter `%s'' is not a valid
number\n",
+                cfg->filename, set->lineno, n);
+        return EINVAL;
+    }
+    *value_r= l;
+    return 0;
+}
 
 int xlu_cfg_get_list(const XLU_Config *cfg, const char *n,
                      XLU_ConfigList **list_r, int *entries_r) {
diff -r d323c34ec982 -r aa90461b1d04 tools/libxl/libxlutil.h
--- a/tools/libxl/libxlutil.h	Tue Aug 31 13:30:21 2010 +0100
+++ b/tools/libxl/libxlutil.h	Tue Aug 31 14:01:07 2010 +0100
@@ -46,7 +46,10 @@ void xlu_cfg_destroy(XLU_Config*);
  */
 
 int xlu_cfg_get_string(const XLU_Config*, const char *n, const char **value_r);
+int xlu_cfg_get_string_default(const XLU_Config *cfg, const char *n,
+                                const char **value_r, const char *def);
 int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r);
+int xlu_cfg_get_long_default(const XLU_Config*, const char *n, long *value_r,
long def);
 
 int xlu_cfg_get_list(const XLU_Config*, const char *n,
                      XLU_ConfigList **list_r /* may be 0 */,
diff -r d323c34ec982 -r aa90461b1d04 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Aug 31 13:30:21 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Tue Aug 31 14:01:07 2010 +0100
@@ -286,36 +286,6 @@ static void init_build_info(libxl_domain
     }
 }
 
-static void init_dm_info(libxl_device_model_info *dm_info,
-        libxl_domain_create_info *c_info, libxl_domain_build_info *b_info)
-{
-    memset(dm_info, ''\0'', sizeof(*dm_info));
-
-    libxl_uuid_generate(&dm_info->uuid);
-
-    dm_info->dom_name = c_info->name;
-    dm_info->device_model = "qemu-dm";
-    dm_info->videoram = b_info->video_memkb / 1024;
-    dm_info->apic = b_info->u.hvm.apic;
-    dm_info->vcpus = b_info->max_vcpus;
-    dm_info->vcpu_avail = b_info->cur_vcpus;
-
-    dm_info->stdvga = 0;
-    dm_info->vnc = 1;
-    dm_info->vnclisten = "127.0.0.1";
-    dm_info->vncdisplay = 0;
-    dm_info->vncunused = 1;
-    dm_info->keymap = NULL;
-    dm_info->sdl = 0;
-    dm_info->opengl = 0;
-    dm_info->nographic = 0;
-    dm_info->serial = NULL;
-    dm_info->boot = "cda";
-    dm_info->usb = 0;
-    dm_info->usbdevice = NULL;
-    dm_info->xen_platform_pci = 1;
-}
-
 static void init_nic_info(libxl_device_nic *nic_info, int devnum)
 {
     const uint8_t *r;
@@ -1008,22 +978,31 @@ skip_vfb:
 
     if (c_info->hvm == 1) {
         /* init dm from c and b */
-        init_dm_info(dm_info, c_info, b_info);
+        memset(dm_info, ''\0'', sizeof(*dm_info));
+
+        libxl_uuid_generate(&dm_info->uuid);
+        dm_info->dom_name = strdup(c_info->name);
+
+        dm_info->videoram = b_info->video_memkb / 1024;
+        dm_info->apic = b_info->u.hvm.apic;
+        dm_info->vcpus = b_info->max_vcpus;
+        dm_info->vcpu_avail = b_info->cur_vcpus;
 
         /* then process config related to dm */
-        if (!xlu_cfg_get_string (config, "device_model", &buf))
+        if (!xlu_cfg_get_string_default (config, "device_model",
&buf, "qemu-dm"))
             dm_info->device_model = strdup(buf);
+
         if (!xlu_cfg_get_long (config, "stdvga", &l))
             dm_info->stdvga = l;
-        if (!xlu_cfg_get_long (config, "vnc", &l))
+        if (!xlu_cfg_get_long_default (config, "vnc", &l, 1))
             dm_info->vnc = l;
-        if (!xlu_cfg_get_string (config, "vnclisten", &buf))
+        if (!xlu_cfg_get_string_default (config, "vnclisten",
&buf, "127.0.0.1"))
             dm_info->vnclisten = strdup(buf);
         if (!xlu_cfg_get_string (config, "vncpasswd", &buf))
             dm_info->vncpasswd = strdup(buf);
         if (!xlu_cfg_get_long (config, "vncdisplay", &l))
             dm_info->vncdisplay = l;
-        if (!xlu_cfg_get_long (config, "vncunused", &l))
+        if (!xlu_cfg_get_long_default (config, "vncunused", &l,
1))
             dm_info->vncunused = l;
         if (!xlu_cfg_get_string (config, "keymap", &buf))
             dm_info->keymap = strdup(buf);
@@ -1035,7 +1014,7 @@ skip_vfb:
             dm_info->nographic = l;
         if (!xlu_cfg_get_string (config, "serial", &buf))
             dm_info->serial = strdup(buf);
-        if (!xlu_cfg_get_string (config, "boot", &buf))
+        if (!xlu_cfg_get_string_default (config, "boot", &buf,
"cda"))
             dm_info->boot = strdup(buf);
         if (!xlu_cfg_get_long (config, "usb", &l))
             dm_info->usb = l;
@@ -1043,7 +1022,7 @@ skip_vfb:
             dm_info->usbdevice = strdup(buf);
         if (!xlu_cfg_get_string (config, "soundhw", &buf))
             dm_info->soundhw = strdup(buf);
-        if (!xlu_cfg_get_long (config, "xen_platform_pci", &l))
+        if (!xlu_cfg_get_long_default (config, "xen_platform_pci",
&l, 1))
             dm_info->xen_platform_pci = l;
     }
 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-31 16:12 UTC

head link

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

On Tue, 2010-08-31 at 16:42 +0100, Gianni Tedesco wrote:> The function init_dm_info() in xl_cmdimpl.c is used to initialise a
> libxl_device_model_info structure with some default values. After being
> called, some of those values are overridden. For the string values which
> are not overwritten we either end up with a double free or attempt to
> free a string literal resulting in a segfault. This type of usage model
> would be complex to fix by adding strdup()''s in the init function
and
> then checking and freeing when over-writing.
> 
> My proposed solution is to add default versions of xlu_cfg_get_string
> and xlu_cfg_get_long.
I like the idea but is it not possible to implement these as wrappers
around the non-default providing versions and therefore avoid
duplicating the code? (or maybe the other way round, defining the
non-default variants in terms of default==NULL etc).
>          /* then process config related to dm */
> -        if (!xlu_cfg_get_string (config, "device_model",
&buf))
> +        if (!xlu_cfg_get_string_default (config, "device_model",
&buf, "qemu-dm"))
>              dm_info->device_model = strdup(buf);
Hasn''t buf already been strdupped by xlu_cfg_get_string_default if the
default ends up being used?

I''m not sure about set->values[0] in the other case but presumably
it is
not already dupped or we wouldn''t already be doing it again. In which
case it looks like xlu_cfg_get_string_default should return the literal
undup''d default and let the caller take care of dupping it.

Presumably this is the same in the other cases too.

+int xlu_cfg_get_long_default(const XLU_Config *cfg, const char
*n,> +                     long *value_r, long def) {
> +    long l;
> +    XLU_ConfigSetting *set;
> +    int e;
> +    char *ep;
> +
> +    e= find_atom(cfg,n,&set);  if (e) { *value_r = def; return 0; }
> +    errno= 0; l= strtol(set->values[0], &ep, 0);
> +    e= errno;
> +    if (errno) {
> +        e= errno;
> +        assert(e==EINVAL || e==ERANGE);
> +        fprintf(cfg->report,
> +                "%s:%d: warning: parameter `%s'' could not be
parsed"
> +                " as a number: %s\n",
> +                cfg->filename, set->lineno, n, strerror(e));
> +        *value_r = def;
It is unclear if the default should be used or a more serious error
raised in the case of failure to parse the value if it is present, as
opposed to the value not being present. I don''t think you are changing
the existing semantics though.
> +        return e;
> +    }
> +    if (*ep || ep==set->values[0]) {
> +        fprintf(cfg->report,
> +                "%s:%d: warning: parameter `%s'' is not a
valid number
\n",> +                cfg->filename, set->lineno, n);
> +        return EINVAL;
> +    }
> +    *value_r= l;
> +    return 0;
Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gianni Tedesco

2010-Aug-31 16:22 UTC

head link

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

On Tue, 2010-08-31 at 17:12 +0100, Ian Campbell wrote:> On Tue, 2010-08-31 at 16:42 +0100, Gianni Tedesco wrote:
> > The function init_dm_info() in xl_cmdimpl.c is used to initialise a
> > libxl_device_model_info structure with some default values. After
being
> > called, some of those values are overridden. For the string values
which
> > are not overwritten we either end up with a double free or attempt to
> > free a string literal resulting in a segfault. This type of usage
model
> > would be complex to fix by adding strdup()''s in the init
function and
> > then checking and freeing when over-writing.
> > 
> > My proposed solution is to add default versions of xlu_cfg_get_string
> > and xlu_cfg_get_long.
> 
> I like the idea but is it not possible to implement these as wrappers
> around the non-default providing versions and therefore avoid
> duplicating the code? (or maybe the other way round, defining the
> non-default variants in terms of default==NULL etc).
see discussion below
> >          /* then process config related to dm */
> > -        if (!xlu_cfg_get_string (config, "device_model",
&buf))
> > +        if (!xlu_cfg_get_string_default (config,
"device_model", &buf, "qemu-dm"))
> >              dm_info->device_model = strdup(buf);
> 
> Hasn''t buf already been strdupped by xlu_cfg_get_string_default if
the
> default ends up being used?
> 
> I''m not sure about set->values[0] in the other case but
presumably it is
> not already dupped or we wouldn''t already be doing it again. In
which
> case it looks like xlu_cfg_get_string_default should return the literal
> undup''d default and let the caller take care of dupping it.
> 
> Presumably this is the same in the other cases too.
Yes that is broken, it leaks. Will fix and re-send.
> +int xlu_cfg_get_long_default(const XLU_Config *cfg, const char *n,
> > +                     long *value_r, long def) {
> > +    long l;
> > +    XLU_ConfigSetting *set;
> > +    int e;
> > +    char *ep;
> > +
> > +    e= find_atom(cfg,n,&set);  if (e) { *value_r = def; return 0;
}
> > +    errno= 0; l= strtol(set->values[0], &ep, 0);
> > +    e= errno;
> > +    if (errno) {
> > +        e= errno;
> > +        assert(e==EINVAL || e==ERANGE);
> > +        fprintf(cfg->report,
> > +                "%s:%d: warning: parameter `%s'' could
not be parsed"
> > +                " as a number: %s\n",
> > +                cfg->filename, set->lineno, n, strerror(e));
> > +        *value_r = def;
> 
> It is unclear if the default should be used or a more serious error
> raised in the case of failure to parse the value if it is present, as
> opposed to the value not being present. I don''t think you are
changing
> the existing semantics though.
I implemented these semantics since it reflects current practice where
callers do not check the return value. It is also why I had duplicated
the code. Not sure exactly what to do with this...

I am all in favour of pressing on after getting some bad digits, abort()
would be too harsh, but converting all callers to distinguish between
not-present, present-and-valid and present-but-not-valid would be a lot
of extra lines of code.

Also, in xm, wouldn''t a lot of these integers used as boolean be
totally
valid if assigned with True/False?

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2010-Aug-31 16:50 UTC

head link

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

Gianni Tedesco writes ("[PATCH]: xl: introduce
xlu_cfg_get_(string|long)_default"):> The function init_dm_info() in xl_cmdimpl.c is used to initialise a
> libxl_device_model_info structure with some default values. After being
> called, some of those values are overridden. For the string values which
> are not overwritten we either end up with a double free or attempt to
> free a string literal resulting in a segfault. This type of usage model
> would be complex to fix by adding strdup()''s in the init function
and
> then checking and freeing when over-writing.
> 
> My proposed solution is to add default versions of xlu_cfg_get_string
> and xlu_cfg_get_long. This way both double-free''s and leaks are
avoided
> and arguably the intention of the code is clearer.
I think in general the approach of trying to do the defaults one
config setting at a time like this is insufficiently powerful.

If we''re going to change it, it would be better to do all the default
setting at the end, after the config file has been completely read.
That means that the default for one config setting can depend on the
actual values for another.

It also gets rid of the memory management problem, because defaults
never need to be applied to anything that is a non-null pointer.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gianni Tedesco

2010-Aug-31 17:04 UTC

head link

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

On Tue, 2010-08-31 at 17:50 +0100, Ian Jackson wrote:> Gianni Tedesco writes ("[PATCH]: xl: introduce
xlu_cfg_get_(string|long)_default"):
> > The function init_dm_info() in xl_cmdimpl.c is used to initialise a
> > libxl_device_model_info structure with some default values. After
being
> > called, some of those values are overridden. For the string values
which
> > are not overwritten we either end up with a double free or attempt to
> > free a string literal resulting in a segfault. This type of usage
model
> > would be complex to fix by adding strdup()''s in the init
function and
> > then checking and freeing when over-writing.
> > 
> > My proposed solution is to add default versions of xlu_cfg_get_string
> > and xlu_cfg_get_long. This way both double-free''s and leaks
are avoided
> > and arguably the intention of the code is clearer.
> 
> I think in general the approach of trying to do the defaults one
> config setting at a time like this is insufficiently powerful.
> 
> If we''re going to change it, it would be better to do all the
default
> setting at the end, after the config file has been completely read.
> That means that the default for one config setting can depend on the
> actual values for another.
> 
> It also gets rid of the memory management problem, because defaults
> never need to be applied to anything that is a non-null pointer.
I had considered that approach first but then discarded it because it
was hard to tell which values had been set or not, eg. if a boolean
option had been set to 0 by an explicit config entry or due to initial
memset() - meaning that you wouldn''t know whether to override it with a
default or not. IOW we would need a bitmap or something to keep track.

Perhaps a higher level API would be better:

struct {
   ...
}configs[] = {
    {.type = STR, .u.str = {&dm_info.vnclisten, "127.0.0.1"}, },
    {.type = INT, .u.num = {&dm_info.vnc, 1}, },
    ...
};

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2010-Aug-31 17:15 UTC

head link

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

Gianni Tedesco writes ("Re: [PATCH]: xl: introduce
xlu_cfg_get_(string|long)_default"):> I had considered that approach first but then discarded it because it
> was hard to tell which values had been set or not, eg. if a boolean
> option had been set to 0 by an explicit config entry or due to initial
> memset() - meaning that you wouldn''t know whether to override it
with a
> default or not. IOW we would need a bitmap or something to keep track.
Yes.  Either a bitmap for which values are valid, or a convention that
booleans are ints which are given -1 or +1 values, or something.
> Perhaps a higher level API would be better:
...>     {.type = STR, .u.str = {&dm_info.vnclisten, "127.0.0.1"},
},
Urgh, that''s overly complicated, I think.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Aug 2010 - [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Xen-devel] [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default