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

RESOLVED FIXED in mozilla7

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: protz, Assigned: protz)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 533744 [details] [diff] [review]
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 :-).
Attachment #533744 - Flags: review?(mrbkap)

Comment 1

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

Comment 2

6 years ago
I'm very much in favor of a hard fail there, but I'll let real JS hackers express their opinion!
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.
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 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.
Attachment #533744 - Flags: review?(mrbkap)
(Assignee)

Updated

6 years ago
Attachment #533744 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 534382 [details] [diff] [review]
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.
Assignee: general → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #534382 - Flags: review?(mrbkap)
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.
Attachment #534382 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

6 years ago
Thanks Blake. Shall I land this on tracemonkey or mozilla-central?
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.
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/mozilla-central/rev/4e79d99681f0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Comment 11

6 years ago
and http://hg.mozilla.org/mozilla-central/rev/108fdd171946
Backed out:
http://hg.mozilla.org/mozilla-central/rev/38bf7cf03a69
http://hg.mozilla.org/mozilla-central/rev/aad7af3a3360
Status: RESOLVED → REOPENED
OS: Linux → All
Hardware: x86_64 → All
Resolution: FIXED → ---
Target Milestone: mozilla7 → ---
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Depends on: 659666
(Assignee)

Updated

6 years ago
Summary: xpcconvert.cpp should have extra assertions before blindly converting a PRBool into a JSVal → Add extra assertions to jsval.h to check for malformed JSBools
I've filed bug 659707 which may have helped to catch the compiler errors earlier.
(Assignee)

Comment 14

6 years ago
Re-landed with bug 659666 fixed

http://hg.mozilla.org/mozilla-central/rev/14b1ffc1bf53
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Depends on: 662086
Depends on: 662125
Depends on: 662126

Comment 15

6 years ago
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?

Updated

6 years ago
Depends on: 583699
(Assignee)

Comment 16

6 years ago
Created attachment 537607 [details] [diff] [review]
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.
Attachment #537607 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

6 years ago
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.
Attachment #537607 - Flags: review?(bzbarsky) → review+
(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

6 years ago
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.
(Assignee)

Comment 20

6 years ago
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.
(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

6 years ago
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!
(Assignee)

Comment 23

6 years ago
http://hg.mozilla.org/mozilla-central/rev/dd3de318717f
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 582523
You need to log in before you can comment on or make changes to this bug.