We have some functions which can run script (e.g. FlushPendingNotifications). For a function that can run script, I believe the following need to be true to be safe: 1) Any caller must be flagged "can run script". 2) All arguments passed to any such function (including "this") must either be strong refs on the stack or arguments of its caller (hence recursively safe). We need some sort of opt-out for bindings (which rely on the JS stack to keep the object alive) and _maybe_ other cases in which we know there's a strong stack ref even if we're not calling through it (e.g. nsLayoutUtils::SurfaceFromElement has this for its Height() and Width() calls).

Andi/Tristan would either of you be interested in taking this on? I'm putting this in the backlog, because it's a really big project and I think it's unreasonable to expect it really soon, but it would help catch a lot of errors which occur because JS code runs and causes all of our state to be invalidated. It would be an interesting problem to try to figure out how to make this ergonomic as well.

Note that I'm OK with starting small and annotating a specific function as "can run script" and trying out the analysis on a small set of callsites, if that would help. Or is the big project the analysis at all, as opposed to making the tree comply with it?

(In reply to Boris Zbarsky [:bz] from comment #2) > Note that I'm OK with starting small and annotating a specific function as > "can run script" and trying out the analysis on a small set of callsites, if > that would help. Or is the big project the analysis at all, as opposed to > making the tree comply with it? The big project was making the tree comply with it mostly. My biggest concern right now is the infectious nature of the CAN_RUN_SCRIPT annotation, I worry that too much may end up needing to be annotated, so we might have to do a lot of work in order to avoid the need to annotate everything.

I have specific proposals for functions that would not infect more than 2-3 other functions, so don't worry about that. And specific proposal for functions that only infect a few dozen functions, all of which I've already hand-audited, and would be a security win.

In that case, I'll increase priority to P2 so we get it done quickly - Andi, do you think you could look into this? We'd want an attribute which can be put on a method which would require that all refcounted arguments are held with a strong on-stack reference which infects its callers, and considers parameters to be strongly held.

(In reply to Michael Layzell [:mystor] from comment #5) > In that case, I'll increase priority to P2 so we get it done quickly - Andi, > do you think you could look into this? > > We'd want an attribute which can be put on a method which would require that > all refcounted arguments are held with a strong on-stack reference which > infects its callers, and considers parameters to be strongly held. Sure, Tristan or I can take care of this.

Tristan has been working on that

Boris, I have a working prototype of this checker and I was just wondering if you have an opinion on how we should do the opt-out annotation; the main problem being that we cannot attach custom annotations to a statement, only to declarations. We can hide any kind of trick we want behind a macro, and I'm guessing that using a trick is fine as long as it doesn't incur any runtime overhead. I would prefer avoiding having to use the lexer to make this trick work (for example, reading the previous line as a string and matching it seems a bit... dirty, and is not easy to implement properly). However, it's completely possible to use the AST to get the last statement, or the parent statement, etc. So I guess that having a macro which binds to a no-op (like a `typedef void MyAnnotation;`, or anything else in this fashion) could work. Let me know if you have better suggestions for this as there might be cleaner solutions which don't even involve compile time overhead. Also, when you are talking about `this` being passed as an argument, are you only talking only about regular arguments or also about method calls where it is passed as an implicit argument?

For purposes of bindings, we could do something like: typedef Foo* KnownLiveFoo; or some such, with whatever annotation we want on that typedef, and use of KnownLiveFoo in the places where we use Foo*. We could also introduce a KnownLive<T> type that wraps up a T* and compiles out to nothing, basically. So like RefPtr but without the refcounting. We'd then annotate it as keeping things alive. Is either one of those easier for you? As for `this`, I mean the implicit-argument case. So this pattern should be ok: class Foo { // refcounted void abc() { def(); } void def() { doSomething(this); // doSomething can run script } }; RefPtr<Foo> foo = getFoo(); foo->abc(); but the same thing with: Foo* foo = getFoo(); foo->abc(); should not be allowed. Neither should it be allowed if doSomething were a member method and called as doSomething() from inside def().

It seems to me that the solution with a no-op type like KnownLive<T> is the best solution here, because you would only have to wrap an argument in it to disable the checker error. Thanks for examples, I understand now.

So this is a first version of the patch which (hopefully) covers all the requirements from Boris's comment. You can use MOZ_CAN_RUN_SCRIPT before a function declaration to mark it, and you can wrap an argument in MOZ_KnownLive(arg) to disable complaints about this specific argument.

(Sorry for now using Mozreview, but I just assumed this was sec even though it's not... Maybe it should be?)

(In reply to Tristan Bourvon from comment #13) > (Sorry for now using Mozreview, but I just assumed this was sec even though > it's not... Maybe it should be?) (I don't use Mozreview most of the time - I don't mind at all ^.^ - use whatever is easier for you).

Comment on attachment 8894952 [details] [diff] [review] 1380423.patch Review of attachment 8894952 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/CanRunScriptChecker.cpp @@ +9,5 @@ > + auto InvalidArg = > + // We want to find any expression, > + ignoreTrivials(expr( > + // which has a pointer type, > + hasType(pointerType()), Please check that it has a pointer to a refcounted type. We still want to be able to pass in a type like `const char*`. ::: build/clang-plugin/CustomMatchers.h @@ +232,5 @@ > + > + D = D->getCanonicalDecl(); > + > + return D && (hasCustomAnnotation(D, "moz_is_refptr") || > + hasCustomAnnotation(D, "moz_is_smartptr_to_refcounted")); Why not make the MOZ_IS_REFPTR annotation expand to include both annotations? @@ +260,5 @@ > } > + > +#if CLANG_VERSION_FULL < 309 > +/// DISCLAIMER: This is a copy/paste from the Clang source code starting from > +/// Clang 3.9, so that this matcher is supported in lower versions. I don't think we support building with clang < 3.9 (as it is required by stylo), so we can probably just directly use this method from clang. If we have a builder on infra which is still using an older version we should probably update it. ::: mfbt/Attributes.h @@ +421,4 @@ > * > * The static analyses that are performed by the plugin are as follows: > * > + * MOZ_CAN_RUN_SCRIPT: Applies to functions which can run script. This forces How about "Applies to functions which can run script. Callers of this function must also be marked as MOZ_CAN_RUN_SCRIPT, and all refcounted arguments must be strongly held in the caller." @@ +421,5 @@ > * > * The static analyses that are performed by the plugin are as follows: > * > + * MOZ_CAN_RUN_SCRIPT: Applies to functions which can run script. This forces > + callers to also have this annotation. It also forces arguments passed to nit: Add '*' to the left column. @@ +476,5 @@ > * are disallowed by default unless they are marked as MOZ_IMPLICIT. This > * attribute must be used for constructors which intend to provide implicit > * conversions. > + * MOZ_IS_REFPTR: Applies to class declarations of ref pointer to mark them as > + * such for use with static-analysis. Can we write down what properties a class should have to be a "RefPtr" class? Also, what's the difference between this and MOZ_IS_SMARTPTR_TO_REFCOUNTED? I would say that RefPtr is a smart pointer to a refcounted object. ::: mfbt/StaticAnalysisFunctions.h @@ +27,5 @@ > + * > + * Example: > + * canRunScript(MOZ_KnownLive(rawPointer)); > + */ > + nit: /** and remove this whitespace.

What are good ways for me to test this patch? For example, can I do a try push (with what syntax?) with this patch applied and MOZ_CAN_RUN_SCRIPT added to some function and see how things look?

If you've run mach bootstrap recently (since stylo came out), you'll have a clang in ~/.mozbuild/clang/bin. Add a mozconfig like: ac_add_options --enable-clang-plugin export CC="$HOME/.mozbuild/clang/bin/clang" export CXX="$HOME/.mozbuild/clang/bin/clang++" and then build, and you should get the analysis running locally. (I think that that clang has everything you need).

(In reply to Michael Layzell [:mystor] from comment #15) > @@ +260,5 @@ > > } > > + > > +#if CLANG_VERSION_FULL < 309 > > +/// DISCLAIMER: This is a copy/paste from the Clang source code starting from > > +/// Clang 3.9, so that this matcher is supported in lower versions. > > I don't think we support building with clang < 3.9 (as it is required by > stylo), so we can probably just directly use this method from clang. > > If we have a builder on infra which is still using an older version we > should probably update it. Well the try server uses 3.8 (not sure about m-i) so we're kind of forced to add this for now. But you're completely right, we should upgrade the version of Clang. It's not the first time I'm forced to do such a workaround and I think we could potentially benefit from stuff added in the new versions.

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #16) > What are good ways for me to test this patch? For example, can I do a try > push (with what syntax?) with this patch applied and MOZ_CAN_RUN_SCRIPT > added to some function and see how things look? In addition to what Michael said, you can use the following try syntax to test your changes against the static analysis: try: -b do -p linux64-st-an,macosx64-st-an,win32-st-an,win64-st-an -u none -t none

Here are the all the changes you asked for except the polyfill related to the old Clang versions.

OK, so I tried to do the try push. It looks like annotating a function MOZ_CAN_RUN_SCRIPT means you also have to annotate its callers, right? That is, the flag is not automatically propagated to the callers, but you have to manually annotate them all? I'm not sure what I think of that. In particular, this will require changing some auto-generated code to spit out MOZ_CAN_RUN_SCRIPT annotations, and the generator may not have the context to do this properly...

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #21) > OK, so I tried to do the try push. It looks like annotating a function > MOZ_CAN_RUN_SCRIPT means you also have to annotate its callers, right? That > is, the flag is not automatically propagated to the callers, but you have to > manually annotate them all? I'm not sure what I think of that. In > particular, this will require changing some auto-generated code to spit out > MOZ_CAN_RUN_SCRIPT annotations, and the generator may not have the context > to do this properly... Oh, sorry! This is what I had understood from your initial post. Let me change that :)

Comment on attachment 8894986 [details] [diff] [review] 1380423.patch Michael, I'm lifting the review request until I'm done with the change that Boris suggested.

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #21) > OK, so I tried to do the try push. It looks like annotating a function > MOZ_CAN_RUN_SCRIPT means you also have to annotate its callers, right? That > is, the flag is not automatically propagated to the callers, but you have to > manually annotate them all? I'm not sure what I think of that. In > particular, this will require changing some auto-generated code to spit out > MOZ_CAN_RUN_SCRIPT annotations, and the generator may not have the context > to do this properly... The big problem here is that we have no way to do global static analysis with the clang plugin. Basically the clang plugin can look at all of the ASTs and track definitions etc. for a single compilation unit, but cannot look at what is going on in other compilation units across the tree. As a call can call a method which is in a different compilation unit, the only way to know that that function calls one annotated with MOZ_CAN_RUN_SCRIPT in the clang plugin is to annotate every method which calls a method annotated with MOZ_CAN_RUN_SCRIPT with MOZ_CAN_RUN_SCRIPT. This might actually be better implemented as a sixgill check (which instead computes the full call graph and runs analyses on that), like bug 1294915 - I'm not sure, as I don't know what the capabilities of sixgill are.

OK. If it's hard to just propagate the tainting without annotation, we may just need to do the annotation. Does the MOZ_CAN_RUN_SCRIPT need to go on the declaration or implementation? What about in the case of virtual methods? Specifically, if you have an nsIFoo with a pure virtual "foo()" method, and an nsFoo subclass overriding and implementing "foo()", whose implementation calls a can-run-script function, which bits would be forced to be annotated? Which bits should be?

(In reply to Michael Layzell [:mystor] from comment #24) > This might actually be better implemented as a sixgill check (which instead > computes the full call graph and runs analyses on that), like bug 1294915 - > I'm not sure, as I don't know what the capabilities of sixgill are. I've looked into doing something similar to this in sixgill (for making sure that nsISupports things are rooted on the stack). To an extent, you can piggyback on what the GC rooting analysis does, but the problem is that the analysis itself doesn't really figure out if the value that flows into the argument is rooted or not. Instead, it relies on the type system (all of that Rooted<JSObject> stuff). So you end up either having to make the analysis much more complex, or to change argument types everywhere (to nsCOMPtr<nsIFoo> or whatever instead of nsIFoo*), which would require a lot of work.

Thanks for pointing this out Michael, I had completely forgotten about this limitation. (In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #25) > OK. If it's hard to just propagate the tainting without annotation, we may > just need to do the annotation. > > Does the MOZ_CAN_RUN_SCRIPT need to go on the declaration or implementation? > What about in the case of virtual methods? > > Specifically, if you have an nsIFoo with a pure virtual "foo()" method, and > an nsFoo subclass overriding and implementing "foo()", whose implementation > calls a can-run-script function, which bits would be forced to be annotated? > Which bits should be? We have full control about which declaration/definition we use, so it could be any rule you would prefer. This are examples of possible rules: - if any declaration of a function has the annotation then consider all of the other ones in the compilation unit do; - inherit the annotation from overridden methods, but remain able to add it when the overridden method is not marked; - only consider the declarations and not the definitions. Really, as long as you define your set of rules, it's pretty flexible.

Hmm. So conceptually, if any virtual override is MOZ_CAN_RUN_SCRIPT, then the thing being overridden should be MOZ_CAN_RUN_SCRIPT. If we do that, can we make it so you can flag the override impl as opposed to the override decl? I ask because the override decl is often inside NS_IMPL_NSIFOO and autogenerated....

I will need to confirm that tomorrow, but from what I know I believe this should be completely feasible. I'll get back to you.

Okay so after giving this a lot of thought and discussing with Andi, I have found a way to do this but you should be aware that it still has the compilation unit limitation: the overridden method will only reverse-inherit the override's annotation within the override's compilation unit. Please note that this also kind of violates the inheritance principles where the child affects the parent. One question: if an overridden method has the annotation, do you want the override to also have it? Let me illustrate this: class A { virtual void foo() {} MOZ_CAN_RUN_SCRIPT virtual void foo2() {} }; class B : A { MOZ_CAN_RUN_SCRIPT void foo() override {} void foo2() override {} }; class C : A { void foo() override {} void foo2() override {} }; From what I understand you want A::foo() to be considered MOZ_CAN_RUN_SCRIPT. However, in this case, should C::foo() be considered to be MOZ_CAN_RUN_SCRIPT? What about B::foo2() and C::foo2()?

> but you should be aware that it still has the compilation unit limitation OK. That makes it a bit less useful, because for our purposes the point is that class A in your example is a pure virtual class in a header somewhere and B is an impl somewhere, and it would be nice to catch calls to methods of A from various places that would land in the B impl. I understand why this isn't really doable across compilation units. So I guess what we _can_ do with the local analysis is to require that if an override is annotated then the overridden method must also be annotated. We probably want to do that as a followup, because it might require xpidl parser/generator changes to make that landable. > From what I understand you want A::foo() to be considered MOZ_CAN_RUN_SCRIPT. Right. > However, in this case, should C::foo() be considered to be MOZ_CAN_RUN_SCRIPT? No. MOZ_CAN_RUN_SCRIPT is an annotation about "what happens if you call this function?". If you're calling foo() on an A instance via the vtable (as in, not calling A::foo directly), it could call B::foo and run script. But if you're calling it on a C instance, that can't happen. > What about B::foo2() and C::foo2() Same thing. They can't run script in this example.

So I had to use a small trick to make this work, but it shouldn't have too big of a performance impact. Basically the first time we enter the check() method for each translation unit, we run an inner matcher which adds all the can-run-script methods in the translation unit to a set, which is then used by the main checker. This allows us to use complex rules like adding overridden methods, or method definitions (in addition to declarations). The reason I did this is that it is impossible to navigate the inheritance tree upwards with the matchers: I have no way of knowing if a method is overridden when I match a call to it in the AST. This is why we need to build a pre-cache which takes into account all the function declarations of the unit. Building it on the go also wouldn't work, because we could be matching the overridden method before matching the overriding method which has the annotation. If you see another way to do this, please let me know, but frankly I've gone over a lot of Clang documentation and this is just a limitation of the AST not storing reverse-inheritance data.

I meant "navigate the inheritance tree *downwards*".

Comment on attachment 8897777 [details] [diff] [review] 1380423.patch Review of attachment 8897777 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/CanRunScriptChecker.cpp @@ +73,5 @@ > + anything())), > + this); > +} > + > +void CanRunScriptChecker::buildFuncSet(ASTContext *Context) { I think it might make sense to add the ability to run one of these data collection passes in a more generic way as I've had multiple times in the past that I've wanted to do some form of data collection before running an analysis. We can do that in a separate bug though, esp. because it might be tricky to do nicely with the clang-plugin runner. @@ +124,5 @@ > + // match. > + const CXXConstructExpr* Construct = > + Result.Nodes.getNodeAs<CXXConstructExpr>("constructExpr"); > + > + // nit: missing comment ::: build/clang-plugin/CanRunScriptChecker.h @@ +10,5 @@ > + > +class CanRunScriptChecker : public BaseCheck { > +public: > + CanRunScriptChecker(StringRef CheckName, > + ContextType *Context = nullptr) nit: Indentation @@ +17,5 @@ > + void check(const MatchFinder::MatchResult &Result) override; > + > + // Simply initialize the can-run-script function set at the beginning of each > + // translation unit. > + void onStartOfTranslationUnit() override { nit: Please move this definition out of the header. Why not run buildFuncSet directly here instead of lazily initializing it? @@ +26,5 @@ > +private: > + /// This class is a callback used internally to match function declarations > + /// with the MOZ_CAN_RUN_SCRIPT annotation, adding these functions and all > + /// the methods they override to the can-run-script function set. > + class FuncSetCallback : public MatchCallback { I think this type can probably be put into an anonymous namespace in the cpp file in its entirety. @@ +31,5 @@ > + public: > + FuncSetCallback(std::unordered_set<const FunctionDecl*> &FuncSet) > + : CanRunScriptFuncs(FuncSet) {} > + > + void run(const MatchFinder::MatchResult &Result) override { nit: Please move this function into the .cpp file. @@ +44,5 @@ > + if (!Method) { > + return; > + } > + > + addAllOverridenMethodsRecursively(Method); nit: I think this will be more clear & shorter as: if (auto* Method = dyn_cast<CXXMethodDecl>(Func)) { addAllOverriddenMethodsRecursively(Method); } @@ +50,5 @@ > + > + private: > + /// This method recursively adds all the methods overriden by the given > + /// paremeter. > + void addAllOverridenMethodsRecursively(const CXXMethodDecl* Method) { (move this function into the .cpp file too) @@ +54,5 @@ > + void addAllOverridenMethodsRecursively(const CXXMethodDecl* Method) { > + for (auto OverriddenMethod : Method->overridden_methods()) { > + CanRunScriptFuncs.insert(OverriddenMethod); > + > + // If this is not the definition, we also add the definition (if it Why not only add definitions? I don't see the reason to add other methods and increase the size of the set. ::: build/clang-plugin/CustomMatchers.h @@ +65,5 @@ > return hasCustomAnnotation(&Node, "moz_no_addref_release_on_return"); > } > > +/// This matcher will match any function declaration that is marked to prohibit > +/// calling AddRef or Release on its return value. This comment is wrong. @@ +143,4 @@ > return false; > } > > +/// This matcher will select classes which are refcounted AND have a ref count. nit: have an mRefCnt member @@ +282,5 @@ > } > + > +#if CLANG_VERSION_FULL < 309 > +/// DISCLAIMER: This is a copy/paste from the Clang source code starting from > +/// Clang 3.9, so that this matcher is supported in lower versions. Do we run clang<3.9 on infra or something? My understanding was that we didn't need to support older versions of clang. ::: mfbt/Attributes.h @@ +603,5 @@ > # endif > # define MOZ_IMPLICIT __attribute__((annotate("moz_implicit"))) > +# define MOZ_IS_REFPTR __attribute__((annotate("moz_is_refptr"))) \ > + MOZ_IS_SMARTPTR_TO_REFCOUNTED > +# define MOZ_IS_SMARTPTR_TO_REFCOUNTED __attribute__((annotate("moz_is_smartptr_to_refcounted"))) Please reverse the order of these two defines.

(In reply to Michael Layzell [:mystor] (PTO Aug 17-23) from comment #34) > Comment on attachment 8897777 [details] [diff] [review] > 1380423.patch > > Review of attachment 8897777 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/clang-plugin/CanRunScriptChecker.cpp > @@ +73,5 @@ > > + anything())), > > + this); > > +} > > + > > +void CanRunScriptChecker::buildFuncSet(ASTContext *Context) { > > I think it might make sense to add the ability to run one of these data > collection passes in a more generic way as I've had multiple times in the > past that I've wanted to do some form of data collection before running an > analysis. > > We can do that in a separate bug though, esp. because it might be tricky to > do nicely with the clang-plugin runner. Sounds like a good idea. What do you have in mind to make this more generic? Anyway, let's indeed talk about this in a separate bug, I think it would be more productive to land this checker and then start building the generic way to do this on top of the existing code. > @@ +17,5 @@ > > + void check(const MatchFinder::MatchResult &Result) override; > > + > > + // Simply initialize the can-run-script function set at the beginning of each > > + // translation unit. > > + void onStartOfTranslationUnit() override { > > nit: Please move this definition out of the header. > > Why not run buildFuncSet directly here instead of lazily initializing it? Because it needs the ASTContext that we get through the MatchResult passed to check(). > @@ +54,5 @@ > > + void addAllOverridenMethodsRecursively(const CXXMethodDecl* Method) { > > + for (auto OverriddenMethod : Method->overridden_methods()) { > > + CanRunScriptFuncs.insert(OverriddenMethod); > > + > > + // If this is not the definition, we also add the definition (if it > > Why not only add definitions? I don't see the reason to add other methods > and increase the size of the set. Because we might be calling through a pure virtual method which only has a declaration, or the definition of the overridden method might be in another compilation unit. > @@ +282,5 @@ > > } > > + > > +#if CLANG_VERSION_FULL < 309 > > +/// DISCLAIMER: This is a copy/paste from the Clang source code starting from > > +/// Clang 3.9, so that this matcher is supported in lower versions. > > Do we run clang<3.9 on infra or something? My understanding was that we > didn't need to support older versions of clang. As I said above, AFAICT the try server still uses 3.8, so we'll have to keep this bit of code here for now. This is not the first time we do this (see plugin.h, which should maybe be updated because we don't use 3.6 anymore).

Comment on attachment 8898268 [details] [diff] [review] 1380423.patch Looks awesome! Thank you so much for your work on this!

Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2510955b5c90 Add static-analysis to enforce strict rules on functions which can run scripts. r=mystor