Re: [PATCH] selftests/landlock: Fix screwed up pointers in the scoped_signal_test

["Mickaël Salaün" <mic@digikod.net>]

Applied (with a Fixes tag), thanks!

On Thu, Jul 09, 2026 at 06:43:40PM +0200, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The scoped_signal_test uses pthread_join(..., (void **)&ret)) in
> a couple of places, i.e. the return value of the thread is stored
> in the shape of a "void *" into the memory location of &ret.
> Pointers are 64-bit on modern computers, but the ret variable is
> declared as a simple "enum thread_return" which is only 32 bits.
> So the pthread_join() will overflow the ret variable by 4 byte.
> 
> The problem is very visible on big endian systems like s390x
> where the test is failing: The least significant byte that carries
> the return code of the thread is not written into the ret variable
> here, but somewhere else in the stack frame, so the comparison
> for the right return code is failing here.
> 
> Fix it by getting rid of the enum and defining the THREAD_* constants
> and "ret" variables as proper "void *" pointers. This way we can
> also get rid of some ugly (void *) castings in a couple of spots.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  .../selftests/landlock/scoped_signal_test.c   | 44 +++++++++----------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
> index f24f2c28f62e5..58d25157fe781 100644
> --- a/tools/testing/selftests/landlock/scoped_signal_test.c
> +++ b/tools/testing/selftests/landlock/scoped_signal_test.c
> @@ -249,12 +249,10 @@ TEST_F(scoped_domains, check_access_signal)
>  		_metadata->exit_code = KSFT_FAIL;
>  }
>  
> -enum thread_return {
> -	THREAD_INVALID = 0,
> -	THREAD_SUCCESS = 1,
> -	THREAD_ERROR = 2,
> -	THREAD_TEST_FAILED = 3,
> -};

I just added clang-format markups.

> +#define THREAD_INVALID		((void *)0)
> +#define THREAD_SUCCESS		((void *)1)
> +#define THREAD_ERROR		((void *)2)
> +#define THREAD_TEST_FAILED	((void *)3)
>  
>  static void *thread_sync(void *arg)
>  {
> @@ -262,15 +260,15 @@ static void *thread_sync(void *arg)
>  	char buf;
>  
>  	if (read(pipe_read, &buf, 1) != 1)
> -		return (void *)THREAD_ERROR;
> +		return THREAD_ERROR;
>  
> -	return (void *)THREAD_SUCCESS;
> +	return THREAD_SUCCESS;
>  }
>  
>  TEST(signal_scoping_thread_before)
>  {
>  	pthread_t no_sandbox_thread;
> -	enum thread_return ret = THREAD_INVALID;
> +	void *ret = THREAD_INVALID;
>  	int thread_pipe[2];
>  
>  	drop_caps(_metadata);
> @@ -285,7 +283,7 @@ TEST(signal_scoping_thread_before)
>  	EXPECT_EQ(0, pthread_kill(no_sandbox_thread, 0));
>  	EXPECT_EQ(1, write(thread_pipe[1], ".", 1));
>  
> -	EXPECT_EQ(0, pthread_join(no_sandbox_thread, (void **)&ret));
> +	EXPECT_EQ(0, pthread_join(no_sandbox_thread, &ret));
>  	EXPECT_EQ(THREAD_SUCCESS, ret);
>  
>  	EXPECT_EQ(0, close(thread_pipe[0]));
> @@ -295,7 +293,7 @@ TEST(signal_scoping_thread_before)
>  TEST(signal_scoping_thread_after)
>  {
>  	pthread_t scoped_thread;
> -	enum thread_return ret = THREAD_INVALID;
> +	void *ret = THREAD_INVALID;
>  	int thread_pipe[2];
>  
>  	drop_caps(_metadata);
> @@ -310,7 +308,7 @@ TEST(signal_scoping_thread_after)
>  	EXPECT_EQ(0, pthread_kill(scoped_thread, 0));
>  	EXPECT_EQ(1, write(thread_pipe[1], ".", 1));
>  
> -	EXPECT_EQ(0, pthread_join(scoped_thread, (void **)&ret));
> +	EXPECT_EQ(0, pthread_join(scoped_thread, &ret));
>  	EXPECT_EQ(THREAD_SUCCESS, ret);
>  
>  	EXPECT_EQ(0, close(thread_pipe[0]));
> @@ -327,20 +325,20 @@ void *thread_setuid(void *ptr)
>  	char buf;
>  
>  	if (read(arg->pipe_read, &buf, 1) != 1)
> -		return (void *)THREAD_ERROR;
> +		return THREAD_ERROR;
>  
>  	/* libc's setuid() should update all thread's credentials. */
>  	if (getuid() != arg->new_uid)
> -		return (void *)THREAD_TEST_FAILED;
> +		return THREAD_TEST_FAILED;
>  
> -	return (void *)THREAD_SUCCESS;
> +	return THREAD_SUCCESS;
>  }
>  
>  TEST(signal_scoping_thread_setuid)
>  {
>  	struct thread_setuid_args arg;
>  	pthread_t no_sandbox_thread;
> -	enum thread_return ret = THREAD_INVALID;
> +	void *ret = THREAD_INVALID;
>  	int pipe_parent[2];
>  	int prev_uid;
>  
> @@ -367,7 +365,7 @@ TEST(signal_scoping_thread_setuid)
>  	EXPECT_EQ(arg.new_uid, getuid());
>  	EXPECT_EQ(1, write(pipe_parent[1], ".", 1));
>  
> -	EXPECT_EQ(0, pthread_join(no_sandbox_thread, (void **)&ret));
> +	EXPECT_EQ(0, pthread_join(no_sandbox_thread, &ret));
>  	EXPECT_EQ(THREAD_SUCCESS, ret);
>  
>  	clear_cap(_metadata, CAP_SETUID);
> @@ -667,20 +665,20 @@ static void *thread_setown_scoped(void *arg)
>  	ruleset_fd =
>  		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>  	if (ruleset_fd < 0)
> -		return (void *)THREAD_ERROR;
> +		return THREAD_ERROR;
>  	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
>  	    landlock_restrict_self(ruleset_fd, 0)) {
>  		close(ruleset_fd);
> -		return (void *)THREAD_ERROR;
> +		return THREAD_ERROR;
>  	}
>  	close(ruleset_fd);
>  
>  	/* Makes this process group own the SIGIO source. */
>  	if (fcntl(fd, F_SETSIG, SIGURG) || fcntl(fd, F_SETOWN, -getpgrp()) ||
>  	    fcntl(fd, F_SETFL, O_ASYNC))
> -		return (void *)THREAD_ERROR;
> +		return THREAD_ERROR;
>  
> -	return (void *)THREAD_SUCCESS;
> +	return THREAD_SUCCESS;
>  }
>  
>  /*
> @@ -702,7 +700,7 @@ TEST(sigio_to_pgid_self)
>  {
>  	int trigger[2];
>  	pthread_t thread;
> -	enum thread_return ret = THREAD_INVALID;
> +	void *ret = THREAD_INVALID;
>  	int i;
>  
>  	drop_caps(_metadata);
> @@ -722,7 +720,7 @@ TEST(sigio_to_pgid_self)
>  	 */
>  	ASSERT_EQ(0, pthread_create(&thread, NULL, thread_setown_scoped,
>  				    &trigger[0]));
> -	ASSERT_EQ(0, pthread_join(thread, (void **)&ret));
> +	ASSERT_EQ(0, pthread_join(thread, &ret));
>  	ASSERT_EQ(THREAD_SUCCESS, ret);
>  
>  	/* Fans SIGURG out to the process group. */
> -- 
> 2.55.0
> 
>