[LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[Xiao Yang]

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

[LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[Cyril Hrubis]

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

[LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[Xiao Yang]

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]",
>

[LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[Cyril Hrubis]

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

[LTP] [PATCH] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[Xiao Yang]

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

[LTP] [PATCH v2] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[Xiao Yang]

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

[LTP] [PATCH v2] lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

[Cyril Hrubis]

Hi!
I've changed the patch to silence some unsigned vs signed comparsion
warnings it introduced and pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz