* [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms
@ 2021-06-08 13:27 Thadeu Lima de Souza Cascardo
2021-06-09 8:23 ` Cyril Hrubis
0 siblings, 1 reply; 6+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2021-06-08 13:27 UTC (permalink / raw)
To: ltp
When multiplying 30 with NSEC_PER_SEC, the result would overflow on 32-bit
platforms, unless cast to long long, which is what tst_ts_from_ns expects.
Though we could change NSEC_PER_SEC to be a long long constant, it was
considered a risk for regressions, as it would affect every use of it.
After this change, futex_wake04 passes on i386.
Reported-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
testcases/kernel/syscalls/futex/futex_wake04.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/testcases/kernel/syscalls/futex/futex_wake04.c b/testcases/kernel/syscalls/futex/futex_wake04.c
index 2260a3936d6e..479001e2e59a 100644
--- a/testcases/kernel/syscalls/futex/futex_wake04.c
+++ b/testcases/kernel/syscalls/futex/futex_wake04.c
@@ -56,7 +56,7 @@ static void setup(void)
tst_res(TINFO, "Testing variant: %s", tv->desc);
futex_supported_by_kernel(tv->fntype);
- to = tst_ts_from_ns(tv->tstype, 30 * NSEC_PER_SEC);
+ to = tst_ts_from_ns(tv->tstype, (long long) 30 * NSEC_PER_SEC);
}
static void *wait_thread1(void *arg LTP_ATTRIBUTE_UNUSED)
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms
2021-06-08 13:27 [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms Thadeu Lima de Souza Cascardo
@ 2021-06-09 8:23 ` Cyril Hrubis
2021-06-09 11:20 ` Thadeu Lima de Souza Cascardo
0 siblings, 1 reply; 6+ messages in thread
From: Cyril Hrubis @ 2021-06-09 8:23 UTC (permalink / raw)
To: ltp
Hi!
Good catch, I guess that it would be slightly cleaner to fix the
NSEC_PER_SEC size instead with:
diff --git a/include/lapi/common_timers.h b/include/lapi/common_timers.h
index b783beff4..e50f698d6 100644
--- a/include/lapi/common_timers.h
+++ b/include/lapi/common_timers.h
@@ -12,7 +12,7 @@
#include "lapi/posix_clocks.h"
#ifndef NSEC_PER_SEC
-#define NSEC_PER_SEC (1000000000L)
+#define NSEC_PER_SEC (1000000000LL)
#endif
static const clock_t clock_list[] = {
What do you think?
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms
2021-06-09 8:23 ` Cyril Hrubis
@ 2021-06-09 11:20 ` Thadeu Lima de Souza Cascardo
2021-06-09 13:38 ` Cyril Hrubis
0 siblings, 1 reply; 6+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2021-06-09 11:20 UTC (permalink / raw)
To: ltp
On Wed, Jun 09, 2021 at 10:23:06AM +0200, Cyril Hrubis wrote:
> Hi!
> Good catch, I guess that it would be slightly cleaner to fix the
> NSEC_PER_SEC size instead with:
>
> diff --git a/include/lapi/common_timers.h b/include/lapi/common_timers.h
> index b783beff4..e50f698d6 100644
> --- a/include/lapi/common_timers.h
> +++ b/include/lapi/common_timers.h
> @@ -12,7 +12,7 @@
> #include "lapi/posix_clocks.h"
>
> #ifndef NSEC_PER_SEC
> -#define NSEC_PER_SEC (1000000000L)
> +#define NSEC_PER_SEC (1000000000LL)
> #endif
>
> static const clock_t clock_list[] = {
>
>
> What do you think?
>
> --
> Cyril Hrubis
> chrubis@suse.cz
Yeah, I even wrote and tested that it would build, but I wasn't able to go
through any build logs or run complete tests.
But maybe I am just overthinking this, and any promotions due to this would
only fix issues and not introduce any new ones.
Well, on second thought, on 32-bit, in cases like variadic functions, it could
cause a problem. So, I am just afraid that applying this fix without looking
for every use of NSEC_PER_SEC might lead to new bugs.
Cascardo.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms
2021-06-09 11:20 ` Thadeu Lima de Souza Cascardo
@ 2021-06-09 13:38 ` Cyril Hrubis
2021-06-09 14:22 ` Thadeu Lima de Souza Cascardo
0 siblings, 1 reply; 6+ messages in thread
From: Cyril Hrubis @ 2021-06-09 13:38 UTC (permalink / raw)
To: ltp
Hi!
> > Good catch, I guess that it would be slightly cleaner to fix the
> > NSEC_PER_SEC size instead with:
> >
> > diff --git a/include/lapi/common_timers.h b/include/lapi/common_timers.h
> > index b783beff4..e50f698d6 100644
> > --- a/include/lapi/common_timers.h
> > +++ b/include/lapi/common_timers.h
> > @@ -12,7 +12,7 @@
> > #include "lapi/posix_clocks.h"
> >
> > #ifndef NSEC_PER_SEC
> > -#define NSEC_PER_SEC (1000000000L)
> > +#define NSEC_PER_SEC (1000000000LL)
> > #endif
> >
> > static const clock_t clock_list[] = {
> >
> >
> > What do you think?
> >
> > --
> > Cyril Hrubis
> > chrubis@suse.cz
>
> Yeah, I even wrote and tested that it would build, but I wasn't able to go
> through any build logs or run complete tests.
>
> But maybe I am just overthinking this, and any promotions due to this would
> only fix issues and not introduce any new ones.
That would be my expectation.
> Well, on second thought, on 32-bit, in cases like variadic functions, it could
> cause a problem. So, I am just afraid that applying this fix without looking
> for every use of NSEC_PER_SEC might lead to new bugs.
Having a look at 'git grep NSEC_PER_SEC' there are five uses of
NSEC_PER_SEC and all of these should be, as far as I can tell, safe.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms
2021-06-09 14:22 ` Thadeu Lima de Souza Cascardo
@ 2021-06-09 14:09 ` Cyril Hrubis
0 siblings, 0 replies; 6+ messages in thread
From: Cyril Hrubis @ 2021-06-09 14:09 UTC (permalink / raw)
To: ltp
Hi!
> Yes, there are even uses where NSEC_PER_SEC is redefined.
>
> I am fine with this other change. I had already tested that it fixes the
> problem just the same.
>
> How about the commit below?
Looks good.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms
2021-06-09 13:38 ` Cyril Hrubis
@ 2021-06-09 14:22 ` Thadeu Lima de Souza Cascardo
2021-06-09 14:09 ` Cyril Hrubis
0 siblings, 1 reply; 6+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2021-06-09 14:22 UTC (permalink / raw)
To: ltp
On Wed, Jun 09, 2021 at 03:38:05PM +0200, Cyril Hrubis wrote:
> Hi!
> > > Good catch, I guess that it would be slightly cleaner to fix the
> > > NSEC_PER_SEC size instead with:
> > >
> > > diff --git a/include/lapi/common_timers.h b/include/lapi/common_timers.h
> > > index b783beff4..e50f698d6 100644
> > > --- a/include/lapi/common_timers.h
> > > +++ b/include/lapi/common_timers.h
> > > @@ -12,7 +12,7 @@
> > > #include "lapi/posix_clocks.h"
> > >
> > > #ifndef NSEC_PER_SEC
> > > -#define NSEC_PER_SEC (1000000000L)
> > > +#define NSEC_PER_SEC (1000000000LL)
> > > #endif
> > >
> > > static const clock_t clock_list[] = {
> > >
> > >
> > > What do you think?
> > >
> > > --
> > > Cyril Hrubis
> > > chrubis@suse.cz
> >
> > Yeah, I even wrote and tested that it would build, but I wasn't able to go
> > through any build logs or run complete tests.
> >
> > But maybe I am just overthinking this, and any promotions due to this would
> > only fix issues and not introduce any new ones.
>
> That would be my expectation.
>
> > Well, on second thought, on 32-bit, in cases like variadic functions, it could
> > cause a problem. So, I am just afraid that applying this fix without looking
> > for every use of NSEC_PER_SEC might lead to new bugs.
>
> Having a look at 'git grep NSEC_PER_SEC' there are five uses of
> NSEC_PER_SEC and all of these should be, as far as I can tell, safe.
>
Yes, there are even uses where NSEC_PER_SEC is redefined.
I am fine with this other change. I had already tested that it fixes the
problem just the same.
How about the commit below?
Cascardo.
> --
> Cyril Hrubis
> chrubis@suse.cz
From 3c248cd5e6fc1dd967787249857fd310e688d89f Mon Sep 17 00:00:00 2001
From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Date: Tue, 8 Jun 2021 10:21:48 -0300
Subject: [PATCH] common_timers: define NSEC_PER_SEC as long long to avoid
overflow on 32-bit
When multiplying 30 with NSEC_PER_SEC, the result would overflow on 32-bit
platforms, unless there was promotion to long long, which is what
tst_ts_from_ns expects.
futex_wake04, which uses that, would end up getting EINVAL when calling futex,
because timespec_valid would fail, as tv_nsec would have an invalid value.
After this change, futex_wake04 passes on i386.
Reported-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
include/lapi/common_timers.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/lapi/common_timers.h b/include/lapi/common_timers.h
index 8da3abec24b2..953741d73810 100644
--- a/include/lapi/common_timers.h
+++ b/include/lapi/common_timers.h
@@ -12,7 +12,7 @@
#include "lapi/posix_clocks.h"
#ifndef NSEC_PER_SEC
-#define NSEC_PER_SEC (1000000000L)
+#define NSEC_PER_SEC (1000000000LL)
#endif
static const clock_t clock_list[] = {
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-09 14:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08 13:27 [LTP] [PATCH] futex_wake04: avoid tst_ts_from_ns overflow on 32-bit platforms Thadeu Lima de Souza Cascardo
2021-06-09 8:23 ` Cyril Hrubis
2021-06-09 11:20 ` Thadeu Lima de Souza Cascardo
2021-06-09 13:38 ` Cyril Hrubis
2021-06-09 14:22 ` Thadeu Lima de Souza Cascardo
2021-06-09 14:09 ` Cyril Hrubis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.