<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi,</p>
<p><em>Hugo Beauzée-Luyssen</em> just pointed out that the commit message in the patch which this email is a reply to contains an extremely weird typo.</p>
<p>The first sentence is supposed to read <em>“As described in <strong>#18836</strong>, […]”</em>, not <em>“#17747”</em> (which isn’t even remotely related to what this patch is set out to address).</p>
<p>The patch has now been applied with the typo fixed, but I deeply apologize for any confusion that it may have caused.</p>
<p>Best Regards,<br />
Filip</p>
<p>On 2017-03-18 18:00, Filip Roséen wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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</code></pre>
</blockquote>
</body>
</html>