Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Stack buffer overflow in selftest when reading long thread names. - [Medium] Helper functions return negative error codes on prctl failures, but the test framework incorrectly evaluates these as success. --
commit fe809367165b80380a7910f0d6bf3371b37e1de5 Author: André Almeida <[email protected]> selftests: prctl: Add test for long thread names Add tests for the new interface to set and get long thread names. The kernel should accept the LONG_NAME and returning it accordingly. For the old PR_GET_NAME interface, the kernel should truncate the name up to 16 chars. /proc/<task>/comm should return the same string ad PR_GET_NAME. > diff --git a/tools/testing/selftests/prctl/set-process-name.c > b/tools/testing/selftests/prctl/set-process-name.c > index 3f7b146d36dfe..951319d606561 100644 > --- a/tools/testing/selftests/prctl/set-process-name.c > +++ b/tools/testing/selftests/prctl/set-process-name.c [ ... ] > @@ -82,6 +114,11 @@ TEST(rename_process) { > EXPECT_GE(set_name(CHANGE_NAME), 0); > EXPECT_TRUE(check_is_name_correct(CHANGE_NAME)); > > + EXPECT_GE(set_ext_name(LONG_NAME), 0); > + EXPECT_TRUE(check_is_ext_name_correct(LONG_NAME)); [Severity: Medium] If prctl() fails in check_is_ext_name_correct(), it returns a negative error code. Since EXPECT_TRUE() evaluates any non-zero value as true, won't this cause the test to incorrectly pass on failure? Should the helper function return 0 on failure, or should the test use EXPECT_EQ(1, check_is_ext_name_correct(LONG_NAME)) instead? > + EXPECT_TRUE(check_is_name_correct(LONG_NAME_CAP)); > + EXPECT_TRUE(check_name()); [Severity: Medium] When this test runs, it calls check_name() while the thread name is set to a 33-character string. If the kernel incorrectly fails to truncate the long process name when reading /proc/self/task/.../comm, could this cause a stack buffer overflow? tools/testing/selftests/prctl/set-process-name.c:check_name() { char output[TASK_COMM_LEN] = {}; ... fscanf(fptr, "%s", output); ... } Since fscanf with "%s" is unbounded, a long name surfaced by the kernel will overflow the 16-byte output buffer and crash the test. Should we add a width limit like "%15s" to the fscanf call to prevent this? > + > EXPECT_GE(set_name(EMPTY_NAME), 0); > EXPECT_TRUE(check_is_name_correct(EMPTY_NAME)); > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
