On Thu, Feb 26, 2009 at 6:53 PM, Benjamin Close
<[email protected]> wrote:
> 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 <[email protected]>
>
>
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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?
It can free the caller's data without them knowing.
XkbRMLVOSet rmlvo;
XkbGetRulesDflts(&rmlvo);
InitKeyboardDeviceStruct(dev, &rmlvo, NULL, NULL);
LogMessage("The default rules are now %s\n", rmlvo.rules);
Whoops, rmlvo.rules is now garbage, having been freed by
XkbSetRulesUsed. Maybe we just accept that you don't play with the
RMLVOSet if it came from GetRulesDflts. But I guess the old rules are
leaked if the caller doesn't free the components, so I guess your
patch is probably better unless GetRulesDflts is reworked.
--
Dan
_______________________________________________
xorg-devel mailing list
[email protected]
http://lists.x.org/mailman/listinfo/xorg-devel
