* [LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA @ 2017-07-17 6:53 Xiao Yang 2017-07-19 12:15 ` Cyril Hrubis 0 siblings, 1 reply; 7+ messages in thread From: Xiao Yang @ 2017-07-17 6:53 UTC (permalink / raw) To: ltp On RHEL5.11GA, pselect01 gets signal SIGSEGV due to unsigned int overflow when all members of the samples array are less than usec in do_timer_test(). all tests which apply timer measurement library could trigger this issue on RHEL5.11GA regularly. Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- lib/tst_timer_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c index f30ad73..178e232 100644 --- a/lib/tst_timer_test.c +++ b/lib/tst_timer_test.c @@ -295,9 +295,9 @@ void do_timer_test(long long usec, unsigned int nsamples) i, samples[0], samples[i-1]); } - for (i = nsamples - 1; samples[i] < usec; i--); + for (i = nsamples - 1; i < nsamples && samples[i] < usec; i--); - if (i < nsamples - 1) { + if (i != nsamples - 1) { tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", scall, nsamples - 1 - i, samples[i+1], samples[nsamples-1]); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA 2017-07-17 6:53 [LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA Xiao Yang @ 2017-07-19 12:15 ` Cyril Hrubis 2017-07-20 2:21 ` Xiao Yang 0 siblings, 1 reply; 7+ messages in thread From: Cyril Hrubis @ 2017-07-19 12:15 UTC (permalink / raw) To: ltp Hi! > On RHEL5.11GA, pselect01 gets signal SIGSEGV due to unsigned int > overflow when all members of the samples array are less than usec > in do_timer_test(). all tests which apply timer measurement library > could trigger this issue on RHEL5.11GA regularly. Ah righ, I've forgotten to add check for the array boundary there as well. > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > --- > lib/tst_timer_test.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c > index f30ad73..178e232 100644 > --- a/lib/tst_timer_test.c > +++ b/lib/tst_timer_test.c > @@ -295,9 +295,9 @@ void do_timer_test(long long usec, unsigned int nsamples) > i, samples[0], samples[i-1]); > } > > - for (i = nsamples - 1; samples[i] < usec; i--); > + for (i = nsamples - 1; i < nsamples && samples[i] < usec; i--); But this does not seem right, we are decrementing i, hence we should check that i > 0, otherwise we overflow anyway. With i < nsamples we will break the cycle after the overflow, which is wrong and works only by accident (since i is unsigned we overflow to UINT_MAX if we decrement 0). This should be the correct patch to fix the problem: diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c index f30ad73dc..0b675f78d 100644 --- a/lib/tst_timer_test.c +++ b/lib/tst_timer_test.c @@ -295,7 +295,7 @@ void do_timer_test(long long usec, unsigned int nsamples) i, samples[0], samples[i-1]); } - for (i = nsamples - 1; samples[i] < usec; i--); + for (i = nsamples - 1; samples[i] < usec && i > 0; i--); if (i < nsamples - 1) { tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA 2017-07-19 12:15 ` Cyril Hrubis @ 2017-07-20 2:21 ` Xiao Yang 2017-07-20 9:13 ` Cyril Hrubis 0 siblings, 1 reply; 7+ messages in thread From: Xiao Yang @ 2017-07-20 2:21 UTC (permalink / raw) To: ltp On 2017/07/19 20:15, Cyril Hrubis wrote: > Hi! >> On RHEL5.11GA, pselect01 gets signal SIGSEGV due to unsigned int >> overflow when all members of the samples array are less than usec >> in do_timer_test(). all tests which apply timer measurement library >> could trigger this issue on RHEL5.11GA regularly. > Ah righ, I've forgotten to add check for the array boundary there as > well. > >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >> --- >> lib/tst_timer_test.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c >> index f30ad73..178e232 100644 >> --- a/lib/tst_timer_test.c >> +++ b/lib/tst_timer_test.c >> @@ -295,9 +295,9 @@ void do_timer_test(long long usec, unsigned int nsamples) >> i, samples[0], samples[i-1]); >> } >> >> - for (i = nsamples - 1; samples[i]< usec; i--); >> + for (i = nsamples - 1; i< nsamples&& samples[i]< usec; i--); > But this does not seem right, we are decrementing i, hence we should > check that i> 0, otherwise we overflow anyway. With i< nsamples we > will break the cycle after the overflow, which is wrong and works only > by accident (since i is unsigned we overflow to UINT_MAX if we decrement > 0). > > > This should be the correct patch to fix the problem: > > diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c > index f30ad73dc..0b675f78d 100644 > --- a/lib/tst_timer_test.c > +++ b/lib/tst_timer_test.c > @@ -295,7 +295,7 @@ void do_timer_test(long long usec, unsigned int nsamples) > i, samples[0], samples[i-1]); > } > > - for (i = nsamples - 1; samples[i]< usec; i--); > + for (i = nsamples - 1; samples[i]< usec&& i> 0; i--); Hi Cyril, It sounds better, but this change leads that samples[0] could not be checked if the whole samples array is less than usec. Could we fix this issue as below: ------------------------------------------------------------------------------------------------------------------------ - for (i = nsamples - 1; samples[i] < usec; i--); + for (i = nsamples - 1; samples[i] < usec && i > 0; i--); if (i < nsamples - 1) { - tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", - scall, nsamples - 1 - i, - samples[i+1], samples[nsamples-1]); + if (i == 0 && samples[i] < usec) { + tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", + scall, nsamples - i, samples[i], + samples[nsamples-1]); + } else { + tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", + scall, nsamples - 1 - i, samples[i+1], + samples[nsamples-1]); + } ------------------------------------------------------------------------------------------------------------------------ Thanks, Xiao Yang > > if (i< nsamples - 1) { > tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA 2017-07-20 2:21 ` Xiao Yang @ 2017-07-20 9:13 ` Cyril Hrubis 2017-07-20 9:25 ` Xiao Yang 2017-07-20 9:38 ` [LTP] [PATCH v2] " Xiao Yang 0 siblings, 2 replies; 7+ messages in thread From: Cyril Hrubis @ 2017-07-20 9:13 UTC (permalink / raw) To: ltp Hi! > It sounds better, but this change leads that samples[0] could not be > checked if the whole samples array > is less than usec. > > Could we fix this issue as below: > ------------------------------------------------------------------------------------------------------------------------ > - for (i = nsamples - 1; samples[i] < usec; i--); > + for (i = nsamples - 1; samples[i] < usec && i > 0; i--); > > if (i < nsamples - 1) { > - tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", > - scall, nsamples - 1 - i, > - samples[i+1], samples[nsamples-1]); > + if (i == 0 && samples[i] < usec) { > + tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", > + scall, nsamples - i, samples[i], > + samples[nsamples-1]); > + } else { > + tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", > + scall, nsamples - 1 - i, samples[i+1], > + samples[nsamples-1]); > + } > ------------------------------------------------------------------------------------------------------------------------ That is still unnecessarily complex. I do not care that much about this corner case, but if we want to fix it correctly we can simply change the i to be signed integer and check for i > -1 in the loop and everything would work fine. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA 2017-07-20 9:13 ` Cyril Hrubis @ 2017-07-20 9:25 ` Xiao Yang 2017-07-20 9:38 ` [LTP] [PATCH v2] " Xiao Yang 1 sibling, 0 replies; 7+ messages in thread From: Xiao Yang @ 2017-07-20 9:25 UTC (permalink / raw) To: ltp On 2017/07/20 17:13, Cyril Hrubis wrote: > Hi! >> It sounds better, but this change leads that samples[0] could not be >> checked if the whole samples array >> is less than usec. >> >> Could we fix this issue as below: >> ------------------------------------------------------------------------------------------------------------------------ >> - for (i = nsamples - 1; samples[i]< usec; i--); >> + for (i = nsamples - 1; samples[i]< usec&& i> 0; i--); >> >> if (i< nsamples - 1) { >> - tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", >> - scall, nsamples - 1 - i, >> - samples[i+1], samples[nsamples-1]); >> + if (i == 0&& samples[i]< usec) { >> + tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", >> + scall, nsamples - i, samples[i], >> + samples[nsamples-1]); >> + } else { >> + tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", >> + scall, nsamples - 1 - i, samples[i+1], >> + samples[nsamples-1]); >> + } >> ------------------------------------------------------------------------------------------------------------------------ > That is still unnecessarily complex. I do not care that much about this > corner case, but if we want to fix it correctly we can simply change the > i to be signed integer and check for i> -1 in the loop and everything > would work fine. Hi Cyril, Thanks for your explanation. I will send v2 patch as you said. Thanks, Xiao Yang ^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] [PATCH v2] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA 2017-07-20 9:13 ` Cyril Hrubis 2017-07-20 9:25 ` Xiao Yang @ 2017-07-20 9:38 ` Xiao Yang 2017-07-20 11:29 ` Cyril Hrubis 1 sibling, 1 reply; 7+ messages in thread From: Xiao Yang @ 2017-07-20 9:38 UTC (permalink / raw) To: ltp On RHEL5.11GA, pselect01 gets signal SIGSEGV due to unsigned int overflow when all members of the samples array are less than usec in do_timer_test(). all tests which apply timer measurement library could trigger this issue on RHEL5.11GA regularly. We add check the array boundary to fix it. Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- lib/tst_timer_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/tst_timer_test.c b/lib/tst_timer_test.c index f30ad73..c8e2d38 100644 --- a/lib/tst_timer_test.c +++ b/lib/tst_timer_test.c @@ -266,7 +266,7 @@ void do_timer_test(long long usec, unsigned int nsamples) unsigned int discard = compute_discard(nsamples); unsigned int keep_samples = nsamples - discard; long long threshold = compute_threshold(usec, keep_samples); - unsigned int i; + int i; int failed = 0; tst_res(TINFO, @@ -295,7 +295,7 @@ void do_timer_test(long long usec, unsigned int nsamples) i, samples[0], samples[i-1]); } - for (i = nsamples - 1; samples[i] < usec; i--); + for (i = nsamples - 1; samples[i] < usec && i > -1; i--); if (i < nsamples - 1) { tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]", -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [LTP] [PATCH v2] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA 2017-07-20 9:38 ` [LTP] [PATCH v2] " Xiao Yang @ 2017-07-20 11:29 ` Cyril Hrubis 0 siblings, 0 replies; 7+ messages in thread From: Cyril Hrubis @ 2017-07-20 11:29 UTC (permalink / raw) To: ltp Hi! I've changed the patch to silence some unsigned vs signed comparsion warnings it introduced and pushed, thanks. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-20 11:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-17 6:53 [LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA Xiao Yang 2017-07-19 12:15 ` Cyril Hrubis 2017-07-20 2:21 ` Xiao Yang 2017-07-20 9:13 ` Cyril Hrubis 2017-07-20 9:25 ` Xiao Yang 2017-07-20 9:38 ` [LTP] [PATCH v2] " Xiao Yang 2017-07-20 11:29 ` Cyril Hrubis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox