add null-arg checks in nsMsgSearchSession.cpp (due to crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)])

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Filters
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: wsmwk, Assigned: aceman)

Tracking

({crash})

Trunk
Thunderbird 14.0
crash
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

22.92 KB, patch
aceman
: review+
Details | Diff | Splinter Review
crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)]
3 crashes in last few months. only v3.1.6. (looks like) only one person.
bp-a9579808-8656-4dc4-b2d9-808d02101111
0	thunderbird-bin	nsMsgFilter::CreateTerm	mailnews/base/search/src/nsMsgFilter.cpp:290
1	thunderbird-bin	VirtualFolderChangeListener::Init	mailnews/base/src/nsMsgAccountManager.cpp:2622
2	thunderbird-bin	nsMsgAccountManager::AddVFListenersForVF	mailnews/base/src/nsMsgAccountManager.cpp:3267
3	thunderbird-bin	nsMsgAccountManager::LoadVirtualFolders	mailnews/base/src/nsMsgAccountManager.cpp:3088
4	libxpcom_core.dylib	libxpcom_core.dylib@0x6e120	
5	thunderbird-bin	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2722
6	thunderbird-bin	XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740

Comment 1

7 years ago
Crash Reason	EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE
Crash Address	0x0

Wayne: can you try to include the crash reason+crash address when you file bugs like this? In theory it should help people recognize whether they're null pointer references (like this one) or unhappy pointer references (more work)

285 nsMsgFilter::CreateTerm(nsIMsgSearchTerm **aResult)
286 {
287    nsMsgSearchTerm *term = new nsMsgSearchTerm;
288    NS_ENSURE_TRUE(term, NS_ERROR_OUT_OF_MEMORY);
289
290    *aResult = static_cast<nsIMsgSearchTerm*>(term); 

caller lines:
2620   filterList->CreateFilter(NS_LITERAL_STRING("temp"), getter_AddRefs(tempFilter));
2621   NS_ENSURE_SUCCESS(rv, rv);
2622   filterList->ParseCondition(tempFilter, searchTermString.get()); 

Unfortunately, this seems like a case of pointer confusion, I can't quickly figure out how line 2622 could reach nsMsgFilter::CreateTerm
Crash Signature: [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)]
one crash in 8 months and the stack looks pretty worthless, so incomplete
bp-3dbaf602-f651-4a9b-8dfe-5d95a2111221 version 8

but if someone gets bored ...
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INCOMPLETE
(Assignee)

Comment 3

5 years ago
I guess we could at least add NS_ENSURE_ARG_POINTER(aResult); into nsMsgSearchSession::CreateTerm(nsIMsgSearchTerm **aResult) like it is now in nsMsgFilter::CreateTerm . (It wasn't there at the time of comment 1, it was added in bug 707306).

I have not yet looked into nsMsgSearchSession.cpp file for possible missing null-arg checks so this could be a good place to do it.
Assignee: nobody → acelists
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
(Assignee)

Updated

5 years ago
Status: REOPENED → ASSIGNED
Summary: crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)] → add null-arg checks in nsMsgSearchSession.cpp (due to crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)])
(Assignee)

Comment 4

5 years ago
Created attachment 606860 [details] [diff] [review]
null-args and cleanup
Attachment #606860 - Flags: review?(mbanner)
Comment on attachment 606860 [details] [diff] [review]
null-args and cleanup

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

::: mailnews/base/search/src/nsMsgSearchSession.cpp
@@ +78,3 @@
>  }
>  
> +/* [noscript] void AddSearchTerm(in nsMsgSearchAttribute attrib, in nsMsgSearchOperator op, in nsMsgSearchValue value, in boolean BooleanAND, in string customString); */

I think we can just drop these type of lines - they don't add any real value to the code.
Attachment #606860 - Flags: review?(mbanner) → review+
(Assignee)

Comment 6

5 years ago
Maybe the at least note which arguments are input and which output and readonly.

Should I remove them before each function?
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 7

5 years ago
Created attachment 611962 [details] [diff] [review]
patch v2

Removes comment lines. r=standard8.
Attachment #606860 - Attachment is obsolete: true
Attachment #611962 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/54480c6bb1f8

Any chance of getting a test for this?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
You need to log in before you can comment on or make changes to this bug.