lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

PATCH New Xiao Yang 2 participants 7 messages Jul 17, 2017 – Jul 20, 2017 Library code

Share & tools

Xiao Yang

author

2017-07-17 06:53 UTC #0 lore raw

PATCH 1/1 lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

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 <[email protected]>

---
 lib/tst_timer_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

lib/tst_timer_test.c +2 -2

index f30ad73..178e232 100644
@@ -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]);

Signature · 1 line

1.8.3.1

Cyril Hrubis

2017-07-19 12:15 UTC #1 lore raw

PATCH 1/1 lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

Hi!

Quoted 4 lines

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.

Quoted 10 lines

Signed-off-by Xiao Yang <[email protected]>
---
 lib/tst_timer_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
lib/tst_timer_test.c +0 -0
index f30ad73..178e232 100644

lib/tst_timer_test.c void do_timer_test(long long usec, unsigned int nsamples) · line 298

Quoted 5 lines

lib/tst_timer_test.c +1 -1

@@ -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:

lib/tst_timer_test.c +1 -1

index f30ad73dc..0b675f78d 100644
@@ -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]",

Signature · 2 lines

Cyril Hrubis
[email protected]

Xiao Yang

author

2017-07-20 02:21 UTC #2 lore raw

PATCH 1/1 lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

Quoted 37 lines · Cyril Hrubis

Hi!
Quoted 4 lines
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.
Quoted 10 lines
Signed-off-by Xiao Yang<[email protected]>
---
  lib/tst_timer_test.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
lib/tst_timer_test.c +0 -0
index f30ad73..178e232 100644
lib/tst_timer_test.c void do_timer_test(long long usec, unsigned int nsamples) · line 298
Quoted 5 lines
lib/tst_timer_test.c +1 -1
@@ -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:
lib/tst_timer_test.c +0 -0
index f30ad73dc..0b675f78d 100644

lib/tst_timer_test.c void do_timer_test(long long usec, unsigned int nsamples) · line 298

Quoted 5 lines

lib/tst_timer_test.c +1 -1

@@ -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

Quoted 4 lines

lib/tst_timer_test.c +0 -0

         if (i<  nsamples - 1) {
                 tst_res(TFAIL, "%s woken up early %u times range: [%lli,%lli]",

Cyril Hrubis

2017-07-20 09:13 UTC #3 lore raw

PATCH 1/1 lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

Hi!

Quoted 13 lines

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:
------------------------------------------------------------------------------------------------------------------------
(quoted diff) +1 -4
-    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]);

Review

Quoted 10 lines

(quoted diff) +9 -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.

Signature · 2 lines

Cyril Hrubis
[email protected]

Xiao Yang

author

2017-07-20 09:25 UTC #4 lore raw

PATCH 1/1 lib/tst_timer_test.c: fix unsigned int overflow on RHEL5.11GA

Quoted 24 lines · Cyril Hrubis

Hi!

Quoted 13 lines

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:
------------------------------------------------------------------------------------------------------------------------
(quoted diff) +1 -4
-    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]);

Review

Quoted 10 lines

(quoted diff) +9 -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]);
+        }
------------------------------------------------------------------------------------------------------------------------

Quoted 4 lines

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

Xiao Yang

author

2017-07-20 09:38 UTC #5 lore raw

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

In reply to Cyril Hrubis

Signed-off-by Xiao Yang <[email protected]>

---
 lib/tst_timer_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

lib/tst_timer_test.c +2 -2

index f30ad73..c8e2d38 100644
@@ -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]",

Signature · 1 line

1.8.3.1

Cyril Hrubis

2017-07-20 11:29 UTC #6 lore raw

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

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

Signature · 2 lines

Cyril Hrubis
[email protected]