166739 - Variables may be used uninitialized in devtoken.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

When compiling NSS under gcc on Linux, we get these
warnings in devtoken.c:

devtoken.c: In function `find_objects_by_template':
devtoken.c:497: warning: `objclass' might be used uninitialized in this function
devtoken.c: In function `get_ck_trust':
devtoken.c:1111: warning: `t' might be used uninitialized in this function

The first one should be harmless because of an assertion
in that function.  The second one is an error.

(This bug refers to rev. 1.28 of devtoken.c.)

Comment 1

[Ian McGreer]

Attachment: silence warnings

It wasn't really an error; all the enumeration values were handled, so t should
always be set.	But this should silence the warnings.

Comment 2

[Ian McGreer]

Attachment: rev 2

Actually, I didn't see the warnings, and I realized my last patch may not do
what is needed.  This one should.

Comment 3

[Wan-Teh Chang]

Comment on attachment 97986 [details] [diff] [review]
rev 2

Ian,

You need to build with BUILD_OPT=1 on Linux to see these
warnings.  gcc does not issue these warnings in the debug
builds.  I don't know why.

If you are sure a warning is not an error, you don't need
to fix it.

>@@ -1108,7 +1108,7 @@
>   nssTrustLevel nssTrust
> )
> {
>-    CK_TRUST t;
>+    CK_TRUST t = CKT_NETSCAPE_TRUST_UNKNOWN;
>     switch (nssTrust) {
>     case nssTrustLevel_Unknown: t = CKT_NETSCAPE_TRUST_UNKNOWN; break;
>     case nssTrustLevel_NotTrusted: t = CKT_NETSCAPE_UNTRUSTED; break;

I suggest you fix this one by adding a 'default' case to the switch
statement and set 't' to CKT_NETSCAPE_TRUST_UNKNOWN in the 'default'
case.

Comment 4

[Ian McGreer]

Attachment: rev3

Comment 5

[Wan-Teh Chang]

Comment on attachment 101002 [details] [diff] [review]
rev3

>+    default:
>+    case nssTrustLevel_Unknown: t = CKT_NETSCAPE_TRUST_UNKNOWN; break;
>     }

It'd be safer to put 'default' as the last case.
I am not sure if the language requires that, but
it is a little strange to see 'default' not at
the end.

Comment 6

[Wan-Teh Chang]

Comment on attachment 101002 [details] [diff] [review]
rev3

>-    CK_OBJECT_CLASS objclass;
>+    CK_OBJECT_CLASS objclass = 0;

I suggest that you replace this 0 by a macro (CKO_DATA?)
to make it more readable.

Comment 7

[Wan-Teh Chang]

The best way to fix the uninitialized 'objclass'
at line 497 is to replace or augment the assertion
at line 506

    PR_ASSERT(i < otsize);

with an if statement like this:

    if (i == otsize) {
        /* handle this case, most
         * likely as an error */
    }

If the case i == otsize should be handled as an
error, make sure you set the error code before
returning objects (which is NULL at that point).

Comment 8

[Ian McGreer]

Attachment: rev4

Try again.

1)  I don't like the idea of initializing to CKO_DATA because it is
meaningless.  If I were unfamiliar with this bug, and saw the code, I would
wonder why it was there.  Since CKO_DATA is apparently 0, I have changed it to
-1, which should be easily recognizable as invalid.  There is no macro for an
invalid class value.

2)  I added the if statement after the assert.	Without initializing the
objclass variable, we still get the warning.  The compiler does not recognize
that all cases have been handled.

Comment 9

[Wan-Teh Chang]

Comment on attachment 101159 [details] [diff] [review]
rev4

r=wtc.

>-    CK_OBJECT_CLASS objclass;
>+    CK_OBJECT_CLASS objclass = -1;

You might want to add a (CK_OBJECT_CLASS) cast before
the -1 because CK_OBJECT_CLASS is an unsigned long.

>+    if (i == otsize) {
>+#ifdef NSS_3_4_CODE
>+	PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>+#endif
>+	return NULL;
>+    }

Just curious, why the #ifdef NSS_3_4_CODE?

Comment 10

[Ian McGreer]

1) done

2) devtoken.c is Stan code. PORT_SetError is a 3.X function.  Any time Stan code
calls a 3.X function, I use NSS_3_4_CODE.  That way, when the tip is built as
4.0 (or actually before, when it is merged with 4.0), it is easy to identify 3.X
code that needs to be removed/replaced.

Patch checked in.