537551 - "ASSERTION: invalid array index" triggered by nsImapFlagAndUidState::GetMessageFlagsFromUID

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[[e-mail]]

Stack of the IMAP thread:
msgimap!nsTArray<unsigned int>::ElementAt+0x31
msgimap!nsTArray<unsigned int>::operator[]+0x13
msgimap!nsImapFlagAndUidState::GetMessageFlagsFromUID+0x132
msgimap!nsImapProtocol::GetFlagsForUID+0x20
msgimap!nsImapProtocol::ProcessStoreFlags+0x2ab
msgimap!nsImapProtocol::ProcessSelectedStateURL+0x1692
msgimap!nsImapProtocol::ProcessCurrentURL+0xad6
msgimap!nsImapProtocol::ImapThreadMainLoop+0x128
msgimap!nsImapProtocol::Run+0x90
xpcom_core!nsThread::ProcessNextEvent+0x1f3
xpcom_core!NS_ProcessNextEvent_P+0x51
xpcom_core!nsThread::ThreadFunc+0xca
nspr4!_PR_NativeRunThread+0xd9
nspr4!pr_root+0x17
MSVCR71D!_beginthreadex+0x196
kernel32!BaseThreadStart+0x37

In nsImapFlagAndUidState::GetMessageFlagsFromUID, fUIDs has zero length (fPartialUIDFetch is 1) and the code tries to access element zero.

In case it's relevant, I have attachment 419597 [details] [diff] [review] applied.

Comment 1

[David :Bienvenu]

The patch for bug 524902 has landed on the trunk - it uses an nsTArray instead of just a c++ pointer, so invalid array references will cause assertions. It also clears out the array when we switch selected folders, which may exacerbate the situation, though switching to a larger folder would in theory have the same issue. We'll just have to keep an eye on this...I have not seen the assertion yet.

Comment 2

[[e-mail]]

So if a cleared array is to be expected, then does GetMessageFlagsFromUID need to be fixed to expect an empty array?

Comment 3

[David :Bienvenu]

(In reply to comment #2)
> So if a cleared array is to be expected, then does GetMessageFlagsFromUID need
> to be fixed to expect an empty array?

Ah, thx, I think I see the issue now. Presumably fUids is empty, and msgIndex is 0 here:

  // next, move msgIndex up to the first slot > than uid.
  while ((PRUint32) uid < fUids[msgIndex])
    msgIndex++;

I'll fix that, and any other issues I see, and try to come up with an xpcshell test.

Comment 4

[David :Bienvenu]

Attachment: proposed fix

I can't do xpcshell tests because we don't have condstore support in our imap fake server. Some day I'm going to have to add that.

Now that we're using nsTArray, there's not much sense in keeping track of how much space we've allocated when nsTArray will tell us how big it is, so I've gotten rid of that extra code.

Comment 5

[[e-mail]]

I wonder whether GreatestIndexLtEq would help here. I see it's new for 1.9.2

Comment 6

[David :Bienvenu]

Yes, I guess that would get rid of the while loop - I can try that when I get my environment set up again - I think the rest of the patch is still useful, though.

Comment 7

[[e-mail]]

Comment on attachment 420339 [details] [diff] [review]
proposed fix

>   fNumberOfMessagesAdded = 0;
>-  fNumberOfMessageSlotsAllocated = numberOfMessages;
So, looking at this more closely, fUids.Length() actually seems closer in meaning to fNumberOfMessagesAdded and fNumberOfMessageSlotsAllocated corresponds to fUids.Capacity() instead. We could eliminate the latter too as nsTArray also performs is own capacity management.

>+  if (!numberOfMessages)
>+    numberOfMessages = kImapFlagAndUidStateSize;
[This is never true as there is only one caller that passes in 100.]

>+  fFlags.InsertElementsAt(0, numberOfMessages, 0);
>+  fUids.InsertElementsAt(0, numberOfMessages, 0);
This would be equivalent to constructing fFlags/fUids i.e.

nsImapFlagAndUidState::nsImapFlagAndUidState(int numberOfMessages)
  : fUids(numberOfMessages),
    fFlags(numberOfMessages)
{

>   if (zeroBasedIndex + 1 > fNumberOfMessagesAdded)
>     fNumberOfMessagesAdded = zeroBasedIndex + 1;
This would have to change to use InsertElements to ensure that fUids/fFlags have an element at that index, but just performing length management (to zeroBasedIndex + 1) rather than capacity management.

Comment 8

[David :Bienvenu]

Attachment: fix addressing comments

Neil, this isn't quite the same as what you suggested, but I think calling SetLength() does the right thing...

Comment 9

[David :Bienvenu]

I'm writing some c++ test code for this, since I didn't want to expose the ability to create flag state objects to xpcom.

Comment 10

[David :Bienvenu]

Attachment: fix with unit test

added some tweaks to fix some assertions I was seeing...

Comment 11

[[e-mail]]

Comment on attachment 423090 [details] [diff] [review]
fix with unit test

>+  if (zeroBasedIndex < fUids.Length())
>+    *aResult = fUids[zeroBasedIndex];
>   else
>+    *aResult = 0xFFFFFFFF; // so that value is non-zero and we don't ask for bad msgs
Nit: could use fUids.SafeElementAt(zeroBAsedIndex, 0xFFFFFFFF);

>   imapMessageFlagsType returnFlags = kNoImapMsgFlag;
>-  if (zeroBasedIndex < fNumberOfMessagesAdded)
>+  if (zeroBasedIndex < fUids.Length())
>     returnFlags = fFlags[zeroBasedIndex];
>   
>   *result = returnFlags;
And again here.

>+  for (counter = 0; counter < (PRUint32) fUids.Length(); counter++)
Nit: Length() is already PRUint32

>+  if (zeroBasedIndex >= fUids.Length())
>   {
>-    PRInt32 sizeToGrowBy = NS_MAX(kImapFlagAndUidStateSize, 
>-                      fNumberOfMessagesAdded - fNumberOfMessageSlotsAllocated);
>-    fNumberOfMessageSlotsAllocated += sizeToGrowBy;
>-    fUids.InsertElementsAt(fUids.Length(), sizeToGrowBy, 0);
>-    fFlags.InsertElementsAt(fFlags.Length(), sizeToGrowBy, 0);
>+    fUids.SetLength(zeroBasedIndex + 1);
>+    fFlags.SetLength(zeroBasedIndex + 1);
The old code zeroed out the intervening slots, the new one does not. Problem?

> imapMessageFlagsType nsImapFlagAndUidState::GetMessageFlagsFromUID(PRUint32 uid, PRBool *foundIt, PRInt32 *ndx)
What about my suggestion to use GreatestIndexLtEq?

>-  while ((PRUint32) uid < fUids[msgIndex])
[Hmm, isn't uid already a PRUint32?]

Comment 12

[David :Bienvenu]

(In reply to comment #11)
> (From update of attachment 423090 [details] [diff] [review])
> >+  if (zeroBasedIndex < fUids.Length())
> >+    *aResult = fUids[zeroBasedIndex];
> >   else
> >+    *aResult = 0xFFFFFFFF; // so that value is non-zero and we don't ask for bad msgs
> Nit: could use fUids.SafeElementAt(zeroBAsedIndex, 0xFFFFFFFF);

I like that, thx.

> 
> >+  if (zeroBasedIndex >= fUids.Length())
> >   {
> >-    PRInt32 sizeToGrowBy = NS_MAX(kImapFlagAndUidStateSize, 
> >-                      fNumberOfMessagesAdded - fNumberOfMessageSlotsAllocated);
> >-    fNumberOfMessageSlotsAllocated += sizeToGrowBy;
> >-    fUids.InsertElementsAt(fUids.Length(), sizeToGrowBy, 0);
> >-    fFlags.InsertElementsAt(fFlags.Length(), sizeToGrowBy, 0);
> >+    fUids.SetLength(zeroBasedIndex + 1);
> >+    fFlags.SetLength(zeroBasedIndex + 1);
> The old code zeroed out the intervening slots, the new one does not. Problem?

It would be a problem...I'll try to see if I can add that to the unit test.
> 
> > imapMessageFlagsType nsImapFlagAndUidState::GetMessageFlagsFromUID(PRUint32 uid, PRBool *foundIt, PRInt32 *ndx)
> What about my suggestion to use GreatestIndexLtEq?

I looked at that, and I can still try it, but the comparator stuff put me off...also, from looking at the code, I don't think it does a binary search.

Comment 13

[David :Bienvenu]

Attachment: fix addressing comments

This addresses the comments, and extends the unit test to detect the case where we're not initializing the fUids when making space for new uids.

Comment 14

[David :Bienvenu]

I'm going to spin up a try server build with this patch for ludo to try, since I think the crash he has been seeing is related to the issues this patch fixes, and I'd like to see if the crash goes away with the patch.

Comment 15

[David :Bienvenu]

Attachment: patch w/o extraneous files

Comment 16

[[e-mail]]

Comment on attachment 424112 [details] [diff] [review]
patch w/o extraneous files

This is looking good, I'll test it out tomorrow.

>-nsImapFlagAndUidState::nsImapFlagAndUidState(PRInt32 numberOfMessages)
>+nsImapFlagAndUidState::nsImapFlagAndUidState(PRInt32 numberOfMessages) 
Nit: A space crept in here; git-apply said there were 5 lines in total.

>+  *foundIt = fUids.GreatestIndexLtEq(uid, nsDefaultComparator<PRUint32, PRUint32>(), (PRUint32 *) ndx);
Nice code replacement :-)

>+  imapMessageFlagsType returnFlags = (*foundIt) ? fFlags[*ndx] : 0;
kNoImapMsgFlag perhaps?

Comment 17

[David :Bienvenu]

ludo, here's a link to the mac try server build with the patch - http://s3.mozillamessaging.com/build/try-server/2010-01-28_13:[e-mail]-1264714116/[e-mail]

Comment 18

[David :Bienvenu]

Attachment: neil's nits addressed

Comment 19

[Ludovic Hirlimann [:Usul]]

(In reply to comment #17)
> ludo, here's a link to the mac try server build with the patch -
> http://s3.mozillamessaging.com/build/try-server/2010-01-28_13:[e-mail]-1264714116/[e-mail]

Running it right now - will let you know monday/tuesday if this is gone.

Comment 20

[Ludovic Hirlimann [:Usul]]

(In reply to comment #19)
 
> Running it right now - will let you know monday/tuesday if this is gone.

haven't crashed while using that build.

Comment 21

[David :Bienvenu]

we definitely want this for beta 1.

Comment 22

[Ludovic Hirlimann [:Usul]]

(In reply to comment #21)
> we definitely want this for beta 1.

/me agrees ! mark could you sr ?

Comment 24

[Mark Banner (:standard8)]

I took a look at this last night. It generally seems ok, however when I backed out the code changes I couldn't get the unit test to fail... any ideas?

Comment 25

[David :Bienvenu]

yeah, it just depends on how memory is initialized/uninitialized in terms of the test failing. I thought I had tweaked it so it fails on a windows debug build, but it's been a while, so my memory is a bit fuzzy. I may have been focusing on making the test fail with the related 3.01 issue.

Comment 27

[David :Bienvenu]

fix checked in.