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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home