Fix ioctl_tree_execute() ret type and initialization

The umockdev-vala testcase randomly fails because the ioctl() testcase checks the return value pointed to by "ret" in ioctl_tree_execute() which might have stale data as it wasn't initialized. Fix it by always initializing the return code, forcing all code paths to provide a valid return code address, and fixing the VAPI declaration (it's purely an out argument). Co-Authored-By: Martin Pitt <[email protected]> https://bugs.debian.org/1091619
martinpitt · Dec 30, 2024 · 42b2af1 · 42b2af1
1 parent ba63ef8
commit 42b2af1
Show file tree

Hide file tree

Showing 5 changed files with 12 additions and 10 deletions.
diff --git a/src/ioctl_tree.c b/src/ioctl_tree.c
@@ -357,6 +357,10 @@ ioctl_tree_execute(ioctl_tree * tree, ioctl_tree * last, IOCTL_REQUEST_TYPE id,
     ioctl_tree *i;
     int r, handled;
 
+    /* initialize return code */
+    assert(ret != NULL);
+    *ret = -1;
+
     DBG(DBG_IOCTL_TREE, "ioctl_tree_execute ioctl %X\n", (unsigned) id);
 
     t = ioctl_type_get_by_id(id);
@@ -369,8 +373,6 @@ ioctl_tree_execute(ioctl_tree * tree, ioctl_tree * last, IOCTL_REQUEST_TYPE id,
 	DBG(DBG_IOCTL_TREE, "  ioctl_tree_execute: stateless\n");
 	if (t->execute(NULL, id, arg, &r))
 	    *ret = r;
-	else
-	    *ret = -1;
 	return last;
     }
 

diff --git a/src/ioctl_tree.vapi b/src/ioctl_tree.vapi
@@ -10,7 +10,7 @@ namespace IoctlTree {
 
       [ReturnsModifiedPointer]
       public void insert(owned Tree node);
-      public void* execute(void* last, ulong id, void* addr, ref int ret);
+      public void* execute(void* last, ulong id, void* addr, out int ret);
       [CCode (instance_pos = -1)]
       public void write(Posix.FILE f);
 

diff --git a/src/umockdev-ioctl.vala b/src/umockdev-ioctl.vala
@@ -572,7 +572,7 @@ public class IoctlClient : GLib.Object {
             IoctlData? data = null;
             ulong size = IoctlTree.data_size_by_id(_request);
             ulong type = (_request >> Ioctl._IOC_TYPESHIFT) & ((1 << Ioctl._IOC_TYPEBITS) - 1);
-            int ret = -1;
+            int ret;
             int my_errno;
 
             try {
@@ -590,7 +590,7 @@ public class IoctlClient : GLib.Object {
             } else {
                 Posix.errno = Posix.ENOTTY;
             }
-            tree.execute(null, _request, *(void**) _arg.data, ref ret);
+            tree.execute(null, _request, *(void**) _arg.data, out ret);
             my_errno = Posix.errno;
             Posix.errno = 0;
 
@@ -897,7 +897,7 @@ internal class IoctlTreeHandler : IoctlBase {
         ulong request = client.request;
         ulong size = IoctlTree.data_size_by_id(request);
         ulong type = (request >> Ioctl._IOC_TYPESHIFT) & ((1 << Ioctl._IOC_TYPEBITS) - 1);
-        int ret = -1;
+        int ret;
         int my_errno;
 
         if (tree == null) {
@@ -938,7 +938,7 @@ internal class IoctlTreeHandler : IoctlBase {
         } else {
             Posix.errno = Posix.ENOTTY;
         }
-        last = tree.execute(last, request, *(void**) client.arg.data, ref ret);
+        last = tree.execute(last, request, *(void**) client.arg.data, out ret);
         my_errno = Posix.errno;
         Posix.errno = 0;
         if (last != null)

diff --git a/tests/test-ioctl-tree.c b/tests/test-ioctl-tree.c
@@ -554,9 +554,9 @@ t_evdev(void)
     g_assert(memcmp(&bits_query, "\0\0\0\0\xAA\xAA\xAA\xAA", 8) == 0);
 
     /* undefined for other ev type */
-    g_assert(!ioctl_tree_execute(tree, NULL, EVIOCGBIT(EV_REL, sizeof(synbits)), &bits_query, NULL));
+    g_assert(!ioctl_tree_execute(tree, NULL, EVIOCGBIT(EV_REL, sizeof(synbits)), &bits_query, &ret));
     /* undefined for other length */
-    g_assert(!ioctl_tree_execute(tree, NULL, EVIOCGBIT(EV_KEY, 4), &bits_query, NULL));
+    g_assert(!ioctl_tree_execute(tree, NULL, EVIOCGBIT(EV_KEY, 4), &bits_query, &ret));
 
     ioctl_tree_free(tree);
 }

diff --git a/tests/test-umockdev-vala.vala b/tests/test-umockdev-vala.vala
@@ -271,7 +271,7 @@ E: SUBSYSTEM=usb
   assert_cmpint (Posix.ioctl (fd, Ioctl.USBDEVFS_CONNECTINFO, ref ci), CompareOperator.EQ, -1);
   // usually ENOTTY, but seem to be EINVAL
   assert_cmpint (Posix.errno, CompareOperator.GE, 22);
-  errno = 0;
+  Posix.errno = 0;
 
   // unknown ioctls don't work on an emulated device
   assert_cmpint (Posix.ioctl (fd, Ioctl.TIOCSBRK, 0), CompareOperator.EQ, -1);