1539948 - "nostdcall" should at least imply "noscript"

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

As far as I can tell, "nostdcall" can be used on a scriptable thing, and it will ... try to make it scriptable? I can't tell for sure, but seems very dubious.

Comment 1

[Andrew McCreight [:mccr8]]

I have a patch for nostdcall implying noscript. It does seem like this could cause badness if you tried to call a nostdcall method that was otherwise accessible to JS. Fortunately, nsIWebNavigation::binaryLoadURI() is the only such method as far as I can see, and it is not called from JS.

I think it doesn't make sense to make nostdcall imply notxpcom, because I think that for some heavily XPCOM-ish, but still C++, code that has to thread around an nsresult not being notxpcom makes some sense. Networking code I've looked at recently seems like that. Feel free to file a followup for that specific subissue if you disagree.

There are likely a lot of annotations that could be removed because of this, but that's already the case for, say, notxpcom implying noscript, so I'll leave it for followup work if anybody wants to look at that.

Comment 2

[Andrew McCreight [:mccr8]]

The only thing noscript is used for is to hide methods and attributes from JS. My first patch centralizes all checking for scriptability in a single method (one for methods and one for attributes). This also fixes a bug where some methods and attributes that are only implicitly builtinclass were not being treated as builtinclass.

Once the checks are all together, updating them to take into account nostdcall in the second patch is easy.

Comment 3

[Andrew McCreight [:mccr8]]

Err some of my logic is garbled up here, but I'll work it out.

Comment 4

[Andrew McCreight [:mccr8]]

Calling from C++ into JS does go through the stubs code first, so it seems like interfaces with any nostdcall methods should be implicitly builtinclass, as Nika noted in a comment in xpidl.py. I ran a local build to check if any interfaces that are implementable by JS have nostdcall, and unfortunately two of them do, nsIWebNavigation and nsISocketTransport. Both are implemented by JS (the latter only for testing?) so maybe the fix here is to un-nostdclass them?

Once that is done, I think we need to make nostdcall imply builtinclass and noscript.

The JS implementation of nsIWebNavigation is toolkit/components/remotebrowserutils/RemoteWebNavigation.jsm, and the nsISocketTransport one is in dom/presentation/tests/mochitest/PresentationSessionChromeScript.js. The latter implementation doesn't implement anything, which is a little weird.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the issue is that for a JS-implemented interface with a nostdcall noscript member, the code that ends up throwing is not reached until we've called the thing with the wrong calling convention, so we don't reach the desired code? I agree that's bad...

I would rather not have nostdcall imply builtinclass, because that's not at all obvious and it would be easy to accidentally cause problems by adding a nostdcall thing. I would prefer that we instead error out if a scriptable non-builtinclass interface has a nostdcall thing, with a message that tells people to either remove the nostdcall or add builtinclass.

Comment 6

[Andrew McCreight [:mccr8]]

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

So the issue is that for a JS-implemented interface with a nostdcall noscript member, the code that ends up throwing is not reached until we've called the thing with the wrong calling convention, so we don't reach the desired code? I agree that's bad...

I think it is worse than that. I think what happens is that C++ calls into a function, expecting nostdcall, but the stubs themselves don't actually know about nostdcall, so they are going to generate an entry in the vtable for a non-nostdcall. Maybe? I'm not entirely sure how this all works. I guess I should mark this security sensitive for now, as bug 809674 is a somewhat similar thing and it was a sec bug.

I would rather not have nostdcall imply builtinclass, because that's not at all obvious and it would be easy to accidentally cause problems by adding a nostdcall thing. I would prefer that we instead error out if a scriptable non-builtinclass interface has a nostdcall thing, with a message that tells people to either remove the nostdcall or add builtinclass.

This is actually what we already do for interfaces with a notxpcom method in them. I was already thinking I should just change it to produce an error, and fixing up places where we need to. That would be a little simpler to implement than what we have currently. There's a test for this behavior that will need to be fixed up. But anyways, I think that's somewhat orthogonal to making nostdcall things not do something bad.

Comment 7

[Andrew McCreight [:mccr8]]

(Also relevant is bug 1475699 where jandem fixed up the stubs to actually work with implicit_jscontext.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

so they are going to generate an entry in the vtable for a non-nostdcall

Right. So we end up doing something weird instead of returning an error. I agree that if this happens it's very bad!

Comment 9

[Andrew McCreight [:mccr8]]

(I file bug 1550770 about getting rid of implicit builtinclass for notxpcom.)

Comment 10

[Andrew McCreight [:mccr8]]

I'll mark this as sec-audit under the optimistic assumption that if somebody was misusing this and it was crashing, we'd already notice, because it will only affect chrome JS.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: nostdcall should imply noscript. (WIP)

Comment 12

[Nika Layzell [:nika] (ni? for response)]

I did a quick scan, and ran into a few scary cases:

• https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/docshell/base/nsIWebNavigation.idl#277
  • nsIWebNavigation isn't builtinclass, and is implemented by JS https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/toolkit/components/remotebrowserutils/RemoteWebNavigation.jsm#182-185
• https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/docshell/base/nsIDocShell.idl#118
  • nsIDocShell is builtinclass, but it should probably be marked as noscript, as it's currently visible to script to call, despite being [nostdcall].
• https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/netwerk/base/nsISocketTransport.idl#63-67
  • There is a JS implementation in tests which implements no methods: https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/dom/presentation/tests/mochitest/PresentationSessionChromeScript.js#114-116
  • The interface is also documented as being a "free-threaded interface, meaning that the methods on this interface may be called from any thread", which is scary for an interface not marked as [builtinclass].
• https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/xpcom/io/nsIBinaryOutputStream.idl#69-73
  • It might be possible to make this [builtinclass]?

At the very least, we should probably start producing errors if these mismatches happen in the future to avoid them coming up again.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

We should possibly also close bug 1508321, as we can't actually make all [noscript] types [nostdcall] due to the other issues mentioned in this issue.

Comment 14

[Andrew McCreight [:mccr8]]

Thanks for taking a look, Nika. Inspired by that, I've started poking at this bug again.

Comment 15

[Andrew McCreight [:mccr8]]

I don't understand the nsISocketTransport shim in PresentationSessionChromeScript.js. It looks like it doesn't implement anything, so what is it even doing? I'm hoping that if I make nsISocketTransport builtinclass the test will continue to pass. I can't test it locally because the test immediately fails because it pops up some permission prompt, but the test fails before I can okay it. I think.

Comment 16

[Andrew McCreight [:mccr8]]

There are 2 method/attributes that are nostdcall but not noscript (as identified above). 68 that are notxpcom and nostdcall but not noscript. 33 places that are notxpcom and neither nostdcall nor script.

Comment 17

[Andrew McCreight [:mccr8]]

Unsurprisingly, my "hope the presentation tests magically work" strategy has failed, and I'm getting the error on TreeHerder that you'd expect: "JavaScript error: http://mochi.test:8888/tests/dom/presentation/tests/mochitest/PresentationSessionChromeScript.js, line 110: NS_ERROR_FAILURE: Failure arg 1 [nsIServerSocketListener.onSocketAccepted]"

Comment 18

[Andrew McCreight [:mccr8]]

I gave up on trying to come up with an elegant solution, and I've reimplemented the mock socket in C++. If it passes tests, I'll upload my patches for review.

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: Bug 1539948, part 1 - Implement mocked socket transport in C++.

The DOM presentation tests require an implementation of
nsISocketTransport that doesn't actually implement anything,
for some reason.

Re-implementing that in C++ allows me to make nsISocketTransport
builtinclass.

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: Bug 1539948, part 2 - Make nsIWebNavigation and nsIWindowlessBrowser builtinclass.

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: Bug 1539948, part 3 - Only allow nostdcall methods and attributes on builtinclass interfaces.

We don't properly implement them in JS, so only allow them for C++.

This patch also makes the only remaining non-builtinclass interface
with a nostdcall method, nsIBinaryOutputStream, builtinclass.

This also changes the isScriptable() method to be consistent,
though I think the change doesn't matter because the only
place that calls it also checks if the interface is builtinclass.

Comment 22

[Andrew McCreight [:mccr8]]

I guess I can unhide this for ease of reviewing and landing, given that I'll fix it soon and I didn't find any actual problems.

Comment 23

[Andrew McCreight [:mccr8]]

I checked comm-central. There's only one interface that uses nostdcall, and it is already marked builtinclass.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef4e49b3701d
part 1 - Implement mocked socket transport in C++. r=nika,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/8bf24bdf431f
part 2 - Make nsIWebNavigation and nsIWindowlessBrowser builtinclass. r=nika,Gijs
https://hg.mozilla.org/integration/autoland/rev/81bed88d0adf
part 3 - Only allow nostdcall methods and attributes on builtinclass interfaces. r=nika

Comment 25

[Razvan Maries]

Backed out for build bustages.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=QeLijotUQeaXpSsjZ8K-SQ.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=81bed88d0adfc778f8bfa3d288527469b305c5a4

Log: https://treeherder.mozilla.org/logviewer?job_id=323997355&repo=autoland&lineNumber=16158

Backout: https://hg.mozilla.org/integration/autoland/rev/244906314ad1be1764f0c4d23660731bea444b8e

Comment 26

[Andrew McCreight [:mccr8]]

Ah, I guess I need to not use NS_IMETHODIMP on the nostdcall methods. Right, I forgot that only Windows cares about that stuff so I forgot to do a try run for it.

Comment 27

[Andrew McCreight [:mccr8]]

It took me a few tries, but I got it working on Windows: https://treeherder.mozilla.org/jobs?repo=try&revision=320b9ee651685ebd3621e1351c0c9c8da0d0b1b5

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f66ad72ffbc
part 1 - Implement mocked socket transport in C++. r=nika,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/ed170f22787b
part 2 - Make nsIWebNavigation and nsIWindowlessBrowser builtinclass. r=nika,Gijs
https://hg.mozilla.org/integration/autoland/rev/d55f965419e3
part 3 - Only allow nostdcall methods and attributes on builtinclass interfaces. r=nika

Comment 29

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/2f66ad72ffbc
https://hg.mozilla.org/mozilla-central/rev/ed170f22787b
https://hg.mozilla.org/mozilla-central/rev/d55f965419e3