[vlc-devel] [PATCH] control/globalhotkeys: xcb: fix p_sys->p_map population

[Filip Roséen]

Hi,

*Hugo Beauzée-Luyssen* just pointed out that the commit message in the
patch which this email is a reply to contains an extremely weird typo.

The first sentence is supposed to read *"As described in **#18836**,
[...]"*, not *"#17747"* (which isn't even remotely related to what
this patch is set out to address).

The patch has now been applied with the typo fixed, but I deeply
apologize for any confusion that it may have caused.

Best Regards,\
Filip

On 2017-03-18 18:00, Filip Roséen wrote:

> As described in #17747, certain configurations end up causing a
> double-free in xcb.c:Close due to the same value returned from
> xcb_key_symbols_get_keycode appearing multiple times in p_sys->p_map.
> 
> The code responsible for clean-up assumes that every value refers to a
> separate allocated resource, whereas Mapping potentially populates
> p_sys->p_map with the same value several times.
> 
> These changes make sure that initialization vs clean-up is in harmony.
> 
> fixes: #18136
> 
> --
> 
> A side-effect of these changes is that the we are allocating more
> resources than before. There are two alternative fixes (each suffering
> from added code complexity):
> 
>  1. store all return-values xcb_key_symbols_get_keycode in a separate
>     array (with unique entries), so that clean-up can use this array
>     instead of looping through p_sys->p_map.
> 
>  2. As duplicate entries in p_sys->p_map in terms of ->p_keys are
>     guaranteed to be seqential, one could also update the clean-up loop
>     so that it does not free the about-to-be-freed value if it compares
>     equal to the previous item.
> 
> It is important to note that xcb_keysym_t is (at least currently)
> typedef'd to "uint8_t", meaning that even though we in theory allocate
> 3 times more memory at the affected code-path - the real impact on
> memory usage is very small.
> 
> Best Regards,\
> Filip Roséen
> ---
>  modules/control/globalhotkeys/xcb.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/modules/control/globalhotkeys/xcb.c b/modules/control/globalhotkeys/xcb.c
> index 42fb9f4854..f083b81336 100644
> --- a/modules/control/globalhotkeys/xcb.c
> +++ b/modules/control/globalhotkeys/xcb.c
> @@ -308,11 +308,6 @@ static bool Mapping( intf_thread_t *p_intf )
>          if( i_vlc_key == KEY_UNSET )
>              continue;
>  
> -        xcb_keycode_t *p_keys = xcb_key_symbols_get_keycode(
> -                p_sys->p_symbols, GetX11Key( i_vlc_key & ~KEY_MODIFIER ) );
> -        if( !p_keys )
> -            continue;
> -
>          const unsigned i_modifier = GetX11Modifier( p_sys->p_connection,
>                  p_sys->p_symbols, i_vlc_key & KEY_MODIFIER );
>  
> @@ -325,6 +320,12 @@ static bool Mapping( intf_thread_t *p_intf )
>              if( i != 0 && i_ignored == 0)
>                  continue;
>  
> +            xcb_keycode_t *p_keys = xcb_key_symbols_get_keycode(
> +                p_sys->p_symbols, GetX11Key( i_vlc_key & ~KEY_MODIFIER ) );
> +
> +            if( !p_keys )
> +                break;
> +
>              hotkey_mapping_t *p_map = realloc( p_sys->p_map,
>                                sizeof(*p_sys->p_map) * (p_sys->i_map+1) );
>              if( !p_map )
> -- 
> 2.12.0
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170320/ed1d1a4c/attachment.html>