Closed Bug 59652 Opened 19 years ago Closed 8 years ago
Numerous places where uninitialized variables might be being used before being set
37.07 KB, text/plain
4.00 KB, text/plain
1.38 KB, patch
|Details | Diff | Splinter Review|
14.40 KB, text/plain
We have seen a couple of bugs (58698 and 59617) where crashes have resulted because variables have been used without being previously initialized. By building Mozilla (with mail/news) with the -Wuninitialized flag and with optimization turned on, I was able to find 192 occurances of this in total (in 109 files). These problems could be the cause of many "random" crashes on platforms which don't automatically initialize local variables to zero (Solaris being one of them). See the first attachment, which contains the compile transcript for all of these 109 files. Even though Netscape 6 RTM is almost out the door, we would like to fix up these problems for our Solaris Netscape 6 RTM/FCS release, so if diffs could be attached to this bug for the various fixes (note the bugs 58698 and 59617 already contain two such sets of diffs), then it would be very much appreciated. Thanks.
The best would be if you could split the log between different modules and file a bug for each. My experience of bugs like this that covers many components is that noone takes it on.
I'm not sure about modules, but you should assign the db/mork stuff to bienvenu extensions/wallet to morse docshell to adamlock
Summary: Numerous places where uninitialized variables must be being used before being set. → [TRACKING] Numerous places where uninitialized variables must be being used before being set.
Thanks Seth. I've added bienvenu, morse and adamlock to the CC: of this bug. I've already opened up enough others bugs for this one. Guys, with reference to the previous comment by Seth, could you please attach diifs (assuming they are needed), to fix the uninitialized variables for the files that you own? Thanks.
Besides the extensions/wallet/singsign.cpp cases, I'm also responsible for the one in extensions/cookie/nsCookie.cpp. I just checked the two cases in singsign and the one case in nsCookie and none are a problem. Here's why (same reason in all three cases). It's true that the variables in question are not initialized when declared but rather are initialized inside a loop. They are then used after the loop terminates. So the compiler would have no way of knowing if the loop was ever executed and therefore considers these as potentially uninitialized. However, inside the loop another variable changes value. And after the loop, a test is made for that variable changing value and the routine is exited if it didn't happen. Therefore the place that the questionable variable is used will never get executed if the loop had never been entered. Bottom line -- there is no problem here in the nsCookie or singsign files.
Correction: Above I said the two cases in singsign and one in nsCookie. That should read the other way around -- one case in singsign and two in nsCookie.
morse: you should still fix the warnings to get the off the radar.
Assignee: asa → chofmann
Component: Browser-General → Tracking
QA Contact: doronr → chofmann
Summary: [TRACKING] Numerous places where uninitialized variables must be being used before being set. → Numerous places where uninitialized variables must be being used before being set.
Added attachment that contains comments regarding the warnings.
Changed summary to reflect what the warnings really say.
Summary: Numerous places where uninitialized variables must be being used before being set. → Numerous places where uninitialized variables might be being used before being set.
Cc'ing mang for advice on the fdlibm warnings and scary-looking unbraced but indented as if written in Python code. From my attachment: e_asin.c:110: warning: `t' might be used uninitialized in this function - will get assigned before use. *** Agreed, but the indentation without bracing at lines 125-129 looks very *** wrong. This fdlibm code originated from sun.com, could you please see *** whether a bug needs to be filed there? Thanks. e_exp.c:143: warning: `hi' might be used uninitialized in this function e_exp.c:143: warning: `lo' might be used uninitialized in this function e_exp.c:144: warning: `k' might be used uninitialized in this function - might need to be init. possible to get used without first setting. *** agreed, these are bugs. This fdlibm code originated from sun.com, can *** you see whether a bug needs to be filed there? Thanks. e_rem_pio2.c:124: warning: `z' might be used uninitialized in this function - may get used at line 199 before getting assigned. need to be init/set! e_sqrt.c:132: warning: `z' might be used uninitialized in this function - may get used at line 224 before getting assigned. need to be init/set! k_cos.c:105: warning: `qx' might be used uninitialized in this function - same as the above two, hmm ... are these relying on some runtime stack values? it seems pretty dangerous not to set/init these, but on the other hand, it seems like they are intended. *** The use of qx on line 120 in k_cos.c is clearly wrong, as the next two *** statements completely overwrite u.d. This fdlibm code originated from *** sun.com, could you please see whether a bug needs to be filed there? *** In any case, the JS engine's copy should be fixed to strike line 120. *** Same for the e_sqrt.c and e_rem_pio2.c uses. Who at Sun can follow up with fdlibm authors? George? /be
Hi Brendan: I don't know what fdlibm is, but I'd be happy to try to track it down. What is it? Also, can you tell me some of its history? Maybe I can go from there and track down the authors. Or if you even know a Sun name (probably too much to ask, but I gotta try), I can go from there.
- bug 81851 was caused by an uninitialized nsresult. - bug 84477 "nsCachePrefObserver::Install can return an uninitialized nsresult" - bug 84498 "xpidl_process_idl may return unitialized value" - bug 84508 "Tinderbox should compile with -O on Linux to produce -Wuninitialized warnings." - attachment 37644 [details] to bug 84508 gives a listing of _243_ uninitialized variables in an up-to-date build. This means that the number more than doubled since this bug was filed!
Another good practice is for each engineer to turn warnings all they way up and do a build of his or her area. I've often find numerous problems in code by doing this in this past. There's plenty of noise, but often there are valid warnings that point to bugs.
Bug 53593 "uninitialized variables in jsj.c" I filed a few new NSS bugs: Bug 95982 "rsa.c: RSA_PrivateKeyOp will return uninitialized SECStatus on success" Bug 95983 "jarver.c: uninitialized variables might be being used before being set" Bug 95989 "certdb/genname.c: cert_CompareNameWithConstraints handles certURI case incorrectly" Bug 95990 "rijndael.c: encrypt/decrypt use c2 and c3 without ever initializing" Unfortunatelly, there are much more of these in NSS...
The number of these warnings seems to be finally going down (http://tinderbox.mozilla.org/SeaMonkey/warn1010614620.16740.html has 269 of them), but very slowly. It's very likely that some of those 269 are real bugs and IMHO it would make sence to try to make sure that do not make it into 1.0
These are the "may be uninitialized" warnings listed in http://tinderbox.mozilla.org/SeaMonkey/warn1010629440.4199.html
Comment on attachment 64819 [details] [diff] [review] fix for warnings in cookie and wallet code sr=alecf
Attachment #64819 - Flags: superreview+
cookie and wallet fixes are now checked in. Removing myself from cc list.
These are the warnings from http://tinderbox.mozilla.org/SeaMonkey/warn1013026020.23864.html (Wed, 06 Feb 2002 15:07 EST). The NSS3.4 ladning got rid of a huge number of warnings, so now we only have 190 of them left!
Attachment #64250 - Attachment is obsolete: true
Distribution by toplevel directory: 46 layout 34 js 16 nsprpub 13 mailnews 10 security 10 extensions 10 content 8 xpfe 6 widget 5 xpinstall 4 xpcom 4 modules 2 string 2 gfx 2 db 1 uriloader 1 rdf 1 intl 1 embedding
Attachment #68209 - Attachment is obsolete: true
26 down, 150 to go...
Attachment #70722 - Attachment is obsolete: true
17 years ago
Distribution of the lines with warnings, by toplevel dir: 31 layout 28 js 14 content 13 nsprpub 11 directory 4 xpcom 4 widget 4 modules 3 security 3 mailnews 3 gfx 2 tools 2 string 2 extensions 1 xpfe 1 rdf 1 profile 1 intl
Attachment #75073 - Attachment is obsolete: true
*** Bug 132145 has been marked as a duplicate of this bug. ***
*** Bug 132141 has been marked as a duplicate of this bug. ***
All the dependent bugs has been closed. Hence closing the meta bug as well.
Assignee: chofmann → atulagrwl
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.