230251 - crash sorting large folder by subject

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

I suspect this was caused by the sort optimization Neil made (or exposed by it,
at least). Purify reports the following:

[E] ABW: Array bounds write in memcpy {1 occurrence}
        Writing 203 bytes to 0x16b6b430 (43 bytes at 0x16b6b4d0 illegal)
        Address 0x16b6b430 is 16 bytes into a 176 byte block at 0x16b6b420
        Address 0x16b6b430 points to a malloc'd block in heap 0x02440000
        Thread ID: 0x860
        Error location
            memcpy         [memcpy.asm:109]
            nsMsgDBView::Sort(int,int) [nsMsgDBView.cpp:3496]
                      info->folder = curFolder;
                    }
                
             =>     memcpy(info->key, keyValue, actualFieldLen);
                    //In order to align memory for systems that require it, such
as HP-UX
                    //calculate the correct value to pad the actualFieldLen value
                    const PRUint32 align = sizeof(IdKey) - sizeof(IdDWord) - 1;
            nsMsgThreadedDBView::Sort(int,int) [nsMsgThreadedDBView.cpp:346]


                [E] ABW: Array bounds write in memcpy {1 occurrence}
                    Writing 203 bytes to 0x16b6b430 (43 bytes at 0x16b6b4d0 illegal)
                    Address 0x16b6b430 is 16 bytes into a 176 byte block at
0x16b6b420
                    Address 0x16b6b430 points to a malloc'd block in heap 0x02440000
                    Thread ID: 0x860
                    Error location
                    memcpy         [memcpy.asm:109]
                    nsMsgDBView::Sort(int,int) [nsMsgDBView.cpp:3496]
                          info->folder = curFolder;
                        }
                    
                 =>     memcpy(info->key, keyValue, actualFieldLen);
                        //In order to align memory for systems that require it,
such as HP-UX
                        //calculate the correct value to pad the actualFieldLen
value
                        const PRUint32 align = sizeof(IdKey) - sizeof(IdDWord) - 1;
                    nsMsgThreadedDBView::Sort(int,int) [nsMsgThreadedDBView.cpp:346]


            Allocation location
                malloc         [dbgheap.c:129]
                PR_Malloc      [prmem.c:474]
                nsMsgDBView::Sort(int,int) [nsMsgDBView.cpp:3470]
                        if ((PRUint32)(pTemp - pBase) + (keyOffset +
actualFieldLen) >= allocSize) {
                          maxSize = (keyOffset + maxLen) * (arraySize - numSoFar);
                          allocSize = PR_MIN(maxBlockSize, maxSize);
                 =>       pTemp = (char *) PR_Malloc(allocSize);
                          NS_ASSERTION(pTemp, "out of memory, can't sort");
                          if (!pTemp)
                          {

Comment 1

[David :Bienvenu]

note this happens on my bug notification folder, which has 2000 messages, most
with long subjects. Sorting this folder by sender, or other folders by subject,
doesn't cause a crash. 

Comment 2

[[e-mail]]

Could it be that this folder has one too many messages to sort in a single
block, so it allocates a new block for the last subject, but uses the default
allocation size, whereas the subject is actually longer than that? I think that
the old code could have suffered from the problem as well, you were just lucky
that the boundaries were different in your case, here's the old code:
      maxSize = (PRUint32)(maxLen + sizeof(EntryInfo) + 1) *
(PRUint32)(arraySize - numSoFar);
      maxBlockSize = (PRUint32) 0xf000L;
      allocSize = PR_MIN(maxBlockSize, maxSize);

and for comparison the new code:
      maxSize = (keyOffset + maxLen) * (arraySize - numSoFar);
      allocSize = PR_MIN(maxBlockSize, maxSize);

keyOffset is my equivalent of sizeof(EntryInfo) - I could restore the + 1 if you
thought it would help, but I think the real fix is to calculate the max size
based on the maximum of the default size and the current item's size, i.e.
      maxSize = (keyOffset + PR_MAX(actualFieldLen, maxLen)) * (arraySize -
numSoFar);

This guarantees that you allocate enough memory when numSoFar == 1.

Comment 3

[David :Bienvenu]

Neil, yes, that's what's going on, and the fix is to calculate the max size
correctly. I think the reason this isn't usually a problem is because collation
keys usually are shorter than our default max len for subject strings.

Comment 4

[David :Bienvenu]

Attachment: proposed fix

make sure alloc'd block is big enough for the current field. Subsequent fields
will be handled by the existing overflow check.

Comment 5

[[e-mail]]

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

Yeah, that's a better idea.

Comment 6

[David :Bienvenu]

fixed