Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/landlock: EXPECT_EQ before reading thread output in restrict()
--
commit 85f196c7c04b9c3b9c11f5b1ad1ee130ba7d4079
Author: Günther Noack [off-list ref]
selftests/landlock: Use EXPECT for closing FDs and stopping threads
This commit changes ASSERT_EQ to EXPECT_EQ across Landlock selftests when
closing file descriptors, cancelling or joining threads whose results are
unused, and cleaning up at the end of named_pipe_ioctl.
quoted hunk ↗ jump to hunk
diff --git a/tools/testing/selftests/landlock/tsync_test.c b/tools/testing/selftests/landlock/tsync_test.c
index 2b53596c986e0..aff973ba4735b 100644
--- a/tools/testing/selftests/landlock/tsync_test.c
+++ b/tools/testing/selftests/landlock/tsync_test.c
@@ -152,10 +152,10 @@ TEST_F(multi_threaded, restrict)
EXPECT_EQ(variant->expected_no_new_privs,
prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0));
- ASSERT_EQ(0, pthread_cancel(t1));
- ASSERT_EQ(0, pthread_cancel(t2));
- ASSERT_EQ(0, pthread_join(t1, NULL));
- ASSERT_EQ(0, pthread_join(t2, NULL));
+ EXPECT_EQ(0, pthread_cancel(t1));
+ EXPECT_EQ(0, pthread_cancel(t2));
+ EXPECT_EQ(0, pthread_join(t1, NULL));
+ EXPECT_EQ(0, pthread_join(t2, NULL));
/* Checks the no_new_privs state of the sibling threads. */
EXPECT_EQ(variant->expected_no_new_privs, no_new_privs1);
[Severity: Medium]
In TEST_F(multi_threaded, restrict), no_new_privs1 and no_new_privs2 are
declared uninitialized on the stack and passed to the idle() threads:
tools/testing/selftests/landlock/tsync_test.c:restrict() {
...
bool no_new_privs1, no_new_privs2;
ASSERT_EQ(0, pthread_create(&t1, NULL, idle, &no_new_privs1));
ASSERT_EQ(0, pthread_create(&t2, NULL, idle, &no_new_privs2));
...
}
In idle(), those variables are only populated when the cancellation cleanup
handler store_no_new_privs() runs upon cancellation:
tools/testing/selftests/landlock/tsync_test.c:idle() {
pthread_cleanup_push(store_no_new_privs, data);
while (true)
sleep(1);
pthread_cleanup_pop(1);
}
Since the subsequent checks on no_new_privs1 and no_new_privs2 depend on
pthread_cancel() and pthread_join() succeeding to populate and synchronize
those values, should pthread_cancel() and pthread_join() remain ASSERT_EQ
here so that a failure does not fall through into reading uninitialized
stack variables?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-selftests-refactor-v2-0-c52f23f7ce33@gmail.com?part=26