Last Comment Bug 707306 - add null-arg checks in nsMsgFilter.cpp, nsMsgFilterList.cpp, nsMsgSearchAdapter.cpp
: add null-arg checks in nsMsgFilter.cpp, nsMsgFilterList.cpp, nsMsgSearchAdapt...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 11.0
Assigned To: :aceman
:
Mentors:
Depends on: 735940
Blocks: 412109 755658 713429
  Show dependency treegraph
 
Reported: 2011-12-02 13:49 PST by :aceman
Modified: 2012-05-16 02:21 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
suggested patch (13.29 KB, patch)
2011-12-02 14:19 PST, :aceman
no flags Details | Diff | Review
v2, suggested patch (13.35 KB, patch)
2011-12-06 12:07 PST, :aceman
standard8: review+
Details | Diff | Review
v3 of patch (15.37 KB, patch)
2011-12-16 09:49 PST, :aceman
acelists: review+
Details | Diff | Review

Description :aceman 2011-12-02 13:49:08 PST
In the attached patch I added some checks to NS_IMETHODIMP functions that lacked checks for null arguments passed to them.

It it useful?
Comment 1 :aceman 2011-12-02 14:19:40 PST
Created attachment 578726 [details] [diff] [review]
suggested patch

I made this patch in the same style as other bugs linked from bug 412109.
I do not have any proof these changes are needed (any read crash). I added it as a robustness measure. I think I read in other bugs that interface methods should check the arguments. Please check if this has any value.
It seems this caused about 3 test failures. I will look into them only if the patch is deemed useful. Otherwise I can just scrap it :)
Comment 2 :aceman 2011-12-06 12:07:14 PST
Created attachment 579401 [details] [diff] [review]
v2, suggested patch

Sorry, the checks in the original patch were too strict, caused some test failures. This patch add less checks, only on safer spots.

The change of code in nsMsgFilterList.cpp, starting at line 1127 adds braces to fix a compiler warning (possible ambiguous else) and reorders code to fold duplicated code. I hope I didn't change the logic of it. There is a logic change that now if rv fails and the function returns, *found is now true (instead of false previously). But the callers seem to check if rv failed so should ignore the returned *found.
Comment 3 Mark Banner (:standard8) 2011-12-16 00:35:23 PST
(In reply to :aceman from comment #0)
> In the attached patch I added some checks to NS_IMETHODIMP functions that
> lacked checks for null arguments passed to them.
> 
> It it useful?

Generally adding the checks means we'll obey xpcom rules a bit more, and it'll make it better for extension development, and stop ourselves doing the wrong thing. However, I don't see doing these on a mass basis (unless we're using automation) highly beneficial over fixing other bugs and updating code using obsolete values.
Comment 4 :aceman 2011-12-16 03:11:29 PST
OK. But I can't mostly do any other bugfixing (see other bugs assigned to me for what I can do). So if this is work I am able to do manually and have already done for these files, will you scrape it? Should I stop doing this even though I will then not do anything else? There even are fixes for obsolete values (PR_FALSE) in the patch.
Comment 5 Mark Banner (:standard8) 2011-12-16 05:07:25 PST
(In reply to :aceman from comment #4)
> OK. But I can't mostly do any other bugfixing (see other bugs assigned to me
> for what I can do). So if this is work I am able to do manually and have
> already done for these files, will you scrape it? Should I stop doing this
> even though I will then not do anything else? There even are fixes for
> obsolete values (PR_FALSE) in the patch.

I certainly don't want to stop you doing things for Thunderbird, I'll send you an email direct and we can think about what's best to get you doing.
Comment 6 Mark Banner (:standard8) 2011-12-16 05:13:04 PST
Comment on attachment 579401 [details] [diff] [review]
v2, suggested patch

Review of attachment 579401 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 with the nits below fixed.

::: mailnews/base/search/src/nsMsgFilter.cpp
@@ +440,1 @@
>                                      (void **)getter_AddRefs(term));

nit: please move the second line of the statement so that the (void ** aligns with the t of termIndex.

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +1145,5 @@
> +              *found = true;
> +          }
> +          if (*found)
> +          {
> +            if (!newFolderUri.IsEmpty())  //if we just want to match the uri's, newFolderUri will be null

Please put the comment on the previous line, and wrap it if necessary.
Comment 7 :aceman 2011-12-16 09:49:32 PST
Created attachment 582305 [details] [diff] [review]
v3 of patch

Carrying over r=standard8.
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 03:18:47 PST
https://hg.mozilla.org/comm-central/rev/5cca813898cc
Comment 9 :aceman 2011-12-20 03:26:33 PST
Sid0, thanks for merging, but you forgot to set the milestone. Is it TB11 as other patches you landed today?
Comment 10 Siddharth Agarwal [:sid0] (inactive) 2011-12-20 03:39:42 PST
Yep, sorry.
Comment 11 David :Bienvenu 2011-12-27 08:20:46 PST
this broke saving of message filters, so people are losing their filters.

  NS_ENSURE_ARG(! (filterCount >= filterIndex));

should be NS_ENSURE_TRUE, or remove the ! - I'm going to land a fix for this, because of the data loss.
Comment 12 :aceman 2011-12-27 11:21:45 PST
I am sorry about that. I must memorize those macros take negated expressions when converting existing code...

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