119481 - Occurances of uninitialized variables being used before being set (in security/manager).

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Aleksey Nogin]

For more details on this problem, see bug 59652

This bug is just for the warnings in various source files in the PSM.

Currently (http://tinderbox.mozilla.org/SeaMonkey/warn1010759460.5770.html) I
see the following warnings:

security/manager/pki/src/nsASN1Outliner.cpp:61
 `PRInt32 rowsToDelete' might be used uninitialized in this function

security/manager/ssl/src/nsCrypto.cpp:182
 `class nsISupports * foundInterface' might be used uninitialized in this function

security/manager/ssl/src/nsCrypto.cpp:1841
 `int numResponses' might be used uninitialized in this function

security/manager/ssl/src/nsCrypto.cpp:192
 `class nsISupports * foundInterface' might be used uninitialized in this function

security/manager/ssl/src/nsCrypto.cpp:202
 `class nsISupports * foundInterface' might be used uninitialized in this function

security/manager/ssl/src/nsKeygenHandler.cpp:115
 `unsigned char * buf' might be used uninitialized in this function

security/manager/ssl/src/nsNSSCertificate.cpp:3206
 `enum SECStatus sec_rv' might be used uninitialized in this function

security/manager/ssl/src/nsNSSIOLayer.cpp:695
 `PRBool oldBlockVal' might be used uninitialized in this function

Comment 1

[Stephane Saux]

Attachment: patch to nsNSSCertificate.cpp

I looked at all of these, and I found one place where code changes where
definitely needed.  nsNSSCertificate did use the sec_rv value uninitialized.

Kai can you review?  Kai, this seems to be used for content handlers, and I
thought that content handlers had been moved to pip boot.  Can you review this
carefully?

You should probably look at the other warnings.  I didn't find anything that
was a bug.  In a few places we could suppress the warnings (rowstoDelete, buf,
and oldBlockVal, but in all cases that would mean adding instructions to the
code path that are not needed, as the initial assignment will invariably be
overwritten), but for the foundInterface warning, they seem to emanate from
XPCOM macros.

Comment 2

[Stephane Saux]

kai

Comment 3

[Kai Engert (:KaiE:)]

>security/manager/pki/src/nsASN1Outliner.cpp:61
> `PRInt32 rowsToDelete' might be used uninitialized in this function

No fix needed.
If we want to, let's init rowsToDelete with zero.

>security/manager/ssl/src/nsCrypto.cpp:182
> `class nsISupports * foundInterface' might be used uninitialized in this function
>security/manager/ssl/src/nsCrypto.cpp:192
> `class nsISupports * foundInterface' might be used uninitialized in this function
>security/manager/ssl/src/nsCrypto.cpp:202
> `class nsISupports * foundInterface' might be used uninitialized in this function

These are not security, but XPCOM warnings, look at
xpcom/base/nsISupportsImpl.h, which defines macros not initializing that variable.

>security/manager/ssl/src/nsCrypto.cpp:1841
> `int numResponses' might be used uninitialized in this function

This warning is wrong, the first usage is assignment.

>security/manager/ssl/src/nsKeygenHandler.cpp:115
> `unsigned char * buf' might be used uninitialized in this function

>security/manager/ssl/src/nsNSSCertificate.cpp:3206
> `enum SECStatus sec_rv' might be used uninitialized in this function

Let's use Stephane's patch.

>security/manager/ssl/src/nsNSSIOLayer.cpp:695
> `PRBool oldBlockVal' might be used uninitialized in this function

No fix needed.
I have no good idea for a default value...

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 64533 [details] [diff] [review]
patch to nsNSSCertificate.cpp

Changes look good, but I think one more change might make sense? You seem to
want to return an error whenever something goes wrong in the method. You could
assign the error value after failure of ImportCertForKey, too.
With that addition, r=kaie.

Comment 5

[Kai Engert (:KaiE:)]

Stephane, re your q whether content handlers had been moved to pipboot, they
have not, they are still in pipnss.

Comment 6

[Aleksey Nogin]

> >security/manager/ssl/src/nsCrypto.cpp:1841
> > `int numResponses' might be used uninitialized in this function
>
> This warning is wrong, the first usage is assignment.

Not quite - there are all those "goto loser" that can go around it. Still, in
the "loser" part numResponses will be used only if certArt is non-null and that
can only happened if numResponses was already assigned...

Comment 7

[Kai Engert (:KaiE:)]

> > This warning is wrong, the first usage is assignment.
>
> Not quite - there are all those "goto loser" that can go around it. Still, in
> the "loser" part numResponses will be used only if certArt is non-null and that
> can only happened if numResponses was already assigned...

Ok, you are right - that oversight from me is one of the reasons why I don't
like goto statements at all. I wish our code didn't have any.

So, let's init numResponses to zero.

Comment 8

[Stephane Saux]

Attachment: updated patch to nsNSSCertificate

Found a few more places where we should return failure:
arena alloc fails, etc... Basically everywhere where we goto,
I set an error.

Kai, I would prefer is you r= this explicitely, since I made more changes than
you requested.

Comment 9

[Stephane Saux]

Attachment: Updated patch which compile.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 64595 [details] [diff] [review]
Updated patch which compile.

I give you
  r=kaie
because I think the patch works.

However, I now realize that we could have made that patch much simpler. Instead
of setting a default of SECSuccess, we could have set it to SECFailure. This
would avoid most changes in failure situations, as there is only one placed
where we detect success.

And instead of using SECStatus everywhere, we could have used the method-global
variable
  nsresult rv = NS_ERROR_FAILURE;
instead, and only if sec_rv is assigned with the value we expect, we could
simply assigned NS_OK. Doing that, the method return statement were a simple
  return rv;

Comment 11

[Stephane Saux]

requested sr from kin

Comment 12

[kinmoz]

I actually prefer Kai's suggestion since it's less code bloat:


Index: security/manager/ssl/src/nsNSSCertificate.cpp
===================================================================
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificate.cpp,v
retrieving revision 1.62
diff -u -r1.62 nsNSSCertificate.cpp
--- security/manager/ssl/src/nsNSSCertificate.cpp       6 Feb 2002 20:38:58 -000
0       1.62
+++ security/manager/ssl/src/nsNSSCertificate.cpp       14 Feb 2002 00:25:01 -00
00
@@ -3229,7 +3229,7 @@
 {
   PK11SlotInfo *slot;
   char * nickname = NULL;
-  SECStatus sec_rv;
+  nsresult rv = NS_ERROR_FAILURE;
   int numCACerts;
   SECItem *CACerts;
   CERTDERCerts * collectArgs;
@@ -3282,7 +3282,8 @@

   if (numCACerts) {
        CACerts = collectArgs->rawCerts+1;
-    sec_rv = CERT_ImportCAChain(CACerts, numCACerts, certUsageUserCertImport);
+    if (!CERT_ImportCAChain(CACerts, numCACerts, certUsageUserCertImport))
+      rv = NS_OK;
   }

 loser:
@@ -3290,7 +3291,7 @@
     PORT_FreeArena(arena, PR_FALSE);
   }
   CERT_DestroyCertificate(cert);
-  return (sec_rv) ? NS_ERROR_FAILURE : NS_OK;
+  return rv;
 }

 /*

Comment 13

[John Unruh]

nsbeta1

Comment 14

[Stephane Saux]

we have a patch, there's no reason why we shouldn't get it in.

Comment 15

[Stephane Saux]

Attachment: Updated patch for file nsNSSCertificate.cpp

Incorporates Kin's suggestions.

Comment 16

[Aleksey Nogin]

Bug 126473 fixed all the "foundInterface" warnings, so now security/manager only
has the following "xxx moght be used uninitialized" warnings:

security/manager/pki/src/nsASN1Outliner.cpp:61
 `PRInt32 rowsToDelete' might be used uninitialized in this function

security/manager/ssl/src/nsCrypto.cpp:1839
 `int numResponses' might be used uninitialized in this function

security/manager/ssl/src/nsKeygenHandler.cpp:115
 `unsigned char * buf' might be used uninitialized in this function

security/manager/ssl/src/nsNSSCertificate.cpp:3235
 `enum SECStatus sec_rv' might be used uninitialized in this function

security/manager/ssl/src/nsNSSIOLayer.cpp:999
 `PRBool oldBlockVal' might be used uninitialized in this function

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 74471 [details] [diff] [review]
Updated patch for file nsNSSCertificate.cpp

This patch has r=kaie and sr=kin

Comment 18

[Kai Engert (:KaiE:)]

Attachment: Patch for other warnings

This patch fixes the warnings in files except nsNSSCertificate.cpp.
Actually I now think, the code in files nsCrypto and nsKeygenHandler could
cause trouble, and this patch will avoid trouble.

Comment 19

[Kai Engert (:KaiE:)]

Javi, can you please review the second patch?

Comment 20

[Javier Delgadillo]

Comment on attachment 78486 [details] [diff] [review]
Patch for other warnings

r=javi

Comment 21

[kinmoz]

Comment on attachment 78486 [details] [diff] [review]
Patch for other warnings

sr=[e-mail]

Comment 22

[Kai Engert (:KaiE:)]

Nominating adt1.0.0, also sent mail to drivers.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 74471 [details] [diff] [review]
Updated patch for file nsNSSCertificate.cpp

a=[e-mail].  Don't forget to check into trunk too.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 78486 [details] [diff] [review]
Patch for other warnings

a=[e-mail].  Don't forget to check into trunk too.

Comment 25

[Kai Engert (:KaiE:)]

ADT1, trivial correctness fixes.

Comment 26

[Kai Engert (:KaiE:)]

Checked in to the trunk. Bug stays open until we land it on the branch.

Comment 27

[Aleksey Nogin]

According to
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1018505460.2351.gz&fulltext=1
(Thu, 11 Apr 2002 02:11 EST) there are no longer any "might be used
uninitialized" warnings in security/manager on the trunk.

Where did vtrunk keyword go?

Comment 28

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADTs behalf) checkin to 1.0. Pls check this into the trunk and 1.0
branch, pending positive feedback from junruh.

Comment 29

[John Unruh]

Using the 4/12 Win2000 trunk build, I have not seen any new regressions after 
running through the acceptance tests.

Comment 30

[Kai Engert (:KaiE:)]

Checked in to the branch, fixed.

Comment 31

[John Unruh]

Verified on branch and trunk. Removing all keywords. adt1.0.0+, verified1.0.0, 
nsbeta1+, topembed+

Comment 32

[Asa Dotzler [:asa]]

adding verified1.0.0 keyword (branch verification) based on bonsai checkin and
QA comments.