Closed Bug 1312136 Opened 3 years ago Closed 3 years ago

Crash in InvalidArrayIndex_CRASH | nsMsgDBView::GetMsgHdrForViewIndex

Categories

(MailNews Core :: Backend, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

(thunderbird50 unaffected, thunderbird51 fixed, thunderbird52 fixed)

VERIFIED FIXED
Thunderbird 52.0
Tracking Status
thunderbird50 --- unaffected
thunderbird51 --- fixed
thunderbird52 --- fixed

People

(Reporter: calum.mackay, Assigned: alta88)

References

Details

(Keywords: crash, topcrash-thunderbird)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-574f3421-f1e5-4b34-b3fe-3ac5a2161021.
=============================================================

I've had this Daily crash (or similar) 5 times now, since the earliest instance with build 20160905030200. The latest, this one, with 20161020030210.

The other reports are as follows; same signature unless noted:

38108d1b-f113-488b-a0f6-a4a3a2161018

679d272f-adff-4d6c-a09a-64c762161016

8c453e4b-accc-47c4-bc1d-807df2160927

b1db78e6-0f33-4344-acc2-1782a2160905 Signature: InvalidArrayIndex_CRASH | nsMsgKeyArray::GetArray
crash stats suggest it affects Windows too.
oldest seems to be 20160821030227
Depends on: 1302478
Component: General → Backend
Product: Thunderbird → MailNews Core
yup, thanks Wayne; I did have mailsummaries temporarily installed, when I had that GetArray() crash, but I don't have it installed now, for the GetMsgHdrForViewIndex() crashes.
This happens because in bug 1159244 invalid array indices were turned into crashes. The fact that we are getting crashes shows that we have some invalid array index issues in nsMsgDBView.

The issue is broader than just this one point though. My suggestion is that we replace most m_key[index] access in view code with a macro that checks array bounds, adds an NS_WARNING and then returns NS_ERROR_INVALID_ARG.
Attached patch index.patchSplinter Review
this happened in a test profile recently, so here's a patch.

there is a func IsValidIndex() that checks upper bound also, and the caller can return NS_MSG_INVALID_DBVIEW_INDEX if appropriate.  this is used in dozens of places in nsMsgDBView, apparently a few were missed.
Assignee: nobody → alta88
Attachment #8805115 - Flags: review?(rkent)
Comment on attachment 8805115 [details] [diff] [review]
index.patch

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

LGTM, but I did not compile and test. Simple though.
Attachment #8805115 - Flags: review?(rkent) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/dd6e3dbcec59187b8dc92e66931d8740e57a34e5
Landed on closed/busted tree, will follow-up with try run.
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
Did I hear anyone say "uplift"?
Comment on attachment 8805115 [details] [diff] [review]
index.patch

follow Bug 1159244 landing in 51.
Attachment #8805115 - Flags: approval-comm-aurora?
Comment on attachment 8805115 [details] [diff] [review]
index.patch

Great, I'll see how my push goes and then uplift.
Attachment #8805115 - Flags: approval-comm-aurora? → approval-comm-aurora+
#3 crash for 50.0b2 and #4 for 50.0b3. please uplift to aurora, then we'll have it for 51 beta. thanks
Blocks: 1314348
Flags: needinfo?(jorgk)
I have three bugs on the radar for Aurora uplift, this one, bug 1311672 and bug 1312314. I'm waiting for C-C to come back, so I can try these in a Daily first. Don't worry, this won't miss TB 51 beta.
Flags: needinfo?(jorgk)
See Also: → 1308923
Duplicate of this bug: 1314348
no crashes starting TB51 beta, so v.fixed
(granted, we didn't automatically update most Mac users to 51 - but there have been enough Mac users there to conclude this is gone)
Status: RESOLVED → VERIFIED

bp-38108d1b-f113-488b-a0f6-a4a3a2161018
 0 	XUL	InvalidArrayIndex_CRASH(unsigned long, unsigned long)	xpcom/glue/nsTArray.cpp:35
1 	XUL	nsMsgDBView::GetMsgHdrForViewIndex(unsigned int, nsIMsgDBHdr**)	/builds/slave/tb-c-cen-m64-ntly-000000000000/build/objdir-tb/x86_64/dist/include/nsTArray.h:1033
2 	XUL	nsMsgDBView::UpdateDisplayMessage(unsigned int)	/builds/slave/tb-c-cen-m64-ntly-000000000000/build/mailnews/base/src/nsMsgDBView.cpp:1081
3 	XUL	nsMsgDBView::ReloadMessage()	/builds/slave/tb-c-cen-m64-ntly-000000000000/build/mailnews/base/src/nsMsgDBView.cpp:1067
4 	XUL	NS_InvokeByIndex	xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:180
You need to log in before you can comment on or make changes to this bug.