Closed Bug 119481 Opened 18 years ago Closed 18 years ago

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

Categories

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

Other Branch
x86
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED
psm2.2

People

(Reporter: mozilla-bugs, Assigned: kaie)

References

Details

(Whiteboard: [adt1])

Attachments

(2 files, 3 obsolete files)

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
Blocks: 59652
Attached patch patch to nsNSSCertificate.cpp (obsolete) — Splinter Review
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.
kai
Assignee: ssaux → kaie
Priority: -- → P1
Target Milestone: --- → 2.2
>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 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.
Stephane, re your q whether content handlers had been moved to pipboot, they
have not, they are still in pipnss.
> >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...
> > 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.
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.
Attached patch Updated patch which compile. (obsolete) — Splinter Review
Attachment #64533 - Attachment is obsolete: true
Attachment #64594 - Attachment is obsolete: true
Attachment #64595 - Flags: review+
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;
requested sr from kin
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;
 }

 /*
nsbeta1
Keywords: nsbeta1
we have a patch, there's no reason why we shouldn't get it in.
Keywords: nsbeta1nsbeta1+
Keywords: patch, review
Incorporates Kin's suggestions.
Attachment #64595 - Attachment is obsolete: true
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
Summary: Occurances of uninitialized variables being used before being set. → Occurances of uninitialized variables being used before being set (in security/manager).
Comment on attachment 74471 [details] [diff] [review]
Updated patch for file nsNSSCertificate.cpp

This patch has r=kaie and sr=kin
Attachment #74471 - Flags: superreview+
Attachment #74471 - Flags: review+
Attachment #74471 - Attachment description: Updated patch → Updated patch for file nsNSSCertificate.cpp
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.
Javi, can you please review the second patch?
Comment on attachment 78486 [details] [diff] [review]
Patch for other warnings

r=javi
Attachment #78486 - Flags: review+
Comment on attachment 78486 [details] [diff] [review]
Patch for other warnings

sr=kin@netscape.com
Attachment #78486 - Flags: superreview+
Nominating adt1.0.0, also sent mail to drivers.
Keywords: adt1.0.0
Comment on attachment 74471 [details] [diff] [review]
Updated patch for file nsNSSCertificate.cpp

a=rjesup@wgate.com.  Don't forget to check into trunk too.
Attachment #74471 - Flags: approval+
Comment on attachment 78486 [details] [diff] [review]
Patch for other warnings

a=rjesup@wgate.com.  Don't forget to check into trunk too.
Attachment #78486 - Flags: approval+
ADT1, trivial correctness fixes.
Status: NEW → ASSIGNED
Whiteboard: [adt1]
Checked in to the trunk. Bug stays open until we land it on the branch.
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?
Keywords: patch, review
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.
Keywords: adt1.0.0adt1.0.0+, topembed
Keywords: topembedtopembed+
Using the 4/12 Win2000 trunk build, I have not seen any new regressions after 
running through the acceptance tests.
Checked in to the branch, fixed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED
Verified on branch and trunk. Removing all keywords. adt1.0.0+, verified1.0.0, 
nsbeta1+, topembed+
Status: RESOLVED → VERIFIED
adding verified1.0.0 keyword (branch verification) based on bonsai checkin and
QA comments.
Keywords: verified1.0.0
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.