281066 - removing some memory allocations from mimedrft.cpp

Status: RESOLVED WORKSFORME

(MailNews Core :: MIME, defect)

Description

[Tim Ruehsen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) KHTML/3.3.2 (like Gecko)
Build Identifier: 

Mainly replacing calls to NS_MsgSACat and NS_MsgSACopy by PR_smprintf and 
PL_strdup. 
Rewrote mime_fix_up_html_address which was buggy. 
Found one PL_strncpy usage without terminating (in MimeGetNamedString()). 
Replaced some PR_Free() by PR_FREEIF(). 
 
 
 

Reproducible: Always

Comment 1

[Tim Ruehsen]

Attachment: cvs diff for mimedrft.cpp

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Please request reviews from the appropriate peers listed at
http://www.mozilla.org/owners.html

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

I'm not sure if Kaie is still doing mail reviews.  Perhaps you could take a look
at this, David?

Comment 4

[David :Bienvenu]

the difference between PR_FREEIF and PR_Free is that PR_FREEIF nulls out the
resulting pointer. I don't believe you need that behaviour (PR_Free also checks
for null).

Can I get a -uw diff so I don't have to see the whitespace-only changes?

Comment 5

[Tim Ruehsen]

Attachment: cvs diff for mimedrft.cpp

Hey, that w option is what I needed. Formerly, I removed these line by hand.
After all, we are talking about 2 occurrences in a total of ~150...sigh.

In some cases I use PR_FREEIF not because it tests the pointer but because it
sets it to 0 after freeing. For debugging/security reasons this should be done
wherever it can. The cost for this is very small and using a 0 pointer would
make the app crash immediately and not somewhere else, which makes it hard to
track down the real problem.
But doing so seem to disturb some people (typical arguments are: 'hey, are you
a girl or what?' ;-).

Comment 6

[timeless]

PR_DELETE()

Comment 7

[Tim Ruehsen]

Attachment: cvs diff for mimedrft.cpp

I replaced some PR_FREEIF by PR_Free or PR_DELETE where it was obvious that it
could be done.
But PR_Free does not check for null (see prmem.c).
My opinion still is: we should ONLY use PR_FREEIF and make PR_Free and
PR_DELETE obsolete. This is because here and there PR_Free and PR_DELETE are
used even when it is not obviously the right choice (or even definitely wrong).
This may come from copy&paste and/or inattentiveness. PR_FREEIF would really
help.

I just realized that there where some syntax errors in the patches before. They
are fixed by now. Still there are some warnings like
   'converting of negative value `-1000' to `unsigned'
e.g. in function mime_decompose_file_init_fn

Comment 8

[David :Bienvenu]

the underlying c library free checks for null. Nulling out a pointer inside a
memory block that you're about to free anyway is usually a waste of code size
and cpu cycles, unless the information is scary to leave in memory, like a password.

Comment 9

[Tim Ruehsen]

ok. anything blocking the patch from going into cvs? 
 

Comment 10

[David :Bienvenu]

Comment on attachment 174674 [details] [diff] [review]
cvs diff for mimedrft.cpp

+  if (tmp)
+  {
+     PR_FREEIF(*body);
+     *body = tmp;
+  }

this should just be:

...
PR_Free(*body);

similarly,

-      PR_FREEIF(result);
     }
+    PR_FREEIF(result);

PR_Free(result);

etc. PR_FREEIF also stores a null in the resulting pointer, which we don't
need.

Comment 11

[Tim Ruehsen]

#10: this is a question of flavour (maybe experience?) To avoid possible issues 
with future copy&paste or changes in the code, I *personally* would always use 
PR_FREEIF(). This is generally called 'defensive'. In fact, I saw it happen 
'often', that little changes on 'perfect' code lead to problems with 'freed 
twice' allocations (among many other possible problems). 
So, no. I disagree to change that. But I won't try to argue if you insist on 
the changes. I am a bit out of the mozilla project. 
 

Comment 12

[Jean-Francois Ducarroz]

Comment on attachment 174674 [details] [diff] [review]
cvs diff for mimedrft.cpp

Very good cleanup. But you are leaking the utf8 variable in
mime_intl_insert_message_header_1

Comment 13

[Tim Ruehsen]

Thanks, Jean-Francois. 
There should be a PR_FREEIF(utf8) or PR_Free(utf8) at the end of 
mime_intl_insert_message_header_1(). 
 
Could someone insert it, please. I am out of mozilla for a while and have no 
time to work into it at the moment. 
 

Comment 14

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 174674 [details] [diff] [review]
cvs diff for mimedrft.cpp

Patch has bitrotted. Tim, up for a new patch? :)

$ patch -p0 --dry-run < ~/Desktop/p281066.diff 
patching file mimedrft.cpp
Hunk #1 FAILED at 573.
Hunk #2 FAILED at 649.
Hunk #3 FAILED at 895.
Hunk #4 succeeded at 937 with fuzz 2 (offset -22 lines).
Hunk #5 FAILED at 1046.
Hunk #6 FAILED at 1090.
Hunk #7 FAILED at 1121.
Hunk #8 succeeded at 1183 (offset -20 lines).
Hunk #9 FAILED at 1314.
Hunk #10 FAILED at 1424.
Hunk #11 FAILED at 1504.
Hunk #12 FAILED at 1531.
Hunk #13 FAILED at 1562.
Hunk #14 FAILED at 1722.
Hunk #15 succeeded at 1816 with fuzz 2 (offset 17 lines).
Hunk #16 FAILED at 1841.
Hunk #17 FAILED at 1921.
14 out of 17 hunks FAILED -- saving rejects to file mimedrft.cpp.rej

Comment 16

[:aceman]

Is this patch still relevant?Should I update it?

What needs to be done? Are comment 10 and comment 13 the only problems with it?
David, should all changes of PR_Free to PR_FREEIF be reverted (removed in the patch)? Or do you object only to some specific occurrences?

Comment 17

[David :Bienvenu]

(In reply to :aceman (away 16.-21.) from comment #16)
> Is this patch still relevant?Should I update it?
No, I don't think so. I made a bunch of changes to mimedrft.cpp to clean up the string usage (using nsCString, which allows for much cleaner memory mgmt), so it's unlikely the patch is relevant.

Comment 18

[:aceman]

But there are many other changes, like NS_MsgSACopy removal. Is nothing from it usable?

Comment 19

[David :Bienvenu]

(In reply to :aceman (away 16.-21.) from comment #18)
> But there are many other changes, like NS_MsgSACopy removal. Is nothing from
> it usable?

No, I think the code has changed so much that you'd have to start over from the beginning.

Comment 20

[:aceman]

It seems this whole bug is filed for making the changes in the attached patch. If that is not relevant anymore, probably the whole bug is not needed now?

Comment 21

[:aceman]

Wayne? Any idea with this bug?

Comment 22

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #20)
> It seems this whole bug is filed for making the changes in the attached
> patch. If that is not relevant anymore, probably the whole bug is not needed
> now?

I don't know, but it sounds like ...