"ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE..." due to optional_argc overflowing 8-bit unsigned

VERIFIED FIXED in Firefox 8

Status

()

Core
XPConnect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: khuey)

Tracking

(Blocks: 1 bug, {assertion, testcase, verified-beta})

Trunk
mozilla10
assertion, testcase, verified-beta
Points:
---

Firefox Tracking Flags

(firefox8 verified, firefox9 verified, firefox10 verified, status1.9.2 unaffected)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 563878 [details]
testcase

###!!! ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE or make the aWantsUntrusted explicit by making aOptionalArgc non-zero.: '!aWantsUntrusted || aOptionalArgc > 1', file content/base/src/nsGenericElement.cpp, line 1078
(Reporter)

Comment 1

6 years ago
Created attachment 563880 [details]
stack trace
The problem is that aOptionalArgc is a PRUint8.  In this case we have 258 arguments, so argc is 258 and since this function has two non-obvious arguments 256 is passed for aOptionalArgc.  But PRUint8(256) == 0.

Is there a good reason to not just make the optional_argc 32-bit and be done with it?

We could also remove the assertion in question, I suspect...
Component: DOM → XPConnect
QA Contact: general → xpconnect
Summary: "ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE..." → "ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE..." due to optional_argc overflowing 8-bit unsigned
The question is, would it be faster to clamp to the maximum number of optional arguments that the function takes, a well predicted branch seems faster than pushing more bytes to the stack.

It's news to me that that the clamping isn't happening already. I wonder if that results in buggy behavior if we do a == check to the maximum number of expected optional arguments somewhere.
> would it be faster to clamp to the maximum number of optional arguments that the function
> takes

Faster, probably not.  But that clamping might be the right thing to do, yeah.
The compiler almost certainly is allocating more than one byte anyways on the stack for this, so this shouldn't change the number of bytes pushed onto the stack.

But yeah, we definitely should clamp here.
Would this just result in us losing the user's arguments? That doesn't sound too scary.
Which "this"?  The bug, or the proposed fix?
Assignee: nobody → khuey
Created attachment 565732 [details] [diff] [review]
Clamp optional_argc values so that it is never higher than the number of parameters

This fixes the testcase.  Percolating through try now.
Attachment #565732 - Flags: review?(mrbkap)
This is green on try.

Updated

6 years ago
Attachment #565732 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/2e555f1dcdff
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla10
Comment on attachment 565732 [details] [diff] [review]
Clamp optional_argc values so that it is never higher than the number of parameters

I think we should take this patch on any branch we are going to ship a release from in the future.  It's very narrow and low risk and prevents scenarios that could cause problems all over the place.
Attachment #565732 - Flags: approval1.9.2.24?
Attachment #565732 - Flags: approval-mozilla-beta?
Attachment #565732 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #565732 - Flags: approval-mozilla-beta?
Attachment #565732 - Flags: approval-mozilla-beta+
Attachment #565732 - Flags: approval-mozilla-aurora?
Attachment #565732 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c11492b5310
https://hg.mozilla.org/releases/mozilla-beta/rev/98e69bdadbd3
status1.9.2: --- → ?
status-firefox10: --- → fixed
status-firefox8: --- → fixed
status-firefox9: --- → fixed
Target Milestone: mozilla10 → mozilla8
Is this fix verifiable a normal Firefox build using the attached testcase?
Whiteboard: [qa?]
No, you need a debug build.
qa- based on comment 14. Can someone who is already set up to test this bug please verify the fix?
Whiteboard: [qa?] → [qa-]
FTR, you don't have to build debug builds yourself.  You can download them from tinderbox for non-Windows platforms (e.g. ftp://ftp.mozilla.org/pub/firefox/nightly/2011-10-12-mozilla-central-debug/)

(Getting Windows debug builds to run on your machine requires installing the matching version of the compiler though)
qa+ per IRC since this should be verifiable easily on non-Windows with the builds from comment 16 (or similar builds on different branches).
Whiteboard: [qa-] → [qa+]

Updated

6 years ago
Attachment #565732 - Flags: approval1.9.2.24? → approval1.9.2.24+
Please land this on 1.9.2.24 asap. We're looking to go to build by the end of the week (at the latest). Thanks!
Ah, sorry forgot about this.  I'm heading out for the night, but I'll land it first thing tomorrow.
Actually, 1.9.2 doesn't even have optional_argc, so we don't need to do anything.
status1.9.2: ? → unaffected

Updated

6 years ago
Target Milestone: mozilla8 → mozilla10
test case doesn't crash 8,9 or 10
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Keywords: verified-aurora, verified-beta
There never was a crash here.  See comments 13-17.
Status: VERIFIED → RESOLVED
Last Resolved: 6 years ago6 years ago
Keywords: verified-aurora, verified-beta
Whiteboard: [qa!] → [qa+]
Kyle, so using the latest debug builds should verify this fix right?

Tracy, can you confirm if you tested with the debug versions of Firefox 8, 9, 10 in comment 21?

Thanks
Yes, if you load the testcase in a debug build and don't see any assertions (note that assertions do not cause crashes) then this bug is fixed.
Attachment #565732 - Flags: approval1.9.2.24+
Group: core-security
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20111110 Firefox/9.0
ftp://ftp.mozilla.org/pub/firefox/nightly/2011-12-13-mozilla-beta-debug

Verified in Firefox 9 Beta on Ubuntu 11.10 x86_64 and Mac OS 10.6 with builds from the above link.

Started Firefox from the terminal and loaded the testcase from comment 0. 
No assertions were displayed.
Keywords: verified-beta
Whiteboard: [qa+] → [qa+], [qa!:9]
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011-12-28-mozilla-beta-debug/

Verified on Firefox 10 beta1 on Mac OS 10.6 and Ubuntu 11.10. No assertions displayed when running testcase from commment 0.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+], [qa!:9] → [qa!], [qa!:9] [qa!:10]
status-firefox10: fixed → verified
status-firefox8: fixed → verified
status-firefox9: fixed → verified
Whiteboard: [qa!], [qa!:9] [qa!:10] → [qa!]
You need to log in before you can comment on or make changes to this bug.