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