1164444 - nsTArray destructor will crash when storing element with nsAutoTArray

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Jean-Yves Avenard [:jya]]

Example to reproduce:

  {
    struct Test {
      explicit Test(int i)
      {
        mBuffer.AppendElement(i);
      }
      nsAutoTArray<int, 4> mBuffer;
    };
    nsTArray<Test> foo;
    foo.AppendElement(Test(0));
    foo.AppendElement(Test(0));
    foo.AppendElement(Test(0));
    foo.AppendElement(Test(0));
    foo.AppendElement(Test(0));
  }

when foo goes out of scope, we have a crash.

(lldb) bt
* thread #1: tid = 0x91b572, 0x00007fff8fd7b331 libsystem_platform.dylib`_platform_memmove$VARIANT$Ivybridge + 49, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x29425a620)
    frame #0: 0x00007fff8fd7b331 libsystem_platform.dylib`_platform_memmove$VARIANT$Ivybridge + 49
  * frame #1: 0x00000001035bd2b1 XUL`nsTArray_CopyWithMemutils::MoveElements(aDest=0x000000012abc3cb8, aSrc=0x000000029425a620, aCount=18446744072193680806, aElemSize=4) + 49 at nsTArray.h:588
    frame #2: 0x00000001035bf35c XUL`nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::ShiftData(this=0x000000013d2fbf08, aStart=0, aOldLen=6063483240, aNewLen=0, aElemSize=4, aElemAlign=4) + 284 at nsTArray-inl.h:268
    frame #3: 0x000000010386e680 XUL`nsTArray_Impl<int, nsTArrayInfallibleAllocator>::RemoveElementsAt(this=0x000000013d2fbf08, aStart=0, aCount=1515870810) + 432 at nsTArray.h:1450
    frame #4: 0x0000000103875b3f XUL`nsTArray_Impl<int, nsTArrayInfallibleAllocator>::Clear(this=0x000000013d2fbf08) + 47 at nsTArray.h:1458
    frame #5: 0x0000000103875af9 XUL`nsTArray_Impl<int, nsTArrayInfallibleAllocator>::~nsTArray_Impl(this=0x000000013d2fbf08) + 25 at nsTArray.h:819
    frame #6: 0x0000000103875ad5 XUL`nsTArray<int>::~nsTArray(this=0x000000013d2fbf08) + 21 at nsTArray.h:1830
    frame #7: 0x000000010662f295 XUL`nsAutoArrayBase<nsTArray<int>, 4ul>::~nsAutoArrayBase(this=0x000000013d2fbf08) + 21 at nsTArray.h:1936
    frame #8: 0x000000010662f275 XUL`nsAutoTArray<int, 4ul>::~nsAutoTArray(this=0x000000013d2fbf08) + 21 at nsTArray.h:2042
    frame #9: 0x000000010662f255 XUL`nsAutoTArray<int, 4ul>::~nsAutoTArray(this=0x000000013d2fbf08) + 21 at nsTArray.h:2042
    frame #10: 0x000000010661df85 

got me puzzled for a little while.
In nsTArray_Impl<int, nsTArrayInfallibleAllocator>::~nsTArrayImpl()
Length() gives me:
(lldb) print this->Length()
(size_type) $0 = 2779096486

Comment 1

[Nathan Froyd [:froydnj]]

I believe Ehsan was trying to add a static analysis to prevent exactly this situation.  Ehsan, don't forget to transitively check elements for MOZ_NON_MEMMOVABLE things. :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Right, you can't store a type with self-pointers in it in nsTArray, because we memmove/realloc the buffer.  That includes autostrings, autoarrays, etc.  A static analysis for that would definitely be welcome.

Comment 3

[Nathan Froyd [:froydnj]]

It's worth noting that you *can* store types with self-pointers in nsTArrays, you just have to be careful to specialize nsTArray_CopyChooser appropriately:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#654

Comment 4

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #1)
> I believe Ehsan was trying to add a static analysis to prevent exactly this
> situation.  Ehsan, don't forget to transitively check elements for
> MOZ_NON_MEMMOVABLE things. :)

Can you clarify what you mean, please?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #1)
> > I believe Ehsan was trying to add a static analysis to prevent exactly this
> > situation.  Ehsan, don't forget to transitively check elements for
> > MOZ_NON_MEMMOVABLE things. :)
> 
> Can you clarify what you mean, please?

I meant that the analysis should check, for:

nsTArray_CopyChooser<T>

that T is MOZ_NON_MEMMOVABLE or that T contains no MOZ_NON_MEMMOVABLE members.  Does it do that?  (I haven't looked at the analysis.)

Comment 6

[Jean-Yves Avenard [:jya]]

Looks like nsAutoTArray isn't movable simply because on how it uses mHdr. As mHdr is a pointer to within the nsAutoTArray, a copy like the nsTArray does (with memmove) of the nsAutoTArray will have mHdr still pointing to the original instance.
When the first nsAutoTArray is destructed, mHdr in the new copy is pointing to invalid data.

A specialised Getter for mHdr would appear to be one way to get around that issue.

Comment 7

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #4)
> > (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #1)
> > > I believe Ehsan was trying to add a static analysis to prevent exactly this
> > > situation.  Ehsan, don't forget to transitively check elements for
> > > MOZ_NON_MEMMOVABLE things. :)
> > 
> > Can you clarify what you mean, please?
> 
> I meant that the analysis should check, for:
> 
> nsTArray_CopyChooser<T>
> 
> that T is MOZ_NON_MEMMOVABLE or that T contains no MOZ_NON_MEMMOVABLE
> members.  Does it do that?  (I haven't looked at the analysis.)

Yes, it does.  Furthermore, it considers T as non-memmovable if a base class is marked as MOZ_NON_MEMMOVABLE, or if a base has such a member.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> > I meant that the analysis should check, for:
> > 
> > nsTArray_CopyChooser<T>
> > 
> > that T is MOZ_NON_MEMMOVABLE or that T contains no MOZ_NON_MEMMOVABLE
> > members.  Does it do that?  (I haven't looked at the analysis.)
> 
> Yes, it does.  Furthermore, it considers T as non-memmovable if a base class
> is marked as MOZ_NON_MEMMOVABLE, or if a base has such a member.

\o/

Comment 9

[(no longer active)]

Can this be duped against bug 1159433?

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Can this be duped against bug 1159433?

I don't think so...

I would much prefer to properly fix things by making the object movable, or have the appropriate specialized nsTArray_CopyChooser rather than just not using them

Comment 12

[(no longer active)]

(In reply to Jean-Yves Avenard [:jya] from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #9)
> > Can this be duped against bug 1159433?
> 
> I don't think so...
> 
> I would much prefer to properly fix things by making the object movable, or
> have the appropriate specialized nsTArray_CopyChooser rather than just not
> using them

Hmm, but I thought your bug is talking about the generic problem, which is what that analysis will catch.  At any rate, if you'd like to keep this open, that's fine by me, but it would probably be a good idea to rephrase the summary according to what you'd like to see happen here. :-)

Comment 13

[Jean-Yves Avenard [:jya]]

Likely will never be fixed, and bug 1159433 prevents that use from happening.