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

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

>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.

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

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

Incorporates Kin's suggestions.

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 on attachment 74471 [details] [diff] [review] Updated patch for file nsNSSCertificate.cpp This patch has r=kaie and sr=kin

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

Comment on attachment 78486 [details] [diff] [review] Patch for other warnings sr=kin@netscape.com

Nominating adt1.0.0, also sent mail to drivers.

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.

Comment on attachment 78486 [details] [diff] [review] Patch for other warnings a=rjesup@wgate.com. Don't forget to check into trunk too.

ADT1, trivial correctness fixes.

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?

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.

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.

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

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