kern/37977: questionable code in sys/dev/ic/nslm7x.c

[fukumoto%imasy.or.jp@localhost]

>Number:         37977
>Category:       kern
>Synopsis:       questionable code in sys/dev/ic/nslm7x.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Feb 07 19:00:00 +0000 2008
>Originator:     fukumoto%imasy.or.jp@localhost
>Release:        -current as of Feb 7
>Organization:
        
>Environment:
        
        
System: NetBSD kestrel 4.0_STABLE NetBSD 4.0_STABLE (GENERIC.MP) #7: Sat Dec 22 
03:36:14 JST 2007 
fukumoto@kestrel:/usr/src.netbsd4/sys/arch/i386/compile/GENERIC.MP i386
Architecture: i386
Machine: i386
>Description:
        While browsing kernel source code, I noticed a questionable
        code in lm_refresh_volt() in sys/dev/ic/nslm7x.c (rev 1.44).

        It compares the data and sets INVALID state, but later
        overwrites with VALID.

        Please note I'm reporting from source code inspection, I don't
        have the device the code is handling.  Sorry if this is a
        false alert.
>How-To-Repeat:
        
>Fix:
Index: nslm7x.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/nslm7x.c,v
retrieving revision 1.44
diff -u -u -p -r1.44 nslm7x.c
--- nslm7x.c    16 Nov 2007 08:00:14 -0000      1.44
+++ nslm7x.c    7 Feb 2008 18:42:57 -0000
@@ -1911,22 +1911,23 @@ lm_refresh_volt(struct lm_softc *sc, int
        int data;
 
        data = (*sc->lm_readreg)(sc, sc->lm_sensors[n].reg);
-       if (data == 0xff)
+       if (data == 0xff) {
                sc->sensors[n].state = ENVSYS_SINVALID;
+       } else {
+               sc->sensors[n].flags = ENVSYS_FCHANGERFACT;
+               sc->sensors[n].value_cur = (data << 4);
 
-       sc->sensors[n].flags = ENVSYS_FCHANGERFACT;
-       sc->sensors[n].value_cur = (data << 4);
+               if (sc->sensors[n].rfact) {
+                       sc->sensors[n].value_cur *= sc->sensors[n].rfact;
+                       sc->sensors[n].value_cur /= 10;
+               } else {
+                       sc->sensors[n].value_cur *= sc->lm_sensors[n].rfact;
+                       sc->sensors[n].value_cur /= 10;
+                       sc->sensors[n].rfact = sc->lm_sensors[n].rfact;
+               }
 
-       if (sc->sensors[n].rfact) {
-               sc->sensors[n].value_cur *= sc->sensors[n].rfact;
-               sc->sensors[n].value_cur /= 10;
-       } else {
-               sc->sensors[n].value_cur *= sc->lm_sensors[n].rfact;
-               sc->sensors[n].value_cur /= 10;
-               sc->sensors[n].rfact = sc->lm_sensors[n].rfact;
+               sc->sensors[n].state = ENVSYS_SVALID;
        }
-
-       sc->sensors[n].state = ENVSYS_SVALID;
        DPRINTF(("%s: volt[%d] data=0x%x value_cur=%d\n",
            __func__, n, data, sc->sensors[n].value_cur));
 }

>Unformatted: