235307 - nsSupportsArray::Read() allocates wrong memory size

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Wladimir Palant]

User-Agent:       
Build Identifier: 

There is a bug in line 261 (allocation of a new buffer):
http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsSupportsArray.cpp#261
The size of the new array is mArraySize instead of newArraySize which means that
the array doesn't grow at all - the new value for mArraySize is set some lines
later. I don't know where nsSupportsArray::Read() is used, but it will crash
when reading some large array.

Reproducible: Didn't try
Steps to Reproduce:

Comment 1

[Wladimir Palant]

There is one more bug in the same method (not critical): line 252 should be
placed outside the inner if block, otherwise it is possible to reduce array size
to less then the size of mAutoArray.

Comment 2

[Wladimir Palant]

And one more in line 610 in nsSupportsArray::SizeTo() when determining whether
to use mAutoArray - it switches to mAutoArray when it is too small instead of
doing so when it is big enough. It should be if (kAutoArraySize >= aSize). This
one isn't critical either but it wastes resources.

Comment 3

[Wladimir Palant]

And while this file has to be patched anyway - how about replacing this horrible
code in nsSupportsArray::GrowArrayBy()? I understand it's a problem deciding
whether the grow algorithm has to be changed but even with the current algorithm
finding the next power-of-two size can be a lot less complicated (I first
thought this code was a bug). Here is a replacement for lines 167-185 as an example:

    if (newSize & (newSize-1)) { // count bits and stuff
      PRUint32 bits = 0;
      while (newSize >>= 1) {
        bits++;
      }
      bits++; // bump to the next power of two
      newSize = 1 << bits;
    }

Comment 4

[Wladimir Palant]

Correction: the bug in nsSupportsArray::SizeTo() _is_ critical because the old
data is copied into the new buffer which is too small for it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I bet this is used in fastload....

Comment 6

[Brendan Eich [:brendan]]

Who wrote that bad code, anyway?  I can take blame for the brain fart reported
in comment 0, but not the rest.  Taking.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fixes based on wladimir's comments

I used prbit.h instead of open-coding a log2 loop.

/be

Comment 8

[Wladimir Palant]

Bug 235217 is probably depending on this one, could someone please check if this
patch fixes it?

Comment 9

[Wladimir Palant]

One more problem I just saw in Read() - it's better to check if mCount is really
less or equal mArraySize/sizeof(nsISupports*) after reading it from the stream:

271   rv = aStream->Read32(&mCount);
272   if (NS_FAILED(rv)) return rv;
273 
274   for (PRUint32 i = 0; i < mCount; i++) {
...

I'm not trusting any streams usually...

Comment 10

[Wladimir Palant]

It's hard to believe and I looked at least 10 times over it, but it seems that
nsSupportsArray::Equals() compares the array to itself instead of using the
other array:

325         if (NS_FAILED( GetElementAt( aIndex, getter_AddRefs( other ) ) ))
326           return PR_FALSE;
327         if (mArray[aIndex] != other) {

I suppose it should be aOther->GetElementAt(...) instead...

Comment 11

[Wladimir Palant]

And another bug: nsSupportsArray::RemoveElementsAt() doesn't check the value of
parameter aCount, it will crash if aIndex+aCount is greater than mCount.

Somehow, this reminds me of bug 91362

Comment 12

[Wladimir Palant]

Please ignore the sizeof() in comment 9, it's nonsense.

There might be a problem with the patch of SizeTo(): I'm not sure if casting
aSize into PRUint32 is a good idea. The original version catches negative values
with the check for aSize < mCount at the beginning of the method, it will just
return PR_TRUE without doing anything. The patched version will pass the
negative value to the new operator instead.

Comment 13

[Brendan Eich [:brendan]]

Agreed on comment 9.

Comment 10 and comment 11 belong in other bugs.  If you want me to patch it all
here, I can, but I'm not going to take assignment of other bugs, or worry about
closing them.

> There might be a problem with the patch of SizeTo(): I'm not sure if casting
> aSize into PRUint32 is a good idea. The original version catches negative
> values with the check for aSize < mCount at the beginning of the method, it
> will just return PR_TRUE without doing anything.

Not so: mCount is unsigned and C and C++ promote from signed to unsigned.

> The patched version will pass the negative value to the new operator instead.

new takes size_t, which is an unsigned integral type.  It will fail for very
large arguments.

If you really want me to defend against negative inputs, I would rather do so
using an assertion.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: patch, v2

I fixed things reported here; this code needs a high colonic!

/be

Comment 15

[Wladimir Palant]

Sorry, I wasn't suggesting for you to take care of the bug, I was only assuming
it would be easy for you to check whether the patched Mozilla version still has
the problem (I have to keep looking for the reason if so). I don't really know
how things are done here (and I don't know how problems are usually solved in
Mozilla's C++ code, I was only making suggestions).

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 142099 [details] [diff] [review]
patch, v2

i would have added a comment above some of the more terse statements.  It took
me a while to figure out what:

if (newSize & (newSize - 1))

was doing.  (i cheated and looked at the comment below).

It would be nice to ask who this blames to for a review as well.  When they
wrote this, they may have been taking things into consideration that we are all
missing.

get it in soon; needs all the loving testing mozilla can provide.

Comment 17

[Brendan Eich [:brendan]]

dougt, the blame is easy to track, but not very interesting.  Some goes to me
(Read), some to waterson, some to kandrot.  But there's nothing deep here, just
bugs and suboptimal power-of-two rounding code.

Fix is in, thanks to all, esp. to Wladimir for reporting this!  Please test and
close any bugs that might also be fixed; I'm sorry I don't have time to do that
myself.

/be

Comment 18

[Mike Kaply [:mkaply]]

For 1.4, is there any way we could get a very simple fix that just fixes the
problem?

Comment 19

[Wladimir Palant]

This file hasn't been changed since January 2003, you can use the same patch for
1.4 as well.

Comment 20

[Mike Kaply [:mkaply]]

I want a "less risky" patch for 1.4.2.

For an older release, I'd prefer to just fix the bug rather than change the
implementation.