189591 - crashes in NS_MsgStripRE() due to multiply-linked strings library

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Christopher Blizzard (:blizzard)]

I'm seeing crashes when opening a mailbox that doesn't already have a .msf file
in NS_MsgStripRE().

The actual code that is crashing looks something like this:

nsXPIDLCString foo;
char *bar = "something";

char *foo = foo ? foo.get() : bar;

In this case, foo hasn't been set to anything so the test should fall through to
set foo = bar.  However, the test, which the compiler casts to this bit of code:

        // overridden to make getter_Copies mechanism work
      virtual const char_type* get() const
        {
          return (mBuffer.get() != GetSharedEmptyBufferHandle())
                    ? mBuffer->DataStart()
                    : 0;
        }

Fails to return '0'.  Instead it returns the string returned in
GetSharedEmptyBufferHandle(), even though the string is empty!

The problem seems to be that since the GRE landed that we link two copes of the
string library into the binary (one directly, as a .a and the other is included
as part of libxpcom.so) you can actually end up with multiple empty buffer
handles, as used in nsXPIDLCString::GetSharedEmptyBufferHandle():

nsXPIDLCString::GetSharedEmptyBufferHandle()
  {
    static shared_buffer_handle_type* sBufferHandle = nsnull;
    static char_type null_char = char_type(0);

    if (!sBufferHandle) {
      sBufferHandle = new nsNonDestructingSharedBufferHandle<char_type>(&null_ch
ar, &null_char, 1);
      sBufferHandle->AcquireReference(); // To avoid the |Destroy|
                                         // mechanism unless threads
                                         // race to set the refcount, in
                                         // which case we'll pull the
                                         // same trick in |Destroy|.
    }
    return sBufferHandle;
  }

Comment 1

[Doug Turner (:dougt)]

can anyone else reproduce this crash?  Blizzard, have you tried a nightly build?

Maybe replacing all string usages with nsEmbedString usages in xpfe/bootstrap
will work.

Comment 2

[Doug Turner (:dougt)]

Attachment: hacky patch

blizzard, can you rebuild with the statics removed.  Here is a hacky patch to
test out.  Apply it to string/src. rebuild strings, xpcom, and xpfe/bootstrap

(we will also have to clean up the statics in string/obsolete)

Comment 3

[Christopher Blizzard (:blizzard)]

That's going to allocate a new buffer every single time, and all strings will be
listed as empty.  That's not going to work.

Am I mis-reading that?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, it will allocate a new buffer every time and break the null-ness of
nsXPIDL[C]String.

Comment 5

[Christopher Blizzard (:blizzard)]

I meant "all of the new allocated strings returned will be empty."  Sorry.  Yes,
it obviously breaks the nullness of the operator in question.

Comment 6

[Doug Turner (:dougt)]

i am not proposing that we check this in... I was hoping that it would work well
enough to prove that the multiple statics are the problem.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That would only make the problem caused by multiple statics worse, since we'd
have one for every call instead of just 2.

Comment 8

[Doug Turner (:dougt)]

duh...  what was I thinking..  What we need is two version of the string libs,
one that declares this stuff, and another that externs is.  thoughts?

Comment 9

[Doug Turner (:dougt)]

jag - is there any clean way to remove the static usages from mozilla/string?

Comment 10

[Doug Turner (:dougt)]

the only other thing that I can think of is to ensure that the mozilla
application does not export any of the string symbols.  Right now, I am sure
that we just dump everything to the global sym table.  

Comment 11

[Doug Turner (:dougt)]

blizzard, can you try this:

drepper: DougT: Well, run moz with LD_DEBUG=reloc,symbols
LD_DEBUG_OUTPUT=<somefile> in the environment
drepper: this will produce an output file file which illustrates the lookup process

Comment 12

[Doug Turner (:dougt)]

blizzard, how exactly do you start mozilla?  do you pass something on the
command line?

Comment 13

[Doug Turner (:dougt)]

drepper DougT: if there is any chance that a string created by the DSO's code is
used in the app or vice versa you lose
drepper DougT: using statics this way is bad design

I think that we agree.  the static here are causing the problem.  We either need
to stop using these string classes in xpfe/bootstrap, or stop using the static
variables.

Jag, is it possible to remove the static variables?

Comment 14

[Ulrich Drepper]

My theory:

you pass a string which was created in a DSO with it's own string implementation
to a function in the application (which has a separate implementation) or vice
versa.

The result: the test for equality with sBufferHandle will fail even if the
string is empty.  This leads to get() returning 0.

Solution: remove the static.  Use a different way to handle empty strings.

Comment 15

[Christopher Blizzard (:blizzard)]

I've got a debug log but it's frigging _huge_.  Do you want me to filter out
certain parts?

Comment 16

[Ulrich Drepper]

I don't think I need the debug log.  As I said in comment #14 the problem is
probably passing a string created in the apps to the DSO or vice versa.

Comment 17

[Doug Turner (:dougt)]

just to clarify, i never saw this crash on windows.  that is why I thought it
might be a platform specific bug.  

bliz, are you passing anything special on the command line?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I realized a workaround in the string code would be to check the kIsImmutable
flag in the buffer handle instead of checking against the common shared buffer
handle.  This would have the disadvantage that assigning from an empty
nsSharableString to an nsXPIDLString would make the latter null.  (However, we
already have that problem between two |nsXPIDLString|s.)  An alternative would
be for |nsXPIDL[C]String| to set the kIsNULL flag on its buffer handle and
respect the kIsNULL flag in its .get().  I don't think that's quite the intent
of the kIsNULL flag (it's more for the DOM), but that might be the more sensible
solution.

jag?  scc?  thoughts?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: string patch that won't compile

This is what I was thinking, but getting it to compile would involve major
changes in what's public and what's not or putting things we don't want in base
classes up in base classes.

Comment 20

[Christopher Blizzard (:blizzard)]

I'm passing -mail on the command line.

Comment 21

[Doug Turner (:dougt)]

dbaron - what about the other statics in the string code?

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think we use any of the others for comparisons like this.

Comment 23

[Doug Turner (:dougt)]

dbaron - what needs to be public that isn't?  

Comment 24

[Doug Turner (:dougt)]

Attachment: string patch that compiles

this is dbaron's patch.  i basically converted kIsNULL to the value defined in
the class (0x80000000).

Comment 25

[Christopher Blizzard (:blizzard)]

This fixes my crash (yay!) but I'm not a huge fan of magic constants (boo.)

Comment 26

[Doug Turner (:dougt)]

regarding the magic constants - this value is defined by the
nsSharedBufferHandle class (see nsBufferHandle.h).  This enum is protected and
therefore isn't accessable outside of the nsSharedBufferHandle (or its derived
classes).  I hope there is a cleaner way to do this.  dbaron, any ideas?

Comment 27

[Doug Turner (:dougt)]

Attachment: with dbaron's suggestions

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 112194 [details] [diff] [review]
with dbaron's suggestions

sr=dbaron  (I'm not quite sure why
GetImplementationFlags/SetImplementationFlags were public while the flags were
private.  Perhaps I'd prefer going the other direction, but this seems OK.)

Comment 29

[Doug Turner (:dougt)]

fix checked in:

Checking in public/nsBufferHandle.h;
/cvsroot/mozilla/string/public/nsBufferHandle.h,v  <--  nsBufferHandle.h
new revision: 1.24; previous revision: 1.23
done
Checking in public/nsXPIDLString.h;
/cvsroot/mozilla/string/public/nsXPIDLString.h,v  <--  nsXPIDLString.h
new revision: 1.7; previous revision: 1.6
done
Checking in src/nsXPIDLString.cpp;
/cvsroot/mozilla/string/src/nsXPIDLString.cpp,v  <--  nsXPIDLString.cpp
new revision: 1.8; previous revision: 1.7
done


class unfrozen per dbaron.

Comment 30

[David :Bienvenu]

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

Comment 31

[jag (Peter Annema)]

+     
sBufferHandle->SetImplementationFlags(sBufferHandle->GetImplementationFlags() &
shared_buffer_handle_type::kIsNULL);


Shouldn't that be '|' instead instead of '&'?

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes.  Eek.  Could you check that in?

Comment 33

[Doug Turner (:dougt)]

Checking in nsXPIDLString.cpp;
/cvsroot/mozilla/string/src/nsXPIDLString.cpp,v  <--  nsXPIDLString.cpp
new revision: 1.9; previous revision: 1.8
done


fixed.

Comment 34

[Andrew Schultz]

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