Last Comment Bug 717407 - add null-arg checks and cleanup in mailnews/mime/src
: add null-arg checks and cleanup in mailnews/mime/src
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 412109
  Show dependency treegraph
 
Reported: 2012-01-11 14:14 PST by :aceman
Modified: 2012-03-07 16:48 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
null checks, whitespace cleanup and NS_ENSUREs (23.34 KB, patch)
2012-01-18 14:25 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (23.89 KB, patch)
2012-03-06 11:38 PST, :aceman
neil: review+
Details | Diff | Splinter Review

Description :aceman 2012-01-11 14:14:46 PST
I'll try to do some null-arg check in files of that directory. Also there are some possibilities to optimize 'if (!unlikely problem) return ERR' to 'NS_ENSURE_TRUE(unlikely problem, ERR)'.

Also, I'll add these from bug 413548:
--- a/mailnews/mime/src/nsMimeConverter.cpp	Tue Jan 22 16:02:08 2008 -0500
+++ b/mailnews/mime/src/nsMimeConverter.cpp	Tue Jan 22 16:31:14 2008 -0500
@@ -136,7 +136,8 @@ nsMimeConverter::EncodeMimePartIIStr(con
                                            PRInt32 encodedWordSize, 
                                            char          **encodedString)
 {
-
+  NS_ENSURE_ARG_POINTER(header);
+  NS_ENSURE_ARG_POINTER(mailCharset);
   // Encoder needs utf-8 string.
   nsAutoString tempUnicodeString;
   nsresult rv = ConvertToUnicode(mailCharset, header, tempUnicodeString);
Comment 1 :aceman 2012-01-18 14:25:55 PST
Created attachment 589647 [details] [diff] [review]
null checks, whitespace cleanup and NS_ENSUREs
Comment 2 neil@parkwaycc.co.uk 2012-03-04 13:42:02 PST
Comment on attachment 589647 [details] [diff] [review]
null checks, whitespace cleanup and NS_ENSUREs

>+  NS_ENSURE_ARG_POINTER(mailCharset);
Are you sure mailCharset can't be null? (I think it just defaults to ASCII.)

>-    char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
>+  char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
[This should actually use NS_Alloc. Sadly all_headers isn't null-terminated, so we can't simply clone it.]

>+  NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory");
>+  NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY);
Not sure we need both checks here.

> static int
> Initialize(MimeObject *obj)
So, the rest of the file has a 4 space indent, which is fair enough, except that you changed this function from a 2 space indent to a weird 4+2 indent...
Comment 3 :aceman 2012-03-05 12:35:21 PST
(In reply to neil@parkwaycc.co.uk from comment #2)
> >+  NS_ENSURE_ARG_POINTER(mailCharset);
> Are you sure mailCharset can't be null? (I think it just defaults to ASCII.)
No idea, I took this from Joey Minta's patch in bug 413548.
I can remove it.

> >-    char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
> >+  char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
> [This should actually use NS_Alloc. Sadly all_headers isn't null-terminated,
> so we can't simply clone it.]
I assume I should not do anything about this.

> >+  NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory");
> >+  NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY);
> Not sure we need both checks here.
Who will decide? :)
Comment 4 neil@parkwaycc.co.uk 2012-03-06 08:31:48 PST
(In reply to aceman from comment #3)
> (In reply to comment #2)
> > >+  NS_ENSURE_ARG_POINTER(mailCharset);
> > Are you sure mailCharset can't be null? (I think it just defaults to ASCII.)
> No idea, I took this from Joey Minta's patch in bug 413548.
Ah, well not all null in arguments are invalid. This was the only one that I spotted though; I guess if you mistakenly trap other null in arguments then exceptions will get thrown...

> > >-    char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
> > >+  char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
> > [This should actually use NS_Alloc. Sadly all_headers isn't null-terminated,
> > so we can't simply clone it.]
> I assume I should not do anything about this.
Well, I would appreciate it if you switched from PR_MALLOC to NS_Alloc.

> > >+  NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory");
> > >+  NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY);
> > Not sure we need both checks here.
> Who will decide? :)
Well, unlike the previous code, NS_ENSURE_TRUE does log an error to stdout, which might make the assertion less useful.
Comment 5 :aceman 2012-03-06 10:21:41 PST
(In reply to neil@parkwaycc.co.uk from comment #4)
> (In reply to aceman from comment #3)
> > (In reply to comment #2)
> > > >+  NS_ENSURE_ARG_POINTER(mailCharset);
> > > Are you sure mailCharset can't be null? (I think it just defaults to ASCII.)
> > No idea, I took this from Joey Minta's patch in bug 413548.
> Ah, well not all null in arguments are invalid. This was the only one that I
> spotted though; I guess if you mistakenly trap other null in arguments then
> exceptions will get thrown...
Yes, I try to only check OUT parameters that are dereferenced and would cause a crash.
only this 'mailCharset' and 'header' IN parameters are checked because Joey had it that was. I can drop those 2.


> > > >-    char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
> > > >+  char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1);
> > > [This should actually use NS_Alloc. Sadly all_headers isn't null-terminated,
> > > so we can't simply clone it.]
> > I assume I should not do anything about this.
> Well, I would appreciate it if you switched from PR_MALLOC to NS_Alloc.
If it is just replacing the function name I can do that.

> > > >+  NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory");
> > > >+  NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY);
> > > Not sure we need both checks here.
> > Who will decide? :)
> Well, unlike the previous code, NS_ENSURE_TRUE does log an error to stdout,
> which might make the assertion less useful.
Ok, I'll remove it.
Comment 6 :aceman 2012-03-06 11:38:11 PST
Created attachment 603387 [details] [diff] [review]
patch v2
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-03-07 16:48:42 PST
http://hg.mozilla.org/comm-central/rev/1bb9b9a58eea

BTW Neil, the Modules wiki doesn't list you as a peer for MailNews Core::MIME. You should probably update that.
https://wiki.mozilla.org/Modules/MailNews_Core

Note You need to log in before you can comment on or make changes to this bug.