658351 - Add extra assertions to jsval.h to check for malformed JSBools

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jonathan Protzenko [:protz]]

Attachment: Add extra assertions to catch these errors early

So we've had that kind of bugs in the past. The thing is bug 654126 is due to nsMsgDBView.cpp (Thunderbird code) doing

 *_retval = (whatever & 0x40000000);

with _retval being a PRBool*. This expression sometimes evaluates to 0x40000000 which leads to things similar to bug 346586. When this PRBool is converted to a JSBool, its value is incorrect. I'm not sure what happened next (:mrbkap knows better), but from what I understood, we then try to convert this value to a string. The code thinks that this is a boxed value, and tries to read at the address and segfaults.

Attached is a patch that shows what mrbkap had in mind, feel free to build upon it :-).

Comment 1

[Josh Matthews [:jdm]]

Could we use a fatal assertion (ie. MOZ_ASSERT or NS_ABORT_IF_FALSE) instead, just to really drive the point home?

Comment 2

[Jonathan Protzenko [:protz]]

I'm very much in favor of a hard fail there, but I'll let real JS hackers express their opinion!

Comment 3

[Jeff Walden [:Waldo]]

I think we need this assertion underneath BOOLEAN_TO_JSVAL somewhere (which level wasn't immediately apparent at a quick glance).  If that's in place it catches *all* instances of this problem, not just this particular one.  If we had that, this specific assertion wouldn't (I think) be necessary, although it wouldn't hurt to keep it around for belt-and-suspenders protection.

Fatal assertions are always good in the opinion of JS engine hackers (although in very very rare cases they might be backed up by opt-build failure-handling), pace other Mozilla developers.

Comment 4

[Jeff Walden [:Waldo]]

On a slightly closer look, BOOLEAN_TO_JSVAL_IMPL(JSBool b) looks like the right place for the assertion.  JS_ASSERT is properly fatal, so no need to worry about selecting the right assertion to use there.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 533744 [details] [diff] [review]
Add extra assertions to catch these errors early

I think Waldo's right on this one. The reason that I started to add the assertion there was mainly to reduce my rebuild time; pushing this assertion into the JS engine will catch more cases.

Comment 6

[Jonathan Protzenko [:protz]]

Attachment: Newer patch

Here's an updated patch more in line with what mrbkap had in mind. I took the liberty of adding comments, although comments are scarce in this file. I hope this is not too much of a n00b behavior :-).

I've been running Thunderbird with this patch, and it seems to behave just fine. Hopefully there's not too many places in the code where we fail to construct proper PRBools.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 534382 [details] [diff] [review]
Newer patch

Review of attachment 534382 [details] [diff] [review]:
-----------------------------------------------------------------

I would write the comment as:

This assertion exists to catch JSAPI users who accidentally create bogo-booleans thanks to the fact that JSBool is, in reality, an integer. These users must convert to JS_TRUE or JS_FALSE before creating packing the boolean into a jsval.

Comment 8

[Jonathan Protzenko [:protz]]

Thanks Blake. Shall I land this on tracemonkey or mozilla-central?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Whichever is easier for you. They get merged to each other often enough.

I also hope you didn't take my comment word for word. It needs my "creating packing" thinko fixed one way or the other.

Comment 10

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/mozilla-central/rev/4e79d99681f0

Comment 11

[Jonathan Protzenko [:protz]]

and http://hg.mozilla.org/mozilla-central/rev/108fdd171946

Comment 12

[Mounir Lamouri (:mounir)]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/38bf7cf03a69
http://hg.mozilla.org/mozilla-central/rev/aad7af3a3360

Comment 13

[Jeff Muizelaar [:jrmuizel]]

I've filed bug 659707 which may have helped to catch the compiler errors earlier.

Comment 14

[Jonathan Protzenko [:protz]]

Re-landed with bug 659666 fixed

http://hg.mozilla.org/mozilla-central/rev/14b1ffc1bf53

Comment 15

[Boris Zbarsky [:bzbarsky]]

So just to be clear, we introduced a known fatal assert on a codepath we _know_ can pass in data that will trigger it without fixing that codepath?  What the hell?  How did this get review?

Comment 16

[Jonathan Protzenko [:protz]]

Attachment: At a conversion from rogue PRBools at the xpcconnect border

Being too strict has caused to much fallout, so the reasonable thing to do here (thanks bz) is to go on with our lives and convert bad PRBools into good ones before feeding them into the JS engine. This goes with a warning, though, and we're trying to fix as much as we can in the bugs this bug depends on so that the warning remains meaningful.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 537607 [details] [diff] [review]
At a conversion from rogue PRBools at the xpcconnect border

>+            if (b != 1 && b != 0)
>+                NS_WARNING("Passing a malformed PRBool through XPConnect");

NS_WARN_IF_FALSE, please.

r=me with that change.

Comment 18

[Jeff Walden [:Waldo]]

(In reply to comment #15)
> So just to be clear, we introduced a known fatal assert on a codepath we
> _know_ can pass in data that will trigger it without fixing that codepath?

What do you mean?  I thought aside from the bug 659566 fallout things had been fixed.

I don't actually think a warning in xpconnect like this is going to get meaningful attention amongst the rest of our warning spew, for what it's worth.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Did you mean bug 659666?

Who knows whether "things have been fixed".  No one bothered to check whether they are!  All we know is that the precise limited set of codepaths exercised by our tests passes.

> I don't actually think a warning in xpconnect like this is going to get
> meaningful attention

That's probably true, but this code can't assert valid input.  It's just not guaranteed that.

Comment 20

[Jonathan Protzenko [:protz]]

I'll wait for the discussion to be settled before landing this, I don't want any more bashing in #developers :-). When you both agree, please let me know and I'll commit (or not) both this and bug 662126 in one push.

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to comment #18)
> (In reply to comment #15)
> > So just to be clear, we introduced a known fatal assert on a codepath we
> > _know_ can pass in data that will trigger it without fixing that codepath?
> 
> What do you mean?  I thought aside from the bug 659566 fallout things had
> been fixed.

FWIW, I felt into that assert yesterday while trying to run Firefox with a profile already in use.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I don't know what there is to agree about other than whether the warning should be there at all.  The !! bit needs to be there.  Please just commit the patch so people who're trying to dogfood the browser can actually do so!

Comment 23

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/mozilla-central/rev/dd3de318717f