536139 - nsTArray_base should not compare pointers with &sEmptyHdr

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

The "mHdr == &sEmptyHdr" construction causes trouble when there are more than one sEmptyHdr instance. I hit the problem during conversing thunderbird to frozen linkage.

nsTArray.o is linked with xpcom_core.so and libxpcomglue_s.a. If an application  is linked with internal linkage, everything works fine (libxpcomglue_s.a is not used, code is linked directly with xpcom_core.so).

But with frozen linkage, each unit contains one sEmptyHdr instance (from libxpcomglue_s.a). Plus when the whole application is launched, ldd attaches xpcom_core.so to xpcom.so and we have one global sEmptyHdr instance.

Example of thunderbird build with frozen linkage:

[komat@localhost bin]$ nm -C components/*.so | egrep "sEmptyHdr|so:"
components/libaccessibility.so:
                 U nsTArray_base::sEmptyHdr
components/libaddrbook.so:
00000000002f19b8 b nsTArray_base::sEmptyHdr
components/libappcomps.so:
components/libauth.so:
components/libautoconfig.so:
components/libbayesflt.so:
000000000022c638 b nsTArray_base::sEmptyHdr
components/libcaps.so:
                 U nsTArray_base::sEmptyHdr
components/libchardet.so:
                 U nsTArray_base::sEmptyHdr
components/libchrome.so:
                 U nsTArray_base::sEmptyHdr
components/libcommandlines.so:
                 U nsTArray_base::sEmptyHdr
components/libcomposer.so:
                 U nsTArray_base::sEmptyHdr
components/libcookie.so:
                 U nsTArray_base::sEmptyHdr
components/libdbusservice.so:
0000000000212bf8 b nsTArray_base::sEmptyHdr
components/libdocshell.so:
                 U nsTArray_base::sEmptyHdr
components/libembedcomponents.so:
                 U nsTArray_base::sEmptyHdr
components/libfileview.so:
                 U nsTArray_base::sEmptyHdr
components/libfts3tok.so:
components/libgkgfxthebes.so:
                 U nsTArray_base::sEmptyHdr
components/libgklayout.so:
                 U nsTArray_base::sEmptyHdr
components/libgkplugin.so:
                 U nsTArray_base::sEmptyHdr
components/libhtmlpars.so:
                 U nsTArray_base::sEmptyHdr
components/libi18n.so:
                 U nsTArray_base::sEmptyHdr
components/libimgicon.so:
000000000021dee8 b nsTArray_base::sEmptyHdr
components/libimglib2.so:
                 U nsTArray_base::sEmptyHdr
components/libimport.so:
000000000024bf78 b nsTArray_base::sEmptyHdr
components/libintlapp.so:
                 U nsTArray_base::sEmptyHdr
components/libjar50.so:
                 U nsTArray_base::sEmptyHdr
components/libjsctypes.so:
                 U nsTArray_base::sEmptyHdr
components/libjsd.so:
components/liblocalmail.so:
00000000002ce7d0 b nsTArray_base::sEmptyHdr
components/libmailcomps.so:
0000000000245d5c b nsTArray_base::sEmptyHdr
components/libmailnews.so:
00000000003bbb28 b nsTArray_base::sEmptyHdr
components/libmailview.so:
0000000000215ba0 b nsTArray_base::sEmptyHdr
components/libmimeemitter.so:
00000000002283a0 b nsTArray_base::sEmptyHdr
components/libmime.so:
000000000027d000 b nsTArray_base::sEmptyHdr
components/libmork.so:
components/libmozfind.so:
components/libmozgnome.so:
000000000021a80c b nsTArray_base::sEmptyHdr
components/libmozldap.so:
components/libmsgcompose.so:
00000000002f3774 b nsTArray_base::sEmptyHdr
components/libmsgdb.so:
000000000026bfe8 b nsTArray_base::sEmptyHdr
components/libmsgimap.so:
0000000000358b70 b nsTArray_base::sEmptyHdr
components/libmsgmdn.so:
000000000021dba0 b nsTArray_base::sEmptyHdr
components/libmsgnews.so:
00000000002b4d30 b nsTArray_base::sEmptyHdr
components/libmsgsmime.so:
0000000000220ea8 b nsTArray_base::sEmptyHdr
components/libnecko.so:
                 U nsTArray_base::sEmptyHdr
components/libnsappshell.so:
                 U nsTArray_base::sEmptyHdr
components/libpermissions.so:
components/libpipboot.so:
components/libpipnss.so:
                 U nsTArray_base::sEmptyHdr
components/libpippki.so:
components/libpref.so:
components/librdf.so:
                 U nsTArray_base::sEmptyHdr
components/libremoteservice.so:
components/libsatchel.so:
components/libspellchecker.so:
                 U nsTArray_base::sEmptyHdr
components/libstoragecomps.so:
                 U nsTArray_base::sEmptyHdr
components/libsystem-pref.so:
components/libtkautocomplete.so:
                 U nsTArray_base::sEmptyHdr
components/libtoolkitcomps.so:
                 U nsTArray_base::sEmptyHdr
components/libtxmgr.so:
components/libuconv.so:
                 U nsTArray_base::sEmptyHdr
components/libucvmath.so:
components/libuniversalchardet.so:
components/libunixproxy.so:
components/libvcard.so:
0000000000211b34 b nsTArray_base::sEmptyHdr
components/libwebbrwsr.so:
                 U nsTArray_base::sEmptyHdr
components/libwidget_gtk2.so:
                 U nsTArray_base::sEmptyHdr
components/libwindowds.so:
components/libxpautocomplete.so:
components/libxpconnect.so:
                 U nsTArray_base::sEmptyHdr
components/libxpinstall.so:
components/libzipwriter.so:
                 U nsTArray_base::sEmptyHdr

So the result of this mess are various crashes (NS_Realloc tries to realloc static buffer and so on).

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

mHdr is initialized to NULL, sEmptyHdr is still used but not for mHdr check.

Comment 2

[timeless]

Comment on attachment 418613 [details] [diff] [review]
patch

+  : mHdr(nsnull) {

     // The array's elements (prefixed with a Header).  This pointer is never
     // null.  If the array is empty, then this will point to sEmptyHdr.
     Header *mHdr;

You need to fix the comment.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Attachment: fixed comments

Ahh, sorry for that, there's an updated version here. I'm not a native speaker so feel free to adjust it.

Comment 4

[Benjamin Smedberg]

I'm not sure I understand this bug: we should never be passing nsTArray across shared library boundaries, and we should never be linking things against both xpcom_core (internal linkage) and xpcomglue_s (external linkage). It would be nice if the linker could help enforce that rule, but I don't see the bug here.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Unfortunately nsTArray is passed across shared libraries (it's components/libmsgimap.so -> components/libmsgdb.so in thunderbird for instance)

Comment 6

[Benjamin Smedberg]

The non-null mHdr invariant is pretty well baked into this code, and was added for performance reasons IIRC. I think that it would be better to just require the static-mail configuration of Thunderbird, than try to hack this out of nsTArray.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

A solution could be to move nsTArray.o from libxpcom_core/libxpcomglue_s to libxpcom.so. Frozen and unfrozen code will share the same variable then...

Comment 8

[Benjamin Smedberg]

That would make nsTArray a frozen interface though, which is unacceptable (and the whole point of having the glue in the first place).

Comment 9

[[e-mail]]

So what about the following API calls:
nsIWidget::GetWindowClipRegion
nsIMenuRollup::GetSubmenuWidgetChain
nsISpellChecker::NextMisspelledWord
nsISpellChecker::CheckWord
nsISpellChecker::GetPersonalDictionary
nsISpellChecker::GetDictionaryList

Comment 10

[Benjamin Smedberg]

I don't believe nsIWidget or nsIMenuRollup are supposed to be used outside of gecko, but if they are they shouldn't be using nsTArray. I don't know what the situation is with nsISpellChecker.

Comment 11

[[e-mail]]

One alternative would be to avoid using sEmptyHdr by ensuring a capacity of at least 1 before crossing modules. This could be done in three ways:
1. For local arrays, using an nsAutoTArray
2. By calling SetCapacity before passing the array
3. By using the size_type constructor and calling SetCapacity after Clear

Exception: DeleteMessages should take an XPCOM array, not an nsTArray.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

It's not needed for libxul any more, closing.

Comment 13

[[e-mail]]

> components/liblocalmail.so:
> 00000000002ce7d0 b nsTArray_base::sEmptyHdr
> components/libmailcomps.so:
> 0000000000245d5c b nsTArray_base::sEmptyHdr
> components/libmailnews.so:
> 00000000003bbb28 b nsTArray_base::sEmptyHdr
> components/libmailview.so:
> 0000000000215ba0 b nsTArray_base::sEmptyHdr
> components/libmimeemitter.so:
> 00000000002283a0 b nsTArray_base::sEmptyHdr
> components/libmime.so:
> 000000000027d000 b nsTArray_base::sEmptyHdr
When using --with-libxul-sdk we now only support building (much of) mail into a single library, which means that we shouldn't have that problem there either.