Re: [LTP] [PATCH v3] openposix: timer_*/speculative: fork() to isolate the SIGSEGV on old glibc

[Petr Vorel <pvorel@suse.cz>]

Hi Avinesh,

> Commit d377d1cff1ec ("openposix: timer_*/speculative: Don't pass a
> stack pointer as timerid") fixed a SIGSEGV on i586, where current
> glibc's sign-bit check (timer_is_sigev_thread()) can misread a stack
> address as a tagged pointer. A small constant avoids that.

> But older glibc (<= 2.31, before this check existed) unconditionally
> dereferences timerid as a pointer, so the same small constant now
> faults instead:

>   timer_delete_sp[13131]: segfault at 186a3 ip 00007f15973aed4e sp 00007ffefb52c5a0 error 4 in librt-2.31.so[7f15973aa000+8000]

> fork() a child to make the call, then inspect the child's exit status.
> POSIX defines no required behavior for an invalid timer ID (EINVAL is
> only a recommendation), so the child being killed by SIGSEGV or SIGBUS
> is treated as success.

> Fixes: d377d1cff1ec ("openposix: timer_*/speculative: Don't pass a stack pointer as timerid")
> Suggested-by: Martin Doucha <mdoucha@suse.cz>
> Signed-off-by: Avinesh Kumar <avinesh.kumar@suse.com>
> ---
>  .../interfaces/timer_delete/speculative/5-1.c | 56 +++++++++++-----
>  .../timer_getoverrun/speculative/6-1.c        | 55 +++++++++++-----
>  .../timer_gettime/speculative/6-1.c           | 58 +++++++++++-----
>  .../timer_settime/speculative/12-1.c          | 66 +++++++++++++------
>  4 files changed, 169 insertions(+), 66 deletions(-)

> diff --git a/testcases/open_posix_testsuite/conformance/interfaces/timer_delete/speculative/5-1.c b/testcases/open_posix_testsuite/conformance/interfaces/timer_delete/speculative/5-1.c
> index dce9277b5d6a..8e7d1b2e54dc 100644
> --- a/testcases/open_posix_testsuite/conformance/interfaces/timer_delete/speculative/5-1.c
> +++ b/testcases/open_posix_testsuite/conformance/interfaces/timer_delete/speculative/5-1.c
> @@ -5,34 +5,58 @@
>   * of this license, see the COPYING file at the top level of this
>   * source tree.

> - * Test to see if timer_delete() returns -1 and sets errno==EINVAL if
> - * timerid is not a valid timer ID or not.
> - * Since this is a "may" requirement, either option is a PASS.
> + * Test to see if timer_delete() sets errno==EINVAL for an invalid
> + * timerid. Recent POSIX versions leave this behavior undefined, so

nit: I suggest before merge to mention the version:

"Recent POSIX versions leave" => "/Since POSIX 2016 Edition is"

(no need for v4). Why? In 5+ years we can hopefully delete those tests.
Specifying the version saves somebody searching it again.

The code itself LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp