80097 - nsSupportsStringImpl tries to memcpy to nsnull

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

FreeBSD 4.1  Mozilla 20010510xx

nsSupportsStringImpl::SetDataWithLength() can try to memcpy to nsnull if aLength
for the new value happens to be the same as the garbage mLength in a freshly
allocated object (which has an mData of nsnull).  This will cause a crash.

The solution is to set mLength to 0 for nsSupportsStringImpl and
nsSupportsWStringImpl.  I'll attach a patch.

I hit this case at startup (I'll attach a GDB trace as well).

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: gdb backtrace

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch to 0 mLength

Comment 3

[R.K.Aa.]

i crash there all the time on linux today (RH7.1)
Sometimes on startup of browser, sometimes on startup of composer.. and
sometimes it just crashes. Had at least 10 so far today, both with a CVS build
and 20010510

Comment 4

[R.K.Aa.]

i crash all the time on linux today. 11 times so far, on different occations.
This is close to a blocker.

Comment 5

[R.K.Aa.]

oops.. didn't see the previous comment got added - crashed :)

Comment 6

[R.K.Aa.]

i reported this in bug 80025 also but resolved it as invalid because as it turns
out i'm not using a supported compiler anymore. This will become a mostfreq in
no time if not checked in fast. I rebuilt with the patch and i don't crash anymore.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Raising to blocker

Comment 8

[Scott Collins]

sr=scc.

Let's get this checked in as soon as possible.  Ed, if you don't have time or a
tree to do it, I will.  Let me know.

Comment 9

[R.K.Aa.]

dr just assigned bug 80088 to himself since he caused the bug - working on a
fix. One of these should be dup'ed against the other.

Comment 10

[Dan Rosen]

*** Bug 80088 has been marked as a duplicate of this bug. ***

Comment 11

[Dan Rosen]

yep, found the problem. i also want to be checking:

  if ((aLength == mLength) && aData && mData)

in SetDataWithLength. i'll check in asap.

Comment 12

[Scott Collins]

er, |aLength <= mLength|, right?

You don't want to re-allocate if you don't have to.

Comment 13

[Dan Rosen]

scc: i suppose i could make that change also -- the change i was referring to is
adding the null-check for |mData|. i'll attach a patch.

upping to smoketest blocker so i can get this checked in before the tree opens.

Comment 14

[Dan Rosen]

Attachment: fixes inits, checks mData for null, reallocs in even fewer cases

Comment 15

[Dan Rosen]

shit, wait, something missing...

Comment 16

[Dan Rosen]

Attachment: fix

Comment 17

[Dan Rosen]

> You don't want to re-allocate if you don't have to.

Ok, the patch attached doesn't realloc if it doesn't have to. This could be bad
though -- somebody can essentially allocate a huge buffer and reuse it for small
strings, and the memory would never be freed... What do you think, Scott?

Comment 18

[Scott Collins]

I think the actual number of times an |nsISupports[W]String| gets re-used is
likely to be small, though I haven't measured.

I think, after consideration, that if it is re-used, it would be re-used
multiple times (e.g., in a loop).  And if this is the case, then it warrants
adding an |mCapacity| member.  Which I'm loathe to do if it turns out they are
_never_ re-used, so now I have to search the tree.  ...searching... Hmm, the
number of uses of |SetDataWithLength| is negligible.  The number of uses of
|SetData| is quite managable.  Don't know how many of those are on the class we
care about.  So, my answer to this problem is ... go back to |==| and make a
note that you _would_ add |mCapacity| and |<=| if objects of this class were
heavily re-cycled.

Also, change your initialization to |mData(0)|, that's the C++ prefered way :-)

Comment 19

[Scott Collins]

it would probably be good to reference this bug in a comment in the code, please
do that

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

I've instrumented it and am rebuilding as I type - I figured it was probably
reallocating way too often and was going to go in to minimize that.

BTW, I wouldn't use the "shrinking allocation"; I'd use something like this:

if (len less than allocated &&
    (len more than 1/2 allocated || allocated <= 32))
{
  memcpy()
}
else
{
   reallocate
}

This requires an mAllocated/mCapacity. However, mLength is apparently
irrelevant, since this is true char* data - you can strlen when needed to get
the data out.  Compared to the cost of Clone(), that's nothing.

If we almost never reallocate it's not super important, but I may make a patch
anyways because excess allocations bug me.

Comment 21

[Dan Rosen]

randell: that's exactly the approach i'd have taken if this weren't a smoketest
blocker :) ... as it stands, i checked in the fix, and i'd encourage you to go
ahead with your patch when you get the chance.

Comment 22

[Dan Rosen]

*** Bug 80112 has been marked as a duplicate of this bug. ***

Comment 23

[Dan Rosen]

*** Bug 80132 has been marked as a duplicate of this bug. ***

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Well, after intrumenting this, and browsing around for a short while (cnn, espn,
buy, a javascript version of breakout, etc.)  Totals:

Program received signal SIGINT, Interrupt.
0x2840af64 in _thread_sys_poll () from /usr/lib/libc_r.so.4
8: gSize / (gInitial + gBigger + gSmaller + gEqual) = 11
7: gMax = 53
6: gSize = 1174
5: gEqual = 0
4: gSmaller = 0
3: gBigger = 0
2: gInitial = 100
(gdb) 

gMax is biggest, gSize is total size of all allocations, equal, bigger and
smaller are the number of times that happened - i.e. we _never_ reallocated. 
gInitial is the number of times we created an nsSupportsStringImpl.  I didn't do
the w version.

If this is indicative (and we don't do it wildly more for the W version), I see
_no_ need for any fancy footwork.  Of course, I may not have hit the cases where
this is used.

Comments?

Comment 25

[Scott Collins]

exactly the conclusion I arrived at (see my comment above) but by a different
path :-)

Dan, does that mean you checked in |==|?

Comment 26

[Dan Rosen]

yes.