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

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 6

[Daniel Veditz [:dveditz]]

Would this just result in us losing the user's arguments? That doesn't sound too scary.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Which "this"?  The bug, or the proposed fix?

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Clamp optional_argc values so that it is never higher than the number of parameters

This fixes the testcase.  Percolating through try now.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is green on try.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/2e555f1dcdff

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9c11492b5310
https://hg.mozilla.org/releases/mozilla-beta/rev/98e69bdadbd3

Comment 13

[u279076]

Is this fix verifiable a normal Firefox build using the attached testcase?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, you need a debug build.

Comment 15

[u279076]

qa- based on comment 14. Can someone who is already set up to test this bug please verify the fix?

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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)

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

qa+ per IRC since this should be verifiable easily on non-Windows with the builds from comment 16 (or similar builds on different branches).

Comment 18

[Alex Keybl [:akeybl]]

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!

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah, sorry forgot about this.  I'm heading out for the night, but I'll land it first thing tomorrow.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Actually, 1.9.2 doesn't even have optional_argc, so we don't need to do anything.

Comment 21

[Tracy Walker [:tracy]]

test case doesn't crash 8,9 or 10

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There never was a crash here.  See comments 13-17.

Comment 23

[u279076]

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

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 25

[Virgil Dicu [:virgil] [QA]]

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.

Comment 26

[Virgil Dicu [:virgil] [QA]]

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.