Skip to contentSkip to conversation

Infinite loop/always true check possible with unsigned counter.

PATCH In review Manish Ahuja 4 participants 10 messages Jul 9, 2007 – Jul 10, 2007 PowerPC

lore
Share & tools
lore

Manish Ahuja

author
#0 lore raw

PATCH 1/1 Infinite loop/always true check possible with unsigned counter.

Fix to correct a possible infinite loop or an always true check when the unsigned long counter "i" is used in lmb_add_region() in the following for loop:

for (i = rgn->cnt-1; i >= 0; i--)

Signed-off-by Manish Ahuja <[email protected]>
---
 arch/powerpc/mm/lmb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/powerpc/mm/lmb.c +2 -2
Index: 2.6.22-rc4/arch/powerpc/mm/lmb.c
===================================================================
@@ -138,8 +138,8 @@ void __init lmb_analyze(void)
 static long __init lmb_add_region(struct lmb_region *rgn, unsigned long base,
 				  unsigned long size)
 {
-	unsigned long i, coalesced = 0;
-	long adjacent;
+	unsigned long coalesced = 0;
+	long adjacent, i;
 
 	/* First try and coalesce this LMB with another. */
 	for (i=0; i < rgn->cnt; i++) {
author
#1 lore raw

Repost to fix my email id.

Fix to correct a possible infinite loop or an always true check when the unsigned long counter "i" is used in lmb_add_region() in the following for loop:

for (i = rgn->cnt-1; i >= 0; i--)

Signed-off-by Manish Ahuja <[email protected]>
---
 arch/powerpc/mm/lmb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/powerpc/mm/lmb.c +2 -2
Index: 2.6.22-rc4/arch/powerpc/mm/lmb.c
===================================================================
@@ -138,8 +138,8 @@ void __init lmb_analyze(void)
 static long __init lmb_add_region(struct lmb_region *rgn, unsigned long base,
 				  unsigned long size)
 {
-	unsigned long i, coalesced = 0;
-	long adjacent;
+	unsigned long coalesced = 0;
+	long adjacent, i;
 
 	/* First try and coalesce this LMB with another. */
 	for (i=0; i < rgn->cnt; i++) {
#2 lore raw
Quoted 7 lines · Manish Ahuja

Repost to fix my email id.

Fix to correct a possible infinite loop or an always true check when the unsigned long counter "i" is used in
lmb_add_region() in the following for loop:

for (i = rgn->cnt-1; i >= 0; i--)

Unfortunately this won't fix the bug. Since rgn->cnt is unsigned long, the comparison will be done as an unsigned comparison even if i is signed.

Instead, change the loop to go from rgn->cnt down to 1, and in the loop body, change all instances of "i+1" to "i", and "i" to "i-1".

Paul.

#3 lore raw
Quoted 9 lines · Paul Mackerras
Quoted 4 lines

Fix to correct a possible infinite loop or an always true check when the unsigned long counter "i" is used in
lmb_add_region() in the following for loop:
for (i = rgn->cnt-1; i >= 0; i--)

Unfortunately this won't fix the bug. Since rgn->cnt is unsigned long, the comparison will be done as an unsigned comparison even if i is signed.
Instead, change the loop to go from rgn->cnt down to 1, and in the loop body, change all instances of "i+1" to "i", and "i" to "i-1".

What's about

i = rgn->cnt;
while (i--)
...
?

(or for (i=rgn->cnt; i--; ), if you want a for-loop)

Felix

#4 lore raw
In reply to Paul Mackerras
Quoted 8 lines · Paul Mackerras
Quoted 6 lines · Manish Ahuja

Repost to fix my email id.

Fix to correct a possible infinite loop or an always true check when the unsigned long counter "i" is used in
lmb_add_region() in the following for loop:

Quoted 5 lines
Quoted 1 line

for (i = rgn->cnt-1; i >= 0; i--)

Unfortunately this won't fix the bug. Since rgn->cnt is unsigned long, the comparison will be done as an unsigned comparison even if i is signed.

??? There is no rgn->cnt involved in the comparison.

Andreas.

Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
#5 lore raw
Quoted 10 lines · Andreas Schwab
Quoted 8 lines · Paul Mackerras
Quoted 6 lines · Manish Ahuja

Repost to fix my email id.

Fix to correct a possible infinite loop or an always true check when the unsigned long counter "i" is used in
lmb_add_region() in the following for loop:

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

for (i = rgn->cnt-1; i >= 0; i--)

Unfortunately this won't fix the bug. Since rgn->cnt is unsigned long, the comparison will be done as an unsigned comparison even if i is signed.

??? There is no rgn->cnt involved in the comparison.

Look further down in lmb_add_region; there is a second for loop that does

	for (i = rgn->cnt-1; i >= 0; i--)

I agree that Manish's patch description was misleading, but there is a real problem in there.

Paul.

#6 lore raw
Quoted 17 lines · Paul Mackerras
Quoted 16 lines · Andreas Schwab
Quoted 13 lines · Paul Mackerras
Quoted 7 lines · Manish Ahuja

Repost to fix my email id.

Fix to correct a possible infinite loop or an always true check when the unsigned long counter "i" is used in
lmb_add_region() in the following for loop:

for (i = rgn->cnt-1; i >= 0; i--)

Unfortunately this won't fix the bug. Since rgn->cnt is unsigned long, the comparison will be done as an unsigned comparison even if i is signed.

Quoted 6 lines
Quoted 1 line

??? There is no rgn->cnt involved in the comparison.

Look further down in lmb_add_region; there is a second for loop that does

	for (i = rgn->cnt-1; i >= 0; i--)

Which is exactly the one quoted above. I still don't see your point.

Andreas.

Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
#7 lore raw
Quoted 8 lines · Andreas Schwab
Quoted 6 lines
Quoted 1 line

??? There is no rgn->cnt involved in the comparison.

Look further down in lmb_add_region; there is a second for loop that does

	for (i = rgn->cnt-1; i >= 0; i--)

Which is exactly the one quoted above. I still don't see your point.

You're right - my mistake.

Paul.

author
#8 lore raw
Quoted 8 lines · Paul Mackerras
Quoted 5 lines · Andreas Schwab
Quoted 5 lines
Quoted 2 lines

??? There is no rgn->cnt involved in the comparison.

Look further down in lmb_add_region; there is a second for loop that does

Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
	for (i = rgn->cnt-1; i >= 0; i--)

Which is exactly the one quoted above. I still don't see your point.

You're right - my mistake.

Paul.

I presume the patch is good then. Do I need to change anything ?

Thanks,
Manish

#9 lore raw
Quoted 1 line · Manish Ahuja

I presume the patch is good then. Do I need to change anything ?

I guess not. It will cause a warning on the first for loop if anyone tries to compile with -Wextra or -Wsign-compare, but it would be only one of lots of warnings in that case (and in fact comparing signed with unsigned is a perfectly legitimate, well-defined thing to do; you just have to be aware that the comparison is done as unsigned).

Paul.