Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ioctl_tree_execute() ret type and initialization #261

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

hdeller
Copy link
Contributor

@hdeller hdeller commented Dec 28, 2024

The umockdev-vala testcase randomly fails on some platforms (hppa, sparc64, powerpc, see:[1][2][3]) 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 properly initializing the return code.
Additionally force all code paths to provide a valid return code address.

Tested on hppa-linux and armhf-linux.

[1] https://buildd.debian.org/status/fetch.php?pkg=umockdev&arch=hppa&ver=0.19.0-1&stamp=1735373270&raw=0
[2] https://buildd.debian.org/status/fetch.php?pkg=umockdev&arch=powerpc&ver=0.19.0-1&stamp=1735375553&raw=0
[3] https://buildd.debian.org/status/fetch.php?pkg=umockdev&arch=sparc64&ver=0.19.0-1&stamp=1735380455&raw=0

Copy link
Owner

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this! Do you want to fix it up yourself, or want me to take over?

src/ioctl_tree.c Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Outdated Show resolved Hide resolved
@hdeller
Copy link
Contributor Author

hdeller commented Dec 29, 2024

Martin, I'm happy that you take over and fix it the way you want it.
Thanks!

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 martinpitt changed the title Fix umockdev-vala testcase Fix ioctl_tree_execute() ret type and initialization Dec 30, 2024
@martinpitt
Copy link
Owner

Thanks @hdeller ! I fixed the points above, and also cleaned up the commit message a bit.

@martinpitt
Copy link
Owner

Thanks @hdeller ! I fixed the points above, and also cleaned up the commit message a bit. Before I do a new release, let's discuss/sort out #262

@martinpitt martinpitt merged commit 42b2af1 into martinpitt:main Dec 30, 2024
23 checks passed
@hdeller
Copy link
Contributor Author

hdeller commented Dec 30, 2024

problem still happens on hppa build:
DEBUG: umockdev.vala:1414: parsing device description for /devices/mycam
DEBUG: umockdev.vala:1492: creating device /devices/mycam (subsystem usb)
DEBUG: umockdev.vala:1538: create_node_for_device: creating file device /tmp/umockdev.SLZ0Z2/dev/001
DEBUG: umockdev.vala:832: umockdev_testbed_uevent: sending uevent add for device /sys/devices/mycam
**
ERROR:../tests/test-umockdev-vala.vala:284:t_usbfs_ioctl_static: assertion failed (ioctl (fd2, TIOCSBRK, 0) == 0): (-1 == 0)
not ok /umockdev-testbed-vala/usbfs_ioctl_static - ERROR:../tests/test-umockdev-vala.vala:284:t_usbfs_ioctl_static: assertion failed (ioctl (fd2, TIOCSBRK, 0) == 0): (-1 == 0)

@hdeller
Copy link
Contributor Author

hdeller commented Dec 30, 2024

With this patch testcase succeeds on hppa and s390x:

diff --git a/tests/test-umockdev-vala.vala b/tests/test-umockdev-vala.vala
index 0d4add0..9cd88e2 100644
--- a/tests/test-umockdev-vala.vala
+++ b/tests/test-umockdev-vala.vala
@@ -281,8 +281,8 @@ E: SUBSYSTEM=usb
   // unknown ioctls do work on non-emulated devices
   int fd2 = Posix.open ("/dev/tty", Posix.O_RDWR, 0);
   if (fd2 > 0) {
-      assert_cmpint (Posix.ioctl (fd2, Ioctl.TIOCSBRK, 0), CompareOperator.EQ, 0);
-      assert_cmpint (Posix.errno, CompareOperator.EQ, 0);
+      assert_cmpint (Posix.ioctl (fd2, Ioctl.TIOCSBRK, 0), CompareOperator.EQ, -1);
+      assert_cmpint (Posix.errno, CompareOperator.EQ, Posix.ENOTTY);
       Posix.close (fd2);
   }

Update:
The test is flaky.... Sometimes the code path isn't executed on s390x.
Posix.open ("/dev/tty") often returns -1 on s390x, which is why the part inside the braces is never executed.
Sometimes it returns file-descriptor 19 and then the check in the braces fail.
Seems to be related to how many tests run in parallel.

UPDATE:
I opened pull request #263 to follow up on this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants