Merge lp:~3v1n0/unity/fix-keyboardutil-crash-920258 into lp:unity

Proposed by Marco Trevisan (Treviño)
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 2515
Proposed branch: lp:~3v1n0/unity/fix-keyboardutil-crash-920258
Merge into: lp:unity
Diff against target: 68 lines (+12/-7)
2 files modified
unity-shared/KeyboardUtil.cpp (+11/-6)
unity-shared/KeyboardUtil.h (+1/-1)
To merge this branch: bzr merge lp:~3v1n0/unity/fix-keyboardutil-crash-920258
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Andrea Azzarone (community) Approve
jenkins (community) continuous-integration Approve
Review via email: mp+113658@code.launchpad.net

Commit message

KeyboardUtil: fix possible crash if XkbGetKeyboard returns null

Also use initializer list

Description of the change

Add safety checks and initializer list to KeyboardUtil

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

typo: FETCH_MAKS

What will happen if it can't get a keyboard?

review: Needs Fixing
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Considering for what it is used here, the Alt+` (Aka Alt+KeyAboveTab) won't work... For sure we should avoid this possible crash where the lower level libraries don't give us the expected result. This still looks to happen on errors.ubuntu.com.

Revision history for this message
Tim Penhey (thumper) wrote :

Marco, what I mean, is if we can't get a keyboard, and avoid the crash that is happening, then what?

Do we still have the keybinding?

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

No, in that case we don't set any keybinding for the Alt+` switcher.

Revision history for this message
jenkins (martin-mrazik+qa) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)
Revision history for this message
Andrea Azzarone (azzar1) wrote :

+1

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote :

Should have * next to type, not name, but apart from that, all good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'unity-shared/KeyboardUtil.cpp'
2--- unity-shared/KeyboardUtil.cpp 2012-06-22 01:38:45 +0000
3+++ unity-shared/KeyboardUtil.cpp 2012-07-05 23:00:27 +0000
4@@ -25,17 +25,18 @@
5
6 namespace unity {
7 namespace ui {
8+namespace {
9+ const unsigned int FETCH_MASK = XkbGBN_KeyNamesMask | XkbGBN_ClientSymbolsMask | XkbGBN_GeometryMask;
10+}
11
12 KeyboardUtil::KeyboardUtil(Display *display)
13 : display_(display)
14-{
15- unsigned int fetch_mask = XkbGBN_KeyNamesMask | XkbGBN_ClientSymbolsMask | XkbGBN_GeometryMask;
16- keyboard_ = XkbGetKeyboard (display, fetch_mask, XkbUseCoreKbd);
17-}
18+ , keyboard_(XkbGetKeyboard(display_, FETCH_MASK, XkbUseCoreKbd))
19+{}
20
21 KeyboardUtil::~KeyboardUtil()
22 {
23- XkbFreeKeyboard (keyboard_, 0, True);
24+ XkbFreeKeyboard(keyboard_, 0, True);
25 }
26
27 bool KeyboardUtil::FindKeyInGeometry(XkbGeometryPtr geo, char *key_name, int& res_section, XkbBoundsRec& res_bounds) const
28@@ -86,6 +87,9 @@
29
30 guint KeyboardUtil::ConvertKeyToKeycode(XkbKeyPtr key) const
31 {
32+ if (!keyboard_)
33+ return 0;
34+
35 int min_code = keyboard_->min_key_code;
36 int max_code = keyboard_->max_key_code;
37
38@@ -94,6 +98,7 @@
39 if (!strncmp(key->name.name, keyboard_->names->keys[i].name, XkbKeyNameLength))
40 return i;
41 }
42+
43 return 0;
44 }
45
46@@ -186,7 +191,7 @@
47
48 int code = XKeysymToKeycode(display_, key_symbol);
49
50- if (!code)
51+ if (!code || !keyboard_)
52 return result;
53
54 if (keyboard_->min_key_code > code || keyboard_->max_key_code < code)
55
56=== modified file 'unity-shared/KeyboardUtil.h'
57--- unity-shared/KeyboardUtil.h 2012-06-20 00:21:25 +0000
58+++ unity-shared/KeyboardUtil.h 2012-07-05 23:00:27 +0000
59@@ -51,8 +51,8 @@
60
61 XkbBoundsRec GetAbsoluteKeyBounds (XkbKeyPtr key, XkbRowPtr row, XkbSectionPtr section, XkbGeometryPtr geo) const;
62
63+ Display *display_;
64 XkbDescPtr keyboard_;
65- Display *display_;
66 };
67
68 }