1608760 - Crash in [@ js::ErrorObject::init]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[[:philipp]]

This bug is for crash report bp-a0e45964-058a-496d-b04b-15eea0200112.

Top 10 frames of crashing thread:

0 xul.dll static js::ErrorObject::init js/src/vm/ErrorObject.cpp:477
1 xul.dll static js::ErrorObject::create js/src/vm/ErrorObject.cpp:530
2 xul.dll JS::CreateError js/src/jsexn.cpp:703
3 xul.dll mozilla::dom::ClonedErrorHolder::ToErrorValue dom/ipc/ClonedErrorHolder.cpp:286
4 xul.dll static mozilla::dom::ClonedErrorHolder::ReadStructuredClone dom/ipc/ClonedErrorHolder.cpp:225
5 xul.dll mozilla::dom::`anonymous namespace'::StructuredCloneCallbacksRead dom/base/StructuredCloneHolder.cpp:55
6 xul.dll JSStructuredCloneReader::startRead js/src/vm/StructuredClone.cpp:2626
7 xul.dll ReadStructuredClone js/src/vm/StructuredClone.cpp:650
8 xul.dll mozilla::dom::StructuredCloneHolder::ReadFromBuffer dom/base/StructuredCloneHolder.cpp:316
9 xul.dll mozilla::dom::ipc::StructuredCloneData::Read dom/ipc/StructuredCloneData.cpp:101

this crash signature is increasing during the 73.0b cycle - perhaps due to the changes in bug 1588839?

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Andrew, can you help find someone to investigate this new crash on 73?

Comment 2

[Andrew Overholt [:overholt]]

Philipp's guess of the relation to bug 1588839 seems plausible. Kris, can you investigate?

Comment 3

[Andrew McCreight [:mccr8]]

It looks like we're crashing when deserializing a JSWindow actor. I looked at a half dozen crashes and they were all crashing on this specific line:

obj->initReservedSlot(FILENAME_SLOT, StringValue(fileName));

Based on prior experience with error messages and IPC, my wild guess would be this has to do with trying to send extremely long strings as a part of error messages, but I'm not sure how that would manifest specifically as a crash on this line. (In many places we've had to truncate strings that are part of error messages sent across IPC, because we were hitting OOM crashes.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

That HandleString fileName is just passed through by calls until we get to ClonedErrorHolder::ToErrorValue, where it's inited like so:

    JS::Rooted<JSString*> filename(aCx);
    JS::Rooted<JSString*> message(aCx);
    if (!ToJSString(aCx, mFilename, &filename) ||
        !ToJSString(aCx, mMessage, &message)) {
      return false;
    }

ToJSString is a custom thing defined in that file which will happily produce invalid things:

static bool ToJSString(JSContext* aCx, const nsACString& aStr,
                       JS::MutableHandle<JSString*> aJSString) {
  if (aStr.IsVoid()) {
    aJSString.set(nullptr);
    return true;
  }
...

so if aStr.IsVoid() is true, we will end up with null in the Rooted<JSString*> and when we try to StringValue() it we will have an invalid JS::Value. In a debug build you will have failed assert by now (e.g. the js::gc::IsCellPointerValid(str) assert in JS::Value::setString, but in an opt build we press on to the initReservedSlot call, and eventually land at HeapSlot::post. That does:

      gc::Cell* cell = this->value.toGCThing();
      if (cell->storeBuffer()) {

The toGCThing() call is just casts and bitmasks, so will return 0. Then Cell::storeBuffer will call Cell::chunk, which also returns 0. Then it will try to do chunk->trailer.storeBuffer and read that value.

offsetof(Chunk, trailer) == 1048552 and offsetof(ChunkTrailer, storeBuffer) == 8. So I would expect crashes to happen around address 1048552 + 8 = 0xffff0 if we're ending up here. That is, in fact, what most of the crashreports show.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Now what's unclear to me is whether JS::CreateError should handle a null filename (like it handles a null message, if you dig down into things, at the level of js::ErrorObject::init) or not. Jason, any opinions?

If it should, then we should fix that. If it should not, we need to figure out what to do in ClonedErrorHolder::ToErrorValue if mFilename is void. Which is its default state (see ClonedErrorHolder::ClonedErrorHolder) until either ClonedErrorHolder::Init happens with a non-null err->filename or we do a ReadStructuredClone.

Now in this case, we have ClonedErrorHolder::ReadStructuredClone on the stack, so presumably the thing we serialized on the other end had a null filename for some reason... Presumably because it was initialized from a JSErrorReport with a null filename?

Comment 6

[Kris Maglione [:kmag]]

Yeah, as far as I can see, the only way this can happen (short of data corruption) is if we serialize an error report with a null filename.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)

Now what's unclear to me is whether JS::CreateError should handle a null filename (like it handles a null message, if you dig down into things, at the level of js::ErrorObject::init) or not. Jason, any opinions?

FWIW, I see a number of places that null check JSErrorBase::filename in this list: https://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_JSErrorBase%3E_filename&redirect=false

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This bug isn't assigned and we're very-quickly running out of time to address this bug during this cycle with the All Hands next week and RC week after that. Is anybody actively working on this?

Comment 9

[Andrew McCreight [:mccr8]]

I can fix this. I was waiting on Jason to answer the needinfo, but I can just ask him to review a patch and see what he thinks.

Comment 10

[Andrew McCreight [:mccr8]]

I did figure out a way to hack up the code that causes a crash, following Boris's detailed breakdown of what the problem is. You just add err->filename = nullptr; to ClonedErrorHolder::Init() right before it null checks err->filename. This makes us send an error across IPC that has a null filename, and then we crash. I'm not sure how exactly to trigger that condition, though I did find bug 1610606. Given that there could be other issues lurking, plus the fact that it seems like a bad idea to crash if the child sends us bad data, I think it makes sense to just handle a null filename directly.

Comment 11

[Andrew McCreight [:mccr8]]

Oh, and after making that code change, you can run the test dom/ipc/tests/JSWindowActor/browser_sendQuery.js to trigger the crash. (I didn't check if some other thing would make it crash.)

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1608760 - Make ClonedErrorHolder::ToErrorValue() deal with a null filename.

If you serialize and then deserialize a JSErrorReport with a null filename
field, then we end up passing a null filename to JS::CreateError, which ends
up crashing. Perhaps CreateError should be robust against a null filename,
but the fact that whatever latent null filename issue wasn't causing crashes
until this serialization was added suggests it might be reasonable to deal
with it inside ClonedErrorHolder.

This patch causes a null string to instead become an empty string. We could do
this on the sending side, but by dealing with it on the receiving side we get
the bonus of keeping a malicious sender from crashing us.

Comment 13

[Andrew McCreight [:mccr8]]

Boris would prefer this be fixed on the JS side, so I'll move it back.

Comment 14

[Andrew McCreight [:mccr8]]

I tried to modify js::ErrorObject::init so that it does for .fileName what is being done for .message, but I started hitting various weird internal JS invariants like Assertion failure: obj->inDictionaryMode() || parent->hasMissingSlot() || child.slot() == parent->maybeSlot() + 1 || (parent->maybeSlot() + 1 < JSSLOT_FREE(obj->getClass()) && child.slot() == JSSLOT_FREE(obj->getClass())) so I think somebody who is more familiar with the JS engine will have to look at this if we're going that route, so I'll re-needinfo Steven.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Non-working attempt to make JS::CreateError callable with a null fileName.

For posterity, here's my patch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Those asserts are from the change to js::ErrorObject::assignInitialShape. That change needs to be undone; the property needs to exist. In particular, the issue is that the slot there is being added out of order, which is what the assert being hit is about.

Now what I don't know is why some of the callsites are setSlotWithType and some are initReservedSlot: I'd have to do some code-sleuthing to figure that out.

Comment 17

[Steven DeTar [:sdetar]]

Jason, it looks like Andrew needs your help after all, could you provide some input or point me to who on the SM team could help.

Comment 18

[Jason Orendorff [:jorendorff]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)

Now what's unclear to me is whether JS::CreateError should handle a null filename (like it handles a null message, if you dig down into things, at the level of js::ErrorObject::init) or not. Jason, any opinions?

It should not.

There are two problems to fix here:

1. something is producing invalid serialized data somehow
2. deserialization must either accept or reject all serialized data, not crash

To fix problem 2, we should probably take mccr8's first patch. A more principled patch would treat that condition as invalid serialized input and refuse to create an Error.

On to problem 1:

If it should, then we should fix that. If it should not, we need to figure out what to do in ClonedErrorHolder::ToErrorValue if mFilename is void. Which is its default state (see ClonedErrorHolder::ClonedErrorHolder) until either ClonedErrorHolder::Init happens with a non-null err->filename or we do a ReadStructuredClone.

Now in this case, we have ClonedErrorHolder::ReadStructuredClone on the stack, so presumably the thing we serialized on the other end had a null filename for some reason... Presumably because it was initialized from a JSErrorReport with a null filename?

We could add a release assertion on initialization and try to catch this closer to the source.

Comment 19

[Jason Orendorff [:jorendorff]]

Another way to proceed on problem 1 is to change JSErrorReport to require a non-null filename in its constructor. Currently the constructor leaves the filename field null, an invalid state. It's up to users to populate it (like this).

Comment 20

[Andrew McCreight [:mccr8]]

I did find one place (bug 1610606) that could result in a null filename field, by code inspection. But of course there could be other places.

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to Jason Orendorff [:jorendorff] from comment #18)

To fix problem 2, we should probably take mccr8's first patch. A more principled patch would treat that condition as invalid serialized input and refuse to create an Error.

I think we can't really handle this more nicely, because the exception is being sent in the middle of some kind of glob of JS, and as far as I can see any kind of error during deserialization results in a crash.

Boris, are you okay with the patch as it is, so we can fix the crash for beta, and then I can work on a larger patch to actually enforce that filename is non-null?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yeah, we can do that. Stamped. Thank you for looking at this!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ded947a2ee5
Make ClonedErrorHolder::ToErrorValue() deal with a null filename. r=bzbarsky

Comment 24

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/0ded947a2ee5

Comment 25

[Pascal Chevrel:pascalc]

Andrew, could you request an uplift to beta please? Thanks

Comment 26

[Andrew McCreight [:mccr8]]

Comment on attachment 9122183 [details]
Bug 1608760 - Make ClonedErrorHolder::ToErrorValue() deal with a null filename.

Beta/Release Uplift Approval Request

• User impact if declined: crashes
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The crash happens when we try to send a bad value from one process to another. We don't know how the bad value is created. The patch papers it over by detecting the bad value and fixing it. The behavior should be unchanged in other conditions.
• String changes made/needed: none

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9122183 [details]
Bug 1608760 - Make ClonedErrorHolder::ToErrorValue() deal with a null filename.

Wallpapers over a crash in Beta73. Approved for 73.0b10.

Comment 28

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/51e3079d2fee