[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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