Closed Bug 132163 Opened 18 years ago Closed 18 years ago

Occurances of uninitialized variables being used before being set (in mailnews/mime)

Categories

(MailNews Core :: MIME, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.1beta

People

(Reporter: mozilla-bugs, Assigned: bugzilla)

References

Details

Attachments

(2 files, 1 obsolete file)

This bug is just for the "xxx might be used uninitialized" warnings in various
source files in mailnews/mime directory.
Currently (http://tinderbox.mozilla.org/SeaMonkey/warn1016552220.1515.html -
Tue, 19 Mar 2002 10:37 EST) TBox shows the following warnings:

mailnews/mime/cthandlers/vcard/nsVCard.cpp:502
 `const char * p2' might be used uninitialized in this function

mailnews/mime/src/mimecryp.cpp:461
 `struct MimeHeaders * outer_headers' might be used uninitialized in this function

mailnews/mime/src/mimetext.cpp:433
 `nsresult res' might be used uninitialized in this function

mailnews/mime/src/mimetext.cpp:434
 `int status' might be used uninitialized in this function

mailnews/mime/src/mimetpfl.cpp:228
 `struct MimeInlineTextPlainFlowedExData * exdata' might be used uninitialized
in this function
Bug 59652 is the meta-bug tracking the fight against these (potentially very
nasty) warnings; bug 59673 is for all such warnings in mailnews.
Blocks: 59652, 59673
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1beta
The mimetext.cpp warnings seem to indicate a real problem in the
MimeInlineText_open_dam function.

First, if text->curDamOffset <= 0 and length <=0, the res will stay
uninitialized and in line 452 we will do NS_SUCCEEDED(res) while it is still
uninitialized.

Second, if both of them (text->curDamOffset and length) are actually 0, then
this function would not do anything at all, except free some stuff and set some
stuff to nsnull and then return an uninitialized int!

Finally, the following code looks very suspicious:

444  if (text->curDamOffset > length)
445    res = MIME_detect_charset(text->lineDamBuffer, text->curDamOffset,
&detectedCharset);  
446  else 
447    res = MIME_detect_charset(text->lineDamBuffer, text->curDamOffset,
&detectedCharset);  

since in both branches of the "if" the code is identical! 

See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/mimetext.cpp&mark=433,434,444-447,452,489#428
This fixes the first two "might be used uninitialized" warnings as well as
fixing couple of other warnings in those files.
Keywords: patch, review
Comment on attachment 84374 [details] [diff] [review]
Fix some warnings in nsVCard.cpp and mimecryp.cpp

Looks good. R=ducarroz
Attachment #84374 - Flags: review+
Comment on attachment 84374 [details] [diff] [review]
Fix some warnings in nsVCard.cpp and mimecryp.cpp

oops, should use nsnull instead of NULL. No need to continue to use that old
stuff.
Comment on attachment 84374 [details] [diff] [review]
Fix some warnings in nsVCard.cpp and mimecryp.cpp

sr=bienvenu, if you fix the nulls
Attachment #84374 - Flags: superreview+
reply to #2,
Both of your points are correct. There is no point to compare
"text->curDamOffset" and "length". The if statement should be removed. 
NULL -> nsnull
Attachment #84374 - Attachment is obsolete: true
Comment on attachment 84457 [details] [diff] [review]
Fix some warnings in nsVCard.cpp and mimecryp.cpp, v2

Carrying forward the sr=bienvenu. 

Jean-Francois, could you please re-review and commit this? Thanks a lot!
Attachment #84457 - Flags: superreview+
Comment on attachment 84457 [details] [diff] [review]
Fix some warnings in nsVCard.cpp and mimecryp.cpp, v2

R=ducarroz. I'll check it in as soon the tree open...
Attachment #84457 - Flags: review+
Shanjian, besides removing the "if" - how should the text->curDamOffset <= 0 and
length <=0 cases be handled (e.g. what is the correct return value there)? Thanks!
default status to 0 and res to NS_OK in the time when those variable are
declared. Nothing can be wrong if nothing has been done.
Attached patch mimetext.cpp fixSplinter Review
Fixing it as suggested by Shanjian.
Comment on attachment 84477 [details] [diff] [review]
mimetext.cpp fix

r=shanjian
Attachment #84477 - Flags: review+
Patch V2 has been checked in the trunk
Comment on attachment 84477 [details] [diff] [review]
mimetext.cpp fix

R=ducarroz
Comment on attachment 84477 [details] [diff] [review]
mimetext.cpp fix

sr=mscott
Attachment #84477 - Flags: superreview+
QA Contact: esther → stephend
Patch V3 has been checked in the trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Well, it's not really fixed - only 4 warnings out of five were silenced with the
mimetpfl.cpp warning still there (see the 31-th one under
http://tinderbox.mozilla.org/SeaMonkey/warn1022032500.6496.html#rhp ). I guess
that's OK - we still have the bug 59673 for warnings in all the mailnews.
This one?

Sorry, I missed this somehow.

31.mailnews/mime/src/mimetpfl.cpp:228 (See build log excerpt)`struct
MimeInlineTextPlainFlowedExData * exdata' might be used uninitialized in this
function

226 // We do it in the beginning so that if an error occur, we can
227 // just free |exdata|.
228 struct MimeInlineTextPlainFlowedExData *exdata;
229 struct MimeInlineTextPlainFlowedExData **prevexdata;
230 prevexdata = &MimeInlineTextPlainFlowedExDataList;
Yes, that one -
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/mimetpfl.cpp&mark=228,232#207
. I haven't included it in my other patches since I do not understand why gcc
thinks that 

while ((exdata = *prevexdata) != nsnull)

can leave exdata uninitialized. Is it a gcc bug? Or am I missing something?
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.