[PATCH] xkb: Fix use of uninitalised memory upon second keyboard init

[Benjamin Close]

On 27/02/2009 10:57 AM, Dan Nicholson wrote:
> On Thu, Feb 26, 2009 at 04:22:56PM +1030, Benjamin Close wrote:
>    
>> When allocating a second keyboard structure xkbGetRulesDflt
>> is called to get the defaults for rmlvo.
>>
>> With the second keyboard instance these defaults
>> were the values previously allocated in the first call to
>> XkbSetRulesDflt; rmlvo is then assigned this value.
>>
>> rmlvo is then passed into InitKeyboardDeviceStruct which in turn
>> calls xkbSetRulesDflt. xkbSetRulesDflts did:
>>
>>      if( xkbRulesDflt )
>>           _XkbFree(XkbRulesDflt);
>>           XkbRulesDflt= (rmlvo->rules?_XkbDupString(rmlvo->rules):NULL);
>>
>> Problem was by freeing XkbRulesDflt, rmlvo->rules was also freed
>> hence the dup returned bogus data.
>>
>> Fix this problem for both the Dflts and the Used cases.
>>
>> Signed-off-by: Benjamin Close<Benjamin.Close at clearchain.com>
>>      
>
> Here's what I had in mind. It doesn't fix the case where the caller can
> free XKB internal data after calling XkbGetRulesDflts.
>
> --
> Dan
>
> > From 27661caafa577d12380d7af1cc9101531fed3ee7 Mon Sep 17 00:00:00 2001
> From: Dan Nicholson<dbn.lists at gmail.com>
> Date: Thu, 26 Feb 2009 16:14:01 -0800
> Subject: [PATCH] xkb: Don't free strings the caller points to
>
> Since XkbGetRulesDflts fills out a XkbRMLVOSet with pointers to internal
> state, XkbSetRulesUsed and XkbSetRulesDflts need to be careful that they
> do not subsequently free these pointers. Among other things, it means
> the following _XkbDupString will copy garbage.
>
> It might be better if XkbGetRulesDflts duped the internal strings for
> the caller.
>
> Signed-off-by: Dan Nicholson<dbn.lists at gmail.com>
> ---
>   xkb/xkbInit.c |   20 ++++++++++----------
>   1 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/xkb/xkbInit.c b/xkb/xkbInit.c
> index 1f5f8dc..cee2532 100644
> --- a/xkb/xkbInit.c
> +++ b/xkb/xkbInit.c
> @@ -193,19 +193,19 @@ char *			pval;
>   static void
>   XkbSetRulesUsed(XkbRMLVOSet *rmlvo)
>   {
> -    if (XkbRulesUsed)
> +    if (XkbRulesUsed != rmlvo->rules)
>           _XkbFree(XkbRulesUsed);
>       XkbRulesUsed= (rmlvo->rules?_XkbDupString(rmlvo->rules):NULL);
> -    if (XkbModelUsed)
> +    if (XkbModelUsed != rmlvo->model)
>   	_XkbFree(XkbModelUsed);
>       XkbModelUsed= (rmlvo->model?_XkbDupString(rmlvo->model):NULL);
> -    if (XkbLayoutUsed)
> +    if (XkbLayoutUsed != rmlvo->layout)
>   	_XkbFree(XkbLayoutUsed);
>       XkbLayoutUsed= (rmlvo->layout?_XkbDupString(rmlvo->layout):NULL);
> -    if (XkbVariantUsed)
> +    if (XkbVariantUsed != rmlvo->variant)
>   	_XkbFree(XkbVariantUsed);
>       XkbVariantUsed= (rmlvo->variant?_XkbDupString(rmlvo->variant):NULL);
> -    if (XkbOptionsUsed)
> +    if (XkbOptionsUsed != rmlvo->options)
>   	_XkbFree(XkbOptionsUsed);
>       XkbOptionsUsed= (rmlvo->options?_XkbDupString(rmlvo->options):NULL);
>       if (XkbWantRulesProp)
> @@ -217,27 +217,27 @@ void
>   XkbSetRulesDflts(XkbRMLVOSet *rmlvo)
>   {
>       if (rmlvo->rules) {
> -        if (XkbRulesDflt)
> +        if (XkbRulesDflt != rmlvo->rules)
>   	    _XkbFree(XkbRulesDflt);
>           XkbRulesDflt= _XkbDupString(rmlvo->rules);
>       }
>       if (rmlvo->model) {
> -	if (XkbModelDflt)
> +        if (XkbModelDflt != rmlvo->model)
>   	    _XkbFree(XkbModelDflt);
>   	XkbModelDflt= _XkbDupString(rmlvo->model);
>       }
>       if (rmlvo->layout) {
> -	if (XkbLayoutDflt)
> +        if (XkbLayoutDflt != rmlvo->layout)
>   	    _XkbFree(XkbLayoutDflt);
>   	XkbLayoutDflt= _XkbDupString(rmlvo->layout);
>       }
>       if (rmlvo->variant) {
> -	if (XkbVariantDflt)
> +        if (XkbVariantDflt != rmlvo->variant)
>   	    _XkbFree(XkbVariantDflt);
>   	XkbVariantDflt= _XkbDupString(rmlvo->variant);
>       }
>       if (rmlvo->options) {
> -	if (XkbOptionsDflt)
> +        if (XkbOptionsDflt != rmlvo->options)
>   	    _XkbFree(XkbOptionsDflt);
>   	XkbOptionsDflt= _XkbDupString(rmlvo->options);
>       }
>    
Hi Dan,
     What's wrong with the original fix? It has no leaks and doesn't 
allow the caller to free internal xkb info?

Cheers,
     Benjamin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.x.org/archives/xorg-devel/attachments/20090227/fcbd5c74/attachment.html