Geis

Merge lp:~bregma/geis/lp-754135 into lp:geis

  1. lp-754135
  2. Merge into trunk
Proposed by Stephen M. Webb
Status: Merged
Merged at revision: 135
Proposed branch: lp:~bregma/geis/lp-754135
Merge into: lp:geis
Diff against target: 287 lines (+183/-9)
5 files modified
ChangeLog (+9/-0)
libutouch-geis/geis_v1.c (+9/-9)
testsuite/geis1/Makefile.am (+1/-0)
testsuite/geis1/check_geis1_api.c (+2/-0)
testsuite/geis1/check_subscription.c (+162/-0)
To merge this branch: bzr merge lp:~bregma/geis/lp-754135
Reviewer Review Type Date Requested Status
Henrik Rydberg (community) Approve
Review via email: mp+57102@code.launchpad.net

Description of the change

Fixes a persistence problem that may occur with callbacks (LP: #754135).

To post a comment you must log in.
Revision history for this message
Henrik Rydberg (rydberg) wrote :

Fix is minimal, test case looks relevant, although I do not see how it tests the duplicate instance problem?

Revision history for this message
Stephen M. Webb (bregma) wrote :

There's a duplicate instance problem?

This fix is for the case where a pointer to a variable with automatic storage duration is passed to geis_subscribe() and that variable goes out of scope, leaving a dangling pointer. That was the cause of the problem reported in lp:754135.

Dereferencing dangling pointers results in undefined behaviour: there is no reliable way to reproduce the problem within the C language itself. The test case is using inside knowledge of the GCC runtime implementation on at least three of the most common architectures (i386, amd64, arm) to reliably force a segfault under the reported circumstances by setting the area previously used by the automatic variable to zeros. The test case does in fact segfault on tested hardware without the rest of the patch but runs cleanly with it.

An additional standalone program to demonstrate the problem was supplied in the bug by the reporter.

Revision history for this message
Henrik Rydberg (rydberg) wrote :

The problem you describe was the problem i meant. Another way to test would be to use two instances of geis, reusing the callback struct but change functions in between. This way the problem could be tested without a crash. Either way is fine, especially with your nice explanation.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'ChangeLog'
--- ChangeLog 2011-04-05 18:04:36 +0000
+++ ChangeLog 2011-04-11 03:23:41 +0000
@@ -1,3 +1,12 @@
12011-04-10 Stephen M. Webb <stephen.webb@canonical.com>
2
3 Repaired a callback persistence problem (LP: #754135).
4
5 * testsuite/geis1/check_subscription.c: new test suite
6 * libutouch-geis/geis_v1.c: saved a copy of the callback tables
7 * testsuite/geis1/Makefile.am: included new test suite
8 * testsuite/geis1/check_geis1_api.c: ran new test suite
9
12011-04-05 Stephen M. Webb <stephen.webb@canonical.com>102011-04-05 Stephen M. Webb <stephen.webb@canonical.com>
211
3 Fixed invalid pointer dereference on XI initialization failure in XCB back12 Fixed invalid pointer dereference on XI initialization failure in XCB back
413
=== modified file 'libutouch-geis/geis_v1.c'
--- libutouch-geis/geis_v1.c 2011-03-25 19:33:18 +0000
+++ libutouch-geis/geis_v1.c 2011-04-11 03:23:41 +0000
@@ -37,9 +37,9 @@
37{37{
38 GeisSubscription subscription;38 GeisSubscription subscription;
39 GeisFilter window_filter;39 GeisFilter window_filter;
40 GeisInputFuncs *input_funcs;40 GeisInputFuncs input_funcs;
41 GeisPointer input_context;41 GeisPointer input_context;
42 GeisGestureFuncs *gesture_funcs;42 GeisGestureFuncs gesture_funcs;
43 GeisPointer gesture_cookie;43 GeisPointer gesture_cookie;
44};44};
4545
@@ -343,19 +343,19 @@
343 switch (geis_event_type(event))343 switch (geis_event_type(event))
344 {344 {
345 case GEIS_EVENT_GESTURE_BEGIN:345 case GEIS_EVENT_GESTURE_BEGIN:
346 v1_instance->gesture_funcs->start(v1_instance->gesture_cookie,346 v1_instance->gesture_funcs.start(v1_instance->gesture_cookie,
347 class_id,347 class_id,
348 geis_frame_id(frame),348 geis_frame_id(frame),
349 attr_count, attrs);349 attr_count, attrs);
350 break;350 break;
351 case GEIS_EVENT_GESTURE_UPDATE:351 case GEIS_EVENT_GESTURE_UPDATE:
352 v1_instance->gesture_funcs->update(v1_instance->gesture_cookie,352 v1_instance->gesture_funcs.update(v1_instance->gesture_cookie,
353 class_id,353 class_id,
354 geis_frame_id(frame),354 geis_frame_id(frame),
355 attr_count, attrs);355 attr_count, attrs);
356 break;356 break;
357 case GEIS_EVENT_GESTURE_END:357 case GEIS_EVENT_GESTURE_END:
358 v1_instance->gesture_funcs->finish(v1_instance->gesture_cookie,358 v1_instance->gesture_funcs.finish(v1_instance->gesture_cookie,
359 class_id,359 class_id,
360 geis_frame_id(frame),360 geis_frame_id(frame),
361 attr_count, attrs);361 attr_count, attrs);
@@ -622,7 +622,7 @@
622{622{
623 GeisStatus result = GEIS_UNKNOWN_ERROR;623 GeisStatus result = GEIS_UNKNOWN_ERROR;
624624
625 instance->gesture_funcs = funcs;625 memcpy(&instance->gesture_funcs, funcs, sizeof(GeisGestureFuncs));
626 instance->gesture_cookie = cookie;626 instance->gesture_cookie = cookie;
627627
628 /* pump the geis event queue to force device and class events to be picked up */628 /* pump the geis event queue to force device and class events to be picked up */
@@ -759,12 +759,12 @@
759 switch (geis_event_type(event))759 switch (geis_event_type(event))
760 {760 {
761 case GEIS_EVENT_DEVICE_AVAILABLE:761 case GEIS_EVENT_DEVICE_AVAILABLE:
762 v1_instance->input_funcs->added(v1_instance->input_context,762 v1_instance->input_funcs.added(v1_instance->input_context,
763 device_id,763 device_id,
764 attrs);764 attrs);
765 break;765 break;
766 case GEIS_EVENT_DEVICE_UNAVAILABLE:766 case GEIS_EVENT_DEVICE_UNAVAILABLE:
767 v1_instance->input_funcs->removed(v1_instance->input_context,767 v1_instance->input_funcs.removed(v1_instance->input_context,
768 device_id,768 device_id,
769 attrs);769 attrs);
770 break;770 break;
@@ -787,7 +787,7 @@
787{787{
788 GeisStatus result = GEIS_STATUS_SUCCESS;788 GeisStatus result = GEIS_STATUS_SUCCESS;
789789
790 instance->input_funcs = funcs;790 memcpy(&instance->input_funcs, funcs, sizeof(GeisInputFuncs));
791 instance->input_context = context;791 instance->input_context = context;
792792
793 geis_register_device_callback(g_geis, _v1_input_callback, instance);793 geis_register_device_callback(g_geis, _v1_input_callback, instance);
794794
=== modified file 'testsuite/geis1/Makefile.am'
--- testsuite/geis1/Makefile.am 2011-03-25 19:33:18 +0000
+++ testsuite/geis1/Makefile.am 2011-04-11 03:23:41 +0000
@@ -17,6 +17,7 @@
17 check_instance.c \17 check_instance.c \
18 check_gesture_types.c \18 check_gesture_types.c \
19 check_gesture_attrs.c \19 check_gesture_attrs.c \
20 check_subscription.c \
20 check_geis1_api.c21 check_geis1_api.c
2122
22check_geis1_api_CFLAGS = \23check_geis1_api_CFLAGS = \
2324
=== modified file 'testsuite/geis1/check_geis1_api.c'
--- testsuite/geis1/check_geis1_api.c 2011-03-25 19:33:18 +0000
+++ testsuite/geis1/check_geis1_api.c 2011-04-11 03:23:41 +0000
@@ -23,6 +23,7 @@
23extern Suite *geis1_instance_suite_new();23extern Suite *geis1_instance_suite_new();
24extern Suite *geis1_gesture_types_new();24extern Suite *geis1_gesture_types_new();
25extern Suite *geis1_gesture_attrs_new();25extern Suite *geis1_gesture_attrs_new();
26extern Suite *geis1_subscription_new();
2627
27int28int
28main(int argc CK_ATTRIBUTE_UNUSED, char* argv[] CK_ATTRIBUTE_UNUSED)29main(int argc CK_ATTRIBUTE_UNUSED, char* argv[] CK_ATTRIBUTE_UNUSED)
@@ -34,6 +35,7 @@
34 srunner_add_suite(sr, geis1_instance_suite_new());35 srunner_add_suite(sr, geis1_instance_suite_new());
35 srunner_add_suite(sr, geis1_gesture_types_new());36 srunner_add_suite(sr, geis1_gesture_types_new());
36 srunner_add_suite(sr, geis1_gesture_attrs_new());37 srunner_add_suite(sr, geis1_gesture_attrs_new());
38 srunner_add_suite(sr, geis1_subscription_new());
3739
38 srunner_set_log(sr, "geis1_api.log");40 srunner_set_log(sr, "geis1_api.log");
39 srunner_run_all(sr, CK_NORMAL);41 srunner_run_all(sr, CK_NORMAL);
4042
=== added file 'testsuite/geis1/check_subscription.c'
--- testsuite/geis1/check_subscription.c 1970-01-01 00:00:00 +0000
+++ testsuite/geis1/check_subscription.c 2011-04-11 03:23:41 +0000
@@ -0,0 +1,162 @@
1/**
2 * @file check_subscription.c
3 * @brief Unit tests for GEIS v1 subscriptions
4 *
5 * Copyright 2011 Canonical Ltd.
6 *
7 * This library is free software; you can redistribute it and/or modify it under
8 * the terms of the GNU Lesser General Public License as published by the Free
9 * Software Foundation; either version 3 of the License, or (at your option) any
10 * later version.
11 *
12 * This library is distributed in the hope that it will be useful, but WITHOUT
13 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
14 * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
15 * details.
16 *
17 * You should have received a copy of the GNU Lesser General Public License
18 * along with this program; if not, write to the Free Software Foundation, Inc.,
19 * 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
20 */
21#include <check.h>
22
23#include <geis/geis.h>
24#include <string.h>
25
26
27#define GEIS_TEST_WINDOW geis_win_type_str(Test)
28
29static const char* s_gestures[] = {
30 GEIS_GESTURE_TYPE_DRAG2, GEIS_GESTURE_TYPE_DRAG3,
31 GEIS_GESTURE_TYPE_PINCH2, GEIS_GESTURE_TYPE_PINCH3,
32 GEIS_GESTURE_TYPE_ROTATE2, GEIS_GESTURE_TYPE_ROTATE3,
33 GEIS_GESTURE_TYPE_TAP2, GEIS_GESTURE_TYPE_TAP3, GEIS_GESTURE_TYPE_TAP4,
34 NULL
35};
36
37static void
38gesture_added(void *cookie CK_ATTRIBUTE_UNUSED,
39 GeisGestureType gesture_type CK_ATTRIBUTE_UNUSED,
40 GeisGestureId gesture_id CK_ATTRIBUTE_UNUSED,
41 GeisSize attr_count CK_ATTRIBUTE_UNUSED,
42 GeisGestureAttr *attrs CK_ATTRIBUTE_UNUSED)
43{
44}
45
46static void
47gesture_removed(void *cookie CK_ATTRIBUTE_UNUSED,
48 GeisGestureType gesture_type CK_ATTRIBUTE_UNUSED,
49 GeisGestureId gesture_id CK_ATTRIBUTE_UNUSED,
50 GeisSize attr_count CK_ATTRIBUTE_UNUSED,
51 GeisGestureAttr *attrs CK_ATTRIBUTE_UNUSED)
52{
53}
54
55/*
56 * Checks for special values generated by the mock back end.
57 */
58static void
59gesture_start(void *cookie CK_ATTRIBUTE_UNUSED,
60 GeisGestureType gesture_type CK_ATTRIBUTE_UNUSED,
61 GeisGestureId gesture_id CK_ATTRIBUTE_UNUSED,
62 GeisSize attr_count CK_ATTRIBUTE_UNUSED,
63 GeisGestureAttr *attrs CK_ATTRIBUTE_UNUSED)
64{
65}
66
67static void
68gesture_update(void *cookie CK_ATTRIBUTE_UNUSED,
69 GeisGestureType gesture_type CK_ATTRIBUTE_UNUSED,
70 GeisGestureId gesture_id CK_ATTRIBUTE_UNUSED,
71 GeisSize attr_count CK_ATTRIBUTE_UNUSED,
72 GeisGestureAttr *attrs CK_ATTRIBUTE_UNUSED)
73{
74}
75
76static void
77gesture_finish(void *cookie CK_ATTRIBUTE_UNUSED,
78 GeisGestureType gesture_type CK_ATTRIBUTE_UNUSED,
79 GeisGestureId gesture_id CK_ATTRIBUTE_UNUSED,
80 GeisSize attr_count CK_ATTRIBUTE_UNUSED,
81 GeisGestureAttr *attrs CK_ATTRIBUTE_UNUSED)
82{
83}
84
85
86/* fixtures */
87static GeisInstance g_instance;
88
89/* fixture setup */
90static void
91construct_instance()
92{
93 GeisStatus status;
94 GeisXcbWinInfo x_win_info = {
95 .display_name = NULL,
96 .screenp = NULL,
97 .window_id = 1
98 };
99 GeisWinInfo win_info = { GEIS_TEST_WINDOW, &x_win_info };
100
101 status = geis_init(&win_info, &g_instance);
102}
103
104/* fixture teardown */
105static void
106destroy_instance()
107{
108 geis_finish(g_instance);
109}
110
111
112static void
113construct_subscription()
114{
115 GeisStatus status;
116 GeisGestureFuncs gesture_funcs;
117
118 gesture_funcs.added = gesture_added;
119 gesture_funcs.removed = gesture_removed;
120 gesture_funcs.start = gesture_start;
121 gesture_funcs.update = gesture_update;
122 gesture_funcs.finish = gesture_finish;
123 status = geis_subscribe(g_instance,
124 GEIS_ALL_INPUT_DEVICES,
125 s_gestures,
126 &gesture_funcs,
127 &gesture_funcs);
128}
129
130
131static void
132dispatch_events()
133{
134 GeisGestureFuncs gesture_funcs;
135 memset(&gesture_funcs, 0, sizeof(GeisGestureFuncs));
136 geis_event_dispatch(g_instance);
137}
138
139
140/* regression test for LP: #754135 */
141START_TEST(check_persistence)
142{
143 construct_subscription();
144 dispatch_events();
145}
146END_TEST
147
148
149Suite *
150geis1_subscription_new()
151{
152 Suite *s = suite_create("geis1_subscription");
153 TCase *test;
154
155 test = tcase_create("lp754135");
156 tcase_add_checked_fixture(test, construct_instance, destroy_instance);
157 tcase_add_test(test, check_persistence);
158 suite_add_tcase(s, test);
159
160 return s;
161}
162

Subscribers

People subscribed via source and target branches