Re: [LTP] [PATCH v2] openposix: timer_*/speculative: Handle SIGSEGV on invalid timer ID

[Petr Vorel <pvorel@suse.cz>]

Hi Avines,

> timer_delete/speculative/5-1, timer_getoverrun/speculative/6-1,
> timer_gettime/speculative/6-1 and timer_settime/speculative/12-1
> all pass a bogus value as an invalid timer_t timerid.
> On i586, glibc dereferences timer_t as a pointer into
> internal state, and the bogus pointer causes a SIGSEGV
> instead of the tests' expected EINVAL:

>   timer_delete_sp[22499]: segfault at 7f4982f0 ip b7e07824 sp bfa4c140 error 4 in libc.so.6[a4824,b7d87000+191000]

Good catch, thanks!

> POSIX defines no required behavior for an invalid timer ID (EINVAL is
> only a recommendation), so a SIGSEGV is just as valid an outcome.

> Signed-off-by: Avinesh Kumar <avinesh.kumar@suse.com>
> ---
>  .../interfaces/timer_delete/speculative/5-1.c | 19 ++++++++++++++++++
>  .../timer_getoverrun/speculative/6-1.c        | 19 ++++++++++++++++++
>  .../timer_gettime/speculative/6-1.c           | 20 +++++++++++++++++++
>  .../timer_settime/speculative/12-1.c          | 20 +++++++++++++++++++
>  4 files changed, 78 insertions(+)

> 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 912cf5800e6f..af01f72a59d1 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
> @@ -13,16 +13,35 @@
>  #include <time.h>
>  #include <stdio.h>
>  #include <errno.h>
> +#include <signal.h>
> +#include <unistd.h>
>  #include "posixtest.h"

>  #define BOGUSTIMERID 99999

> +/*
> + * when timerid argument does not correspond to a timer ID returned by
> + * timer_create(), POSIX recommends EINVAL, but SIGSEGV is also
> + * valid outcome.
> + */
> +static void sigsegv_handler(int signum PTS_ATTRIBUTE_UNUSED)
> +{
> +	PTS_WRITE_MSG("Got SIGSEGV when calling timer_delete() with an invalid timer ID\n");
> +	PTS_WRITE_MSG("Test PASSED\n");
> +	_exit(PTS_PASS);
> +}

I'm not sure if the approach of creating bogus timer_t is a good idea anyway.
Shouldn't be just some timer_t tid set to invalid value? That would IMHO be
less awkward solution.

That would help not having to fallback due SIGSEGV which 1) can happen on any
arch (covered because fix is not i686 specific) 2) test is simple but any
potential SIGSEGV.

And, if we want to use kind of bad pointer, shouldn't we backport
tst_get_bad_addr() from lib/tst_get_bad_addr.c?

void *tst_get_bad_addr(void (*cleanup_fn) (void))
{
	void *bad_addr;

	bad_addr = mmap(0, 1, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
	if (bad_addr == MAP_FAILED)
		tst_brkm(TBROK, cleanup_fn, "mmap() failed to get bad address");

	return bad_addr;
}

Tests which are using this it does not SIGSEGV.

nit: if the current approach is ok, even the function is small, it's in 4 files.
We have general helpers in testcases/open_posix_testsuite/include/,
sigsegv_handler() could be in one of them (I'd omit function name, which is test
specific).

Kind regards,
Petr

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