Xen devel - Sep 2010 - [PATCH,v2]: xl: don''t free string literals

The function init_dm_info() is initialising some strings from literals.
This is bad juju because when the destructor is called we cannot know if
the string literal was overridden with a strdup()''d value. Therefore
strdup values in the initialiser then introduce and use the function
libxlu_cfg_replace_string() which free''s whatever is set before
strdupping the new value on top of it. The rule for the new call should
be clear due to const vs. non-const arguments - changing the behaviour
of libxlu_cfg_get_string() would cause more complexity than it saves.

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

diff -r e14a3c281982 tools/libxl/libxlu_cfg.c
--- a/tools/libxl/libxlu_cfg.c	Wed Sep 08 16:54:16 2010 +0100
+++ b/tools/libxl/libxlu_cfg.c	Wed Sep 08 17:27:05 2010 +0100
@@ -151,7 +151,18 @@ int xlu_cfg_get_string(const XLU_Config 
     *value_r= set->values[0];
     return 0;
 }
-        
+ 
+int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n,
+                           char **value_r) {
+    XLU_ConfigSetting *set;
+    int e;
+
+    e= find_atom(cfg,n,&set);  if (e) return e;
+    free(*value_r);
+    *value_r= strdup(set->values[0]);
+    return 0;
+}
+
 int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
                      long *value_r) {
     long l;
diff -r e14a3c281982 tools/libxl/libxlutil.h
--- a/tools/libxl/libxlutil.h	Wed Sep 08 16:54:16 2010 +0100
+++ b/tools/libxl/libxlutil.h	Wed Sep 08 17:27:05 2010 +0100
@@ -46,6 +46,7 @@ void xlu_cfg_destroy(XLU_Config*);
  */
 
 int xlu_cfg_get_string(const XLU_Config*, const char *n, const char **value_r);
+int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n, char
**value_r); /* free/strdup version */
 int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r);
 
 int xlu_cfg_get_list(const XLU_Config*, const char *n,
diff -r e14a3c281982 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Sep 08 16:54:16 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Wed Sep 08 17:27:05 2010 +0100
@@ -297,7 +297,7 @@ static void init_dm_info(libxl_device_mo
     libxl_uuid_generate(&dm_info->uuid);
 
     dm_info->dom_name = c_info->name;
-    dm_info->device_model = "qemu-dm";
+    dm_info->device_model = strdup("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;
@@ -305,7 +305,7 @@ static void init_dm_info(libxl_device_mo
 
     dm_info->stdvga = 0;
     dm_info->vnc = 1;
-    dm_info->vnclisten = "127.0.0.1";
+    dm_info->vnclisten = strdup("127.0.0.1");
     dm_info->vncdisplay = 0;
     dm_info->vncunused = 1;
     dm_info->keymap = NULL;
@@ -313,7 +313,7 @@ static void init_dm_info(libxl_device_mo
     dm_info->opengl = 0;
     dm_info->nographic = 0;
     dm_info->serial = NULL;
-    dm_info->boot = "cda";
+    dm_info->boot = strdup("cda");
     dm_info->usb = 0;
     dm_info->usbdevice = NULL;
     dm_info->xen_platform_pci = 1;
@@ -1022,38 +1022,30 @@ skip_vfb:
         init_dm_info(dm_info, c_info, b_info);
 
         /* then process config related to dm */
-        if (!xlu_cfg_get_string (config, "device_model", &buf))
-            dm_info->device_model = strdup(buf);
+        !xlu_cfg_replace_string (config, "device_model",
&dm_info->device_model);
         if (!xlu_cfg_get_long (config, "stdvga", &l))
             dm_info->stdvga = l;
         if (!xlu_cfg_get_long (config, "vnc", &l))
             dm_info->vnc = l;
-        if (!xlu_cfg_get_string (config, "vnclisten", &buf))
-            dm_info->vnclisten = strdup(buf);
-        if (!xlu_cfg_get_string (config, "vncpasswd", &buf))
-            dm_info->vncpasswd = strdup(buf);
+        xlu_cfg_replace_string (config, "vnclisten",
&dm_info->vnclisten);
+        xlu_cfg_replace_string (config, "vncpasswd",
&dm_info->vncpasswd);
         if (!xlu_cfg_get_long (config, "vncdisplay", &l))
             dm_info->vncdisplay = l;
         if (!xlu_cfg_get_long (config, "vncunused", &l))
             dm_info->vncunused = l;
-        if (!xlu_cfg_get_string (config, "keymap", &buf))
-            dm_info->keymap = strdup(buf);
+        xlu_cfg_replace_string (config, "keymap",
&dm_info->keymap);
         if (!xlu_cfg_get_long (config, "sdl", &l))
             dm_info->sdl = l;
         if (!xlu_cfg_get_long (config, "opengl", &l))
             dm_info->opengl = l;
         if (!xlu_cfg_get_long (config, "nographic", &l))
             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))
-            dm_info->boot = strdup(buf);
+        xlu_cfg_replace_string (config, "serial",
&dm_info->serial);
+        xlu_cfg_replace_string (config, "boot",
&dm_info->boot);
         if (!xlu_cfg_get_long (config, "usb", &l))
             dm_info->usb = l;
-        if (!xlu_cfg_get_string (config, "usbdevice", &buf))
-            dm_info->usbdevice = strdup(buf);
-        if (!xlu_cfg_get_string (config, "soundhw", &buf))
-            dm_info->soundhw = strdup(buf);
+        xlu_cfg_replace_string (config, "usbdevice",
&dm_info->usbdevice);
+        xlu_cfg_replace_string (config, "soundhw",
&dm_info->soundhw);
         if (!xlu_cfg_get_long (config, "xen_platform_pci", &l))
             dm_info->xen_platform_pci = l;
     }



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

On Wed, 2010-09-08 at 17:31 +0100, Gianni Tedesco wrote:
> 
>          /* then process config related to dm */
> -        if (!xlu_cfg_get_string (config, "device_model",
&buf))
> -            dm_info->device_model = strdup(buf);
> +        !xlu_cfg_replace_string (config, "device_model",
> &dm_info->device_model); 
Stray ! at the start of the new line?

Ian.



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

On Wed, 2010-09-08 at 17:31 +0100, Gianni Tedesco wrote:
> The function init_dm_info() is initialising some strings from literals.
> This is bad juju because when the destructor is called we cannot know if
> the string literal was overridden with a strdup()''d value.
Therefore
> strdup values in the initialiser then introduce and use the function
> libxlu_cfg_replace_string() which free''s whatever is set before
> strdupping the new value on top of it. The rule for the new call should
> be clear due to const vs. non-const arguments - changing the behaviour
> of libxlu_cfg_get_string() would cause more complexity than it saves.
> 
> Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
About to bail for the day, but I guess this (untested) patch makes sense
on top of this?

Avoids c_info->name being a string literal as well. Although I wonder if
maybe domain configurations with no name could just be rejected as
invalid? Having a whole bunch of domains called "test"
doesn''t seem
likely to be what anyone wants...

Ian.

Subject: xl: use xlu_cfg_replace_string in a few more places.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r d6026f6dcebf tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Sep 08 17:37:47 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Wed Sep 08 17:41:56 2010 +0100
@@ -606,10 +606,8 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "hap", &l))
         c_info->hap = l;
 
-    if (!xlu_cfg_get_string (config, "name", &buf))
-        c_info->name = strdup(buf);
-    else
-        c_info->name = "test";
+    if (xlu_cfg_replace_string (config, "name",
&c_info->name))
+        c_info->name = strdup("test");
 
     if (!xlu_cfg_get_string (config, "uuid", &buf) ) {
         if ( libxl_uuid_from_string(&c_info->uuid, buf) ) {
@@ -695,8 +693,7 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "videoram", &l))
         b_info->video_memkb = l * 1024;
 
-    if (!xlu_cfg_get_string (config, "kernel", &buf))
-        b_info->kernel.path = strdup(buf);
+    xlu_cfg_replace_string (config, "kernel",
&b_info->kernel.path);
 
     if (c_info->hvm == 1) {
         if (!xlu_cfg_get_long (config, "pae", &l))
@@ -734,10 +731,8 @@ static void parse_config_data(const char
             exit(1);
         }
 
-        if (!xlu_cfg_get_string (config, "bootloader", &buf))
-            b_info->u.pv.bootloader = strdup(buf);
-        if (!xlu_cfg_get_string (config, "bootloader_args",
&buf))
-            b_info->u.pv.bootloader_args = strdup(buf);
+        xlu_cfg_replace_string (config, "bootloader",
&b_info->u.pv.bootloader);
+        xlu_cfg_replace_string (config, "bootloader_args",
&b_info->u.pv.bootloader_args);
 
         if (!b_info->u.pv.bootloader && !b_info->kernel.path) {
             fprintf(stderr, "Neither kernel nor bootloader
specified\n");
@@ -745,8 +740,7 @@ static void parse_config_data(const char
         }
 
         b_info->u.pv.cmdline = cmdline;
-        if (!xlu_cfg_get_string (config, "ramdisk", &buf))
-            b_info->u.pv.ramdisk.path = strdup(buf);
+        xlu_cfg_replace_string (config, "ramdisk",
&b_info->u.pv.ramdisk.path);
     }
 
     if (!xlu_cfg_get_list (config, "disk", &vbds, 0)) {



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

On Wed, 2010-09-08 at 17:46 +0100, Ian Campbell wrote:
> On Wed, 2010-09-08 at 17:31 +0100, Gianni Tedesco wrote:
> > The function init_dm_info() is initialising some strings from
literals.
> > This is bad juju because when the destructor is called we cannot know
if
> > the string literal was overridden with a strdup()''d value.
Therefore
> > strdup values in the initialiser then introduce and use the function
> > libxlu_cfg_replace_string() which free''s whatever is set
before
> > strdupping the new value on top of it. The rule for the new call
should
> > be clear due to const vs. non-const arguments - changing the behaviour
> > of libxlu_cfg_get_string() would cause more complexity than it saves.
> > 
> > Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
> 
> About to bail for the day, but I guess this (untested) patch makes sense
> on top of this?
Yes, very much so
> Avoids c_info->name being a string literal as well. Although I wonder if
> maybe domain configurations with no name could just be rejected as
> invalid? Having a whole bunch of domains called "test"
doesn''t seem
> likely to be what anyone wants...
Yeah, that looks suspect to me too.

Gianni


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

On Wed, 2010-09-08 at 17:46 +0100, Ian Campbell wrote:
> On Wed, 2010-09-08 at 17:31 +0100, Gianni Tedesco wrote:
> > 
> >          /* then process config related to dm */
> > -        if (!xlu_cfg_get_string (config, "device_model",
&buf))
> > -            dm_info->device_model = strdup(buf);
> > +        !xlu_cfg_replace_string (config, "device_model",
> > &dm_info->device_model); 
> 
> Stray ! at the start of the new line?
Indeed.

Jackson, I can re-post this or probably easier for you to fix up
manually before committing?

Gianni


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

Ian Campbell writes ("Re: [Xen-devel] [PATCH,v2]: xl: don''t free
string literals"):
> About to bail for the day, but I guess this (untested) patch makes sense
> on top of this?
...
> Subject: xl: use xlu_cfg_replace_string in a few more places.
Yes, I have applied it.
> Avoids c_info->name being a string literal as well. Although I wonder if
> maybe domain configurations with no name could just be rejected as
> invalid? Having a whole bunch of domains called "test"
doesn''t seem
> likely to be what anyone wants...
Yes.  Really we should be rejecting duplicate names.

Ian.

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