Variables may be used uninitialized in devtoken.c

RESOLVED FIXED in 3.6

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Ian McGreer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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.)
(Assignee)

Comment 1

16 years ago
Created attachment 97985 [details] [diff] [review]
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.
(Assignee)

Comment 2

16 years ago
Created attachment 97986 [details] [diff] [review]
rev 2


Actually, I didn't see the warnings, and I realized my last patch may not do
what is needed.  This one should.
Attachment #97985 - Attachment is obsolete: true
(Reporter)

Comment 3

16 years ago
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.
(Assignee)

Updated

16 years ago
Attachment #97985 - Attachment is obsolete: false
(Assignee)

Updated

16 years ago
Attachment #97986 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Priority: -- → P1
Target Milestone: --- → 3.6
(Assignee)

Comment 4

15 years ago
Created attachment 101002 [details] [diff] [review]
rev3
Attachment #97985 - Attachment is obsolete: true
(Reporter)

Comment 5

15 years ago
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.
(Reporter)

Comment 6

15 years ago
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.
(Reporter)

Comment 7

15 years ago
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).
(Assignee)

Comment 8

15 years ago
Created attachment 101159 [details] [diff] [review]
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.
Attachment #101002 - Attachment is obsolete: true
(Reporter)

Comment 9

15 years ago
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?
Attachment #101159 - Flags: review+
(Assignee)

Comment 10

15 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.