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

[Stephen M. Webb]

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

[Henrik Rydberg (rydberg)]

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

[Stephen M. Webb (bregma)]

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.

[Henrik Rydberg (rydberg)]

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

=== modified file 'ChangeLog'
--- ChangeLog	2011-04-05 18:04:36 +0000
+++ ChangeLog	2011-04-11 03:23:41 +0000
@@ -1,3 +1,12 @@
[phone]  Stephen M. Webb  <[e-mail]>
+
+  Repaired a callback persistence problem (LP: #754135).
+
+  * testsuite/geis1/check_subscription.c: new test suite
+  * libutouch-geis/geis_v1.c: saved a copy of the callback tables
+  * testsuite/geis1/Makefile.am: included new test suite
+  * testsuite/geis1/check_geis1_api.c: ran new test suite
+
 2011-04-05  Stephen M. Webb  <[e-mail]>
 
   Fixed invalid pointer dereference on XI initialization failure in XCB back

=== 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 @@
 {
   GeisSubscription  subscription;
   GeisFilter        window_filter;
-  GeisInputFuncs   *input_funcs;
+  GeisInputFuncs    input_funcs;
   GeisPointer       input_context;
-  GeisGestureFuncs *gesture_funcs;
+  GeisGestureFuncs  gesture_funcs;
   GeisPointer       gesture_cookie;
 };
 
@@ -343,19 +343,19 @@
       switch (geis_event_type(event))
       {
 	case GEIS_EVENT_GESTURE_BEGIN:
-	  v1_instance->gesture_funcs->start(v1_instance->gesture_cookie,
+	  v1_instance->gesture_funcs.start(v1_instance->gesture_cookie,
 	                                    class_id,
 	                                    geis_frame_id(frame),
 	                                    attr_count, attrs);
 	  break;
 	case GEIS_EVENT_GESTURE_UPDATE:
-	  v1_instance->gesture_funcs->update(v1_instance->gesture_cookie,
+	  v1_instance->gesture_funcs.update(v1_instance->gesture_cookie,
 	                                     class_id,
 	                                     geis_frame_id(frame),
 	                                     attr_count, attrs);
 	  break;
 	case GEIS_EVENT_GESTURE_END:
-	  v1_instance->gesture_funcs->finish(v1_instance->gesture_cookie,
+	  v1_instance->gesture_funcs.finish(v1_instance->gesture_cookie,
 	                                     class_id,
 	                                     geis_frame_id(frame),
 	                                     attr_count, attrs);
@@ -622,7 +622,7 @@
 {
   GeisStatus result = GEIS_UNKNOWN_ERROR;
 
-  instance->gesture_funcs = funcs;
+  memcpy(&instance->gesture_funcs, funcs, sizeof(GeisGestureFuncs));
   instance->gesture_cookie = cookie;
 
   /* pump the geis event queue to force device and class events to be picked up */
@@ -759,12 +759,12 @@
   switch (geis_event_type(event))
   {
     case GEIS_EVENT_DEVICE_AVAILABLE:
-      v1_instance->input_funcs->added(v1_instance->input_context,
+      v1_instance->input_funcs.added(v1_instance->input_context,
                                       device_id,
                                       attrs);
       break;
     case GEIS_EVENT_DEVICE_UNAVAILABLE:
-      v1_instance->input_funcs->removed(v1_instance->input_context,
+      v1_instance->input_funcs.removed(v1_instance->input_context,
                                         device_id,
                                         attrs);
       break;
@@ -787,7 +787,7 @@
 {
   GeisStatus result = GEIS_STATUS_SUCCESS;
 
-  instance->input_funcs = funcs;
+  memcpy(&instance->input_funcs, funcs, sizeof(GeisInputFuncs));
   instance->input_context = context;
 
   geis_register_device_callback(g_geis, _v1_input_callback, instance);

=== 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 @@
 	check_instance.c \
 	check_gesture_types.c \
 	check_gesture_attrs.c \
+	check_subscription.c \
 	check_geis1_api.c
 
 check_geis1_api_CFLAGS = \

=== 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 @@
 extern Suite *geis1_instance_suite_new();
 extern Suite *geis1_gesture_types_new();
 extern Suite *geis1_gesture_attrs_new();
+extern Suite *geis1_subscription_new();
 
 int
 main(int argc CK_ATTRIBUTE_UNUSED, char* argv[] CK_ATTRIBUTE_UNUSED)
@@ -34,6 +35,7 @@
   srunner_add_suite(sr, geis1_instance_suite_new());
   srunner_add_suite(sr, geis1_gesture_types_new());
   srunner_add_suite(sr, geis1_gesture_attrs_new());
+  srunner_add_suite(sr, geis1_subscription_new());
 
   srunner_set_log(sr, "geis1_api.log");
   srunner_run_all(sr, CK_NORMAL);

=== 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 @@
+/**
+ * @file check_subscription.c
+ * @brief Unit tests for GEIS v1 subscriptions
+ *
+ * Copyright 2011 Canonical Ltd.
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License as published by the Free
+ * Software Foundation; either version 3 of the License, or (at your option) any
+ * later version.
+ *
+ * This library is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+#include <check.h>
+
+#include <geis/geis.h>
+#include <string.h>
+
+
+#define GEIS_TEST_WINDOW geis_win_type_str(Test)
+
+static const char* s_gestures[] = {
+  GEIS_GESTURE_TYPE_DRAG2, GEIS_GESTURE_TYPE_DRAG3,
+  GEIS_GESTURE_TYPE_PINCH2, GEIS_GESTURE_TYPE_PINCH3,
+  GEIS_GESTURE_TYPE_ROTATE2, GEIS_GESTURE_TYPE_ROTATE3,
+  GEIS_GESTURE_TYPE_TAP2, GEIS_GESTURE_TYPE_TAP3, GEIS_GESTURE_TYPE_TAP4,
+  NULL
+};
+
+static void
+gesture_added(void            *cookie CK_ATTRIBUTE_UNUSED,
+              GeisGestureType  gesture_type CK_ATTRIBUTE_UNUSED,
+              GeisGestureId    gesture_id CK_ATTRIBUTE_UNUSED,
+              GeisSize         attr_count CK_ATTRIBUTE_UNUSED,
+              GeisGestureAttr *attrs CK_ATTRIBUTE_UNUSED)
+{
+}
+
+static void
+gesture_removed(void              *cookie CK_ATTRIBUTE_UNUSED,
+                GeisGestureType    gesture_type CK_ATTRIBUTE_UNUSED,
+                GeisGestureId      gesture_id CK_ATTRIBUTE_UNUSED,
+                GeisSize           attr_count CK_ATTRIBUTE_UNUSED,
+                GeisGestureAttr   *attrs CK_ATTRIBUTE_UNUSED)
+{
+}
+
+/*
+ * Checks for special values generated by the mock back end.
+ */
+static void
+gesture_start(void              *cookie CK_ATTRIBUTE_UNUSED,
+              GeisGestureType    gesture_type CK_ATTRIBUTE_UNUSED,
+              GeisGestureId      gesture_id CK_ATTRIBUTE_UNUSED,
+              GeisSize           attr_count CK_ATTRIBUTE_UNUSED,
+              GeisGestureAttr   *attrs CK_ATTRIBUTE_UNUSED)
+{
+}
+
+static void
+gesture_update(void              *cookie CK_ATTRIBUTE_UNUSED,
+               GeisGestureType    gesture_type CK_ATTRIBUTE_UNUSED,
+               GeisGestureId      gesture_id CK_ATTRIBUTE_UNUSED,
+               GeisSize           attr_count CK_ATTRIBUTE_UNUSED,
+               GeisGestureAttr   *attrs CK_ATTRIBUTE_UNUSED)
+{
+}
+
+static void
+gesture_finish(void              *cookie CK_ATTRIBUTE_UNUSED,
+               GeisGestureType    gesture_type CK_ATTRIBUTE_UNUSED,
+               GeisGestureId      gesture_id CK_ATTRIBUTE_UNUSED,
+               GeisSize           attr_count CK_ATTRIBUTE_UNUSED,
+               GeisGestureAttr   *attrs CK_ATTRIBUTE_UNUSED)
+{
+}
+
+
+/* fixtures */
+static GeisInstance g_instance;
+
+/* fixture setup */
+static void
+construct_instance()
+{
+  GeisStatus   status;
+  GeisXcbWinInfo x_win_info = {
+    .display_name  = NULL,
+    .screenp       = NULL,
+    .window_id     = 1
+  };
+  GeisWinInfo  win_info = { GEIS_TEST_WINDOW, &x_win_info };
+
+  status = geis_init(&win_info, &g_instance);
+}
+
+/* fixture teardown */
+static void
+destroy_instance()
+{
+  geis_finish(g_instance);
+}
+
+
+static void
+construct_subscription()
+{
+  GeisStatus status;
+  GeisGestureFuncs gesture_funcs;
+
+  gesture_funcs.added   = gesture_added;
+  gesture_funcs.removed = gesture_removed;
+  gesture_funcs.start   = gesture_start;
+  gesture_funcs.update  = gesture_update;
+  gesture_funcs.finish  = gesture_finish;
+  status = geis_subscribe(g_instance,
+                          GEIS_ALL_INPUT_DEVICES,
+                          s_gestures,
+                          &gesture_funcs,
+                          &gesture_funcs);
+}
+
+
+static void
+dispatch_events()
+{
+  GeisGestureFuncs gesture_funcs;
+  memset(&gesture_funcs, 0, sizeof(GeisGestureFuncs));
+  geis_event_dispatch(g_instance);
+}
+
+
+/* regression test for LP: #754135 */
+START_TEST(check_persistence)
+{
+  construct_subscription();
+  dispatch_events();
+}
+END_TEST
+
+
+Suite *
+geis1_subscription_new()
+{
+  Suite *s = suite_create("geis1_subscription");
+  TCase *test;
+
+  test = tcase_create("lp754135");
+  tcase_add_checked_fixture(test, construct_instance, destroy_instance);
+  tcase_add_test(test, check_persistence);
+  suite_add_tcase(s, test);
+
+  return s;
+}
+