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

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect)

Description

[Aleksey Nogin]

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

Comment 1

[Aleksey Nogin]

Bug 59652 is the meta-bug tracking the fight against these (potentially very
nasty) warnings; bug 59673 is for all such warnings in mailnews.

Comment 2

[Aleksey Nogin]

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

Comment 3

[Aleksey Nogin]

Attachment: Fix some warnings in nsVCard.cpp and mimecryp.cpp

This fixes the first two "might be used uninitialized" warnings as well as
fixing couple of other warnings in those files.

Comment 4

[Jean-Francois Ducarroz]

Comment on attachment 84374 [details] [diff] [review]
Fix some warnings in nsVCard.cpp and mimecryp.cpp

Looks good. R=ducarroz

Comment 5

[Jean-Francois Ducarroz]

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 6

[David :Bienvenu]

Comment on attachment 84374 [details] [diff] [review]
Fix some warnings in nsVCard.cpp and mimecryp.cpp

sr=bienvenu, if you fix the nulls

Comment 7

[Shanjian Li]

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. 

Comment 8

[Aleksey Nogin]

Attachment: Fix some warnings in nsVCard.cpp and mimecryp.cpp, v2

NULL -> nsnull

Comment 9

[Aleksey Nogin]

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!

Comment 10

[Jean-Francois Ducarroz]

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

Comment 11

[Aleksey Nogin]

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!

Comment 12

[Shanjian Li]

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.

Comment 13

[Aleksey Nogin]

Attachment: mimetext.cpp fix

Fixing it as suggested by Shanjian.

Comment 14

[Shanjian Li]

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

r=shanjian

Comment 15

[Jean-Francois Ducarroz]

Patch V2 has been checked in the trunk

Comment 16

[Jean-Francois Ducarroz]

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

R=ducarroz

Comment 17

[Scott MacGregor]

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

sr=mscott

Comment 18

[Jean-Francois Ducarroz]

Patch V3 has been checked in the trunk

Comment 19

[[e-mail] (gone - use [e-mail] instead)]

According to http://tinderbox.mozilla.org/SeaMonkey/warn1022032500.6496.html,
this is fixed.

Verified.

Comment 20

[Aleksey Nogin]

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.

Comment 21

[[e-mail] (gone - use [e-mail] instead)]

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;

Comment 22

[Aleksey Nogin]

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?