ubi: wl: Silence uninitialized variable warning

PATCH In review Dan Carpenter 3 participants 5 messages Feb 28, 2019

Subsystems (3)

Kernel janitors Memory technology devices (MTD) Unsorted block images (UBI)

lore

Share & tools

lore

Dan Carpenter

author

2019-02-28 05:35 UTC #0 lore raw

PATCH 1/1 ubi: wl: Silence uninitialized variable warning

This condition needs to be fipped around because "err" is uninitialized when "force" is set. The Smatch static analysis tool complains and UBsan will also complain at runtime.

Fixes 663586c0a892 ("ubi: Expose the bitrot interface")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/mtd/ubi/wl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/ubi/wl.c +1 -1

index 40f838d54b0f..2709dc02fc24 100644
@@ -1517,7 +1517,7 @@ int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force)
 		mutex_unlock(&ubi->buf_mutex);
 	}
 
-	if (err = UBI_IO_BITFLIPS || force) {
+	if (force || err = UBI_IO_BITFLIPS) {
 		/*
 		 * Okay, bit flip happened, let's figure out what we can do.
 		 */

Signature · 1 line

2.17.1

Richard Weinberger

maintainer

2019-02-28 08:35 UTC #1 lore raw

Am Donnerstag, 28. Februar 2019, 06:35:51 CET schrieb Dan Carpenter:

Quoted 16 lines

This condition needs to be fipped around because "err" is uninitialized when "force" is set. The Smatch static analysis tool complains and UBsan will also complain at runtime.
Fixes 663586c0a892 ("ubi: Expose the bitrot interface")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/mtd/ubi/wl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/ubi/wl.c +0 -0
index 40f838d54b0f..2709dc02fc24 100644

drivers/mtd/ubi/wl.c int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force) · line 1523

Quoted 8 lines

drivers/mtd/ubi/wl.c +1 -1

@@ -1517,7 +1517,7 @@ int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force)
 		mutex_unlock(&ubi->buf_mutex);
 	}
 
-	if (err = UBI_IO_BITFLIPS || force) {
+	if (force || err = UBI_IO_BITFLIPS) {
 		/*
 		 * Okay, bit flip happened, let's figure out what we can do.
 		 */

Good catch, Dan!
I thought gcc is supposed to find such issues too. :-/

Thanks,
//richard

Nathan Chancellor

Reviewed-by Tested-by

2019-02-28 08:50 UTC #2 lore raw

Quoted 7 lines · Richard Weinberger

Am Donnerstag, 28. Februar 2019, 06:35:51 CET schrieb Dan Carpenter:
Quoted 6 lines
This condition needs to be fipped around because "err" is uninitialized when "force" is set. The Smatch static analysis tool complains and UBsan will also complain at runtime.
Fixes 663586c0a892 ("ubi: Expose the bitrot interface")
Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Nathan Chancellor <[email protected]>

Tested-by Nathan Chancellor <[email protected]>

This fixes a -Wsometimes-uninitialized warning from Clang:

drivers/mtd/ubi/wl.c:1514:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]

        if (!force) {
            ^~~~~~
drivers/mtd/ubi/wl.c:1520:6: note: uninitialized use occurs here
        if (err = UBI_IO_BITFLIPS || force) {
            ^~~
drivers/mtd/ubi/wl.c:1514:2: note: remove the 'if' if its condition is always true
        if (!force) {
        ^~~~~~~~~~~~
drivers/mtd/ubi/wl.c:1478:9: note: initialize the variable 'err' to silence this warning
        int err;
               ^
                = 0

1 warning generated.

Quoted 12 lines

---
 drivers/mtd/ubi/wl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/ubi/wl.c +0 -0

index 40f838d54b0f..2709dc02fc24 100644
@@ -1517,7 +1517,7 @@ int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force)
 		mutex_unlock(&ubi->buf_mutex);
 	}

Quoted 9 lines

drivers/mtd/ubi/wl.c int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force) · line 1523
Quoted 6 lines
drivers/mtd/ubi/wl.c +1 -1
-	if (err = UBI_IO_BITFLIPS || force) {
+	if (force || err = UBI_IO_BITFLIPS) {
 		/*
 		 * Okay, bit flip happened, let's figure out what we can do.
 		 */
Good catch, Dan!
I thought gcc is supposed to find such issues too. :-/

This isn't the first time GCC hasn't caught something...

https://lore.kernel.org/lkml/20190221222123.GC6474@magnolia/

Quoted 5 lines

Thanks,
//richard

Thanks,
Nathan

Richard Weinberger

maintainer

2019-02-28 09:51 UTC #3 lore raw

Am Donnerstag, 28. Februar 2019, 09:50:58 CET schrieb Nathan Chancellor:

Quoted 11 lines · Richard Weinberger

Quoted 7 lines
Am Donnerstag, 28. Februar 2019, 06:35:51 CET schrieb Dan Carpenter:
Quoted 6 lines
This condition needs to be fipped around because "err" is uninitialized when "force" is set. The Smatch static analysis tool complains and UBsan will also complain at runtime.
Fixes 663586c0a892 ("ubi: Expose the bitrot interface")
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Nathan Chancellor <[email protected]>
Tested-by Nathan Chancellor <[email protected]>

Did you really test the code or just compile it?

Quoted 15 lines

This fixes a -Wsometimes-uninitialized warning from Clang:

drivers/mtd/ubi/wl.c:1514:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]

        if (!force) {
            ^~~~~~
drivers/mtd/ubi/wl.c:1520:6: note: uninitialized use occurs here
        if (err = UBI_IO_BITFLIPS || force) {
            ^~~
drivers/mtd/ubi/wl.c:1514:2: note: remove the 'if' if its condition is always true
        if (!force) {
        ^~~~~~~~~~~~
drivers/mtd/ubi/wl.c:1478:9: note: initialize the variable 'err' to silence this warning
        int err;
               ^
                = 0

Quoted 1 line

1 warning generated.

How much false positives does this trigger?
Many useful gcc warnings are disabled because they produce too much churn.

Quoted 22 lines

Quoted 12 lines
Quoted 12 lines
---
 drivers/mtd/ubi/wl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/ubi/wl.c +0 -0
index 40f838d54b0f..2709dc02fc24 100644
@@ -1517,7 +1517,7 @@ int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force)
 		mutex_unlock(&ubi->buf_mutex);
 	}
 
Quoted 9 lines
drivers/mtd/ubi/wl.c int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force) · line 1523
Quoted 6 lines
drivers/mtd/ubi/wl.c +1 -1
-	if (err = UBI_IO_BITFLIPS || force) {
+	if (force || err = UBI_IO_BITFLIPS) {
 		/*
 		 * Okay, bit flip happened, let's figure out what we can do.
 		 */
Good catch, Dan!
I thought gcc is supposed to find such issues too. :-/

Quoted 3 lines

This isn't the first time GCC hasn't caught something...
https://lore.kernel.org/lkml/20190221222123.GC6474@magnolia/

Compilers are not perfect. :-)

Thanks,
//richard

Nathan Chancellor

2019-02-28 15:33 UTC #4 lore raw

Quoted 7 lines · Richard Weinberger

Am Donnerstag, 28. Februar 2019, 09:50:58 CET schrieb Nathan Chancellor:
Quoted 6 lines · Richard Weinberger
Quoted 5 lines
Am Donnerstag, 28. Februar 2019, 06:35:51 CET schrieb Dan Carpenter:
Quoted 4 lines
This condition needs to be fipped around because "err" is uninitialized when "force" is set. The Smatch static analysis tool complains and UBsan will also complain at runtime.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Fixes 663586c0a892 ("ubi: Expose the bitrot interface")
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Nathan Chancellor <[email protected]>
Tested-by Nathan Chancellor <[email protected]>
Did you really test the code or just compile it?

Compiled tested. If I shouldn't use that tag in that instance, please let me know.

Quoted 17 lines

Quoted 15 lines
This fixes a -Wsometimes-uninitialized warning from Clang:
drivers/mtd/ubi/wl.c:1514:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
        if (!force) {
            ^~~~~~
drivers/mtd/ubi/wl.c:1520:6: note: uninitialized use occurs here
        if (err = UBI_IO_BITFLIPS || force) {
            ^~~
drivers/mtd/ubi/wl.c:1514:2: note: remove the 'if' if its condition is always true
        if (!force) {
        ^~~~~~~~~~~~
drivers/mtd/ubi/wl.c:1478:9: note: initialize the variable 'err' to silence this warning
        int err;
               ^
                = 0
Quoted 1 line
1 warning generated.

Quoted 3 lines

How much false positives does this trigger?
Many useful gcc warnings are disabled because they produce too much churn.

I haven't gone through them all yet but it doesn't seem to trigger as often as GCC. I think there are only 22 files with a problem across arm, arm64, and x86_64 allyesconfig.

Quoted 22 lines

Quoted 22 lines
Quoted 21 lines
Quoted 10 lines
---
 drivers/mtd/ubi/wl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/ubi/wl.c +0 -0
index 40f838d54b0f..2709dc02fc24 100644
drivers/mtd/ubi/wl.c int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force) · line 1523
Quoted 8 lines
drivers/mtd/ubi/wl.c +1 -1
@@ -1517,7 +1517,7 @@ int ubi_bitflip_check(struct ubi_device *ubi, int pnum, int force)
 		mutex_unlock(&ubi->buf_mutex);
 	}
 
-	if (err = UBI_IO_BITFLIPS || force) {
+	if (force || err = UBI_IO_BITFLIPS) {
 		/*
 		 * Okay, bit flip happened, let's figure out what we can do.
 		 */
Good catch, Dan!
I thought gcc is supposed to find such issues too. :-/

Quoted 5 lines

Quoted 3 lines
This isn't the first time GCC hasn't caught something...
https://lore.kernel.org/lkml/20190221222123.GC6474@magnolia/
Compilers are not perfect. :-)

Would make everyone's life a lot better if they were :)

Nathan

Quoted 5 lines

Thanks,
//richard