This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default
From: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Date: Tue, 31 Aug 2010 17:12:22 +0100
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Tue, 31 Aug 2010 09:13:15 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1283269353.25936.9.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1283269353.25936.9.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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
> +                cfg->filename, set->lineno, n);
> +        return EINVAL;
> +    }
> +    *value_r= l;
> +    return 0;


Xen-devel mailing list