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; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-07 18:02 UTC
[Xen-devel] Re: [PATCH]: xl: don''t free string literals
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com lists.xensource.com/xen-devel
Gianni Tedesco
2010-Sep-08 12:13 UTC
[Xen-devel] Re: [PATCH]: xl: don''t free string literals
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com lists.xensource.com/xen-devel