* [PATCH]: xl: don't free string literals
@ 2010-09-06 12:17 Gianni Tedesco
2010-09-07 18:02 ` Ian Jackson
0 siblings, 1 reply; 3+ messages in thread
From: Gianni Tedesco @ 2010-09-06 12:17 UTC (permalink / raw)
To: Xen Devel; +Cc: Ian Jackson, Stefano Stabellini
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 the initialisers in init_dm_info() and unconditionally free them
before assigning non-default values to prevent their leakage.
Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
diff -r 5f53805b349e -r e954e6b6d311 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Fri Sep 03 18:44:49 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c Mon Sep 06 13:13:46 2010 +0100
@@ -294,7 +294,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;
@@ -302,7 +302,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;
@@ -310,7 +310,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;
@@ -1019,38 +1019,54 @@ 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))
+ if (!xlu_cfg_get_string (config, "device_model", &buf)) {
+ free(dm_info->device_model);
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))
dm_info->vnc = l;
- if (!xlu_cfg_get_string (config, "vnclisten", &buf))
+ if (!xlu_cfg_get_string (config, "vnclisten", &buf)) {
+ free(dm_info->vnclisten);
dm_info->vnclisten = strdup(buf);
- if (!xlu_cfg_get_string (config, "vncpasswd", &buf))
+ }
+ if (!xlu_cfg_get_string (config, "vncpasswd", &buf)) {
+ free(dm_info->vncpasswd);
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))
dm_info->vncunused = l;
- if (!xlu_cfg_get_string (config, "keymap", &buf))
+ if (!xlu_cfg_get_string (config, "keymap", &buf)) {
+ free(dm_info->keymap);
dm_info->keymap = strdup(buf);
+ }
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))
+ if (!xlu_cfg_get_string (config, "serial", &buf)) {
+ free(dm_info->serial);
dm_info->serial = strdup(buf);
- if (!xlu_cfg_get_string (config, "boot", &buf))
+ }
+ if (!xlu_cfg_get_string (config, "boot", &buf)) {
+ free(dm_info->boot);
dm_info->boot = strdup(buf);
+ }
if (!xlu_cfg_get_long (config, "usb", &l))
dm_info->usb = l;
- if (!xlu_cfg_get_string (config, "usbdevice", &buf))
+ if (!xlu_cfg_get_string (config, "usbdevice", &buf)) {
+ free(dm_info->usbdevice);
dm_info->usbdevice = strdup(buf);
- if (!xlu_cfg_get_string (config, "soundhw", &buf))
+ }
+ if (!xlu_cfg_get_string (config, "soundhw", &buf)) {
+ free(dm_info->soundhw);
dm_info->soundhw = strdup(buf);
+ }
if (!xlu_cfg_get_long (config, "xen_platform_pci", &l))
dm_info->xen_platform_pci = l;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH]: xl: don't free string literals
2010-09-06 12:17 [PATCH]: xl: don't free string literals Gianni Tedesco
@ 2010-09-07 18:02 ` Ian Jackson
2010-09-08 12:13 ` Gianni Tedesco
0 siblings, 1 reply; 3+ messages in thread
From: Ian Jackson @ 2010-09-07 18:02 UTC (permalink / raw)
To: Gianni Tedesco; +Cc: Xen Devel, Stefano Stabellini
Gianni Tedesco writes ("[PATCH]: 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 the initialisers in init_dm_info() and unconditionally free them
> before assigning non-default values to prevent their leakage.
Can't we replace the dozen copies of this
> - if (!xlu_cfg_get_string (config, "device_model", &buf))
> + if (!xlu_cfg_get_string (config, "device_model", &buf)) {
> + free(dm_info->device_model);
> dm_info->device_model = strdup(buf);
> + }
with something like
> - if (!xlu_cfg_get_string (config, "device_model", &buf))
> + if (!xlu_cfg_get_string_mallocd (config, "device_model", &buf)) {
or whatever you want to call the function ? Or just change the
definition of xlu_cfg_get_string and declare that it always frees its
argument if it's not 0 to start with.
Ian.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH]: xl: don't free string literals
2010-09-07 18:02 ` Ian Jackson
@ 2010-09-08 12:13 ` Gianni Tedesco
0 siblings, 0 replies; 3+ messages in thread
From: Gianni Tedesco @ 2010-09-08 12:13 UTC (permalink / raw)
To: Ian Jackson; +Cc: Xen Devel, Stefano Stabellini
On Tue, 2010-09-07 at 19:02 +0100, Ian Jackson wrote:
> Gianni Tedesco writes ("[PATCH]: 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 the initialisers in init_dm_info() and unconditionally free them
> > before assigning non-default values to prevent their leakage.
>
> Can't we replace the dozen copies of this
>
> > - if (!xlu_cfg_get_string (config, "device_model", &buf))
> > + if (!xlu_cfg_get_string (config, "device_model", &buf)) {
> > + free(dm_info->device_model);
> > dm_info->device_model = strdup(buf);
> > + }
>
> with something like
>
> > - if (!xlu_cfg_get_string (config, "device_model", &buf))
> > + if (!xlu_cfg_get_string_mallocd (config, "device_model", &buf)) {
>
> or whatever you want to call the function ? Or just change the
> definition of xlu_cfg_get_string and declare that it always frees its
> argument if it's not 0 to start with.
>
> Ian.
Absolutely, I think I'll just change xlu_cfg_get_string() everywhere.
Don't see a need to have two versions of a function and a non-obvious
rule about which to use where. Will hack that up and re-send later.
Gianni
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-09-08 12:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-06 12:17 [PATCH]: xl: don't free string literals Gianni Tedesco
2010-09-07 18:02 ` Ian Jackson
2010-09-08 12:13 ` Gianni Tedesco
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.