849423 - Enforce stack-only allocation of appropriate classes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

The JS engine has a bunch of classes that should only ever be allocated on the stack.  While doing bug 847248 I just saw this trick in content/xul/templates/src/nsTemplateMatch.h:

  class nsTemplateMatch {
  private:
    // Hide so that only Create() and Destroy() can be used to
    // allocate and deallocate from the heap
    void* operator new(size_t) CPP_THROW_NEW { return 0; }
    void operator delete(void*, size_t) {}

This is similar to the trick used to ban the use of a copy constructor.

Now, as written, nsTemplateMatch does it really badly -- the operators silently do nothing.  This bit me, because I called |new| when I needed |::new|!

So I tried getting rid of the operator function bodies, as is usually done with the copy constructor case.  I got this strange error on Windows:

  nsTemplateMatch.obj : error LNK2019: unresolved external symbol "private: static void __cdecl nsTemplateMatch::operator delete(void *,unsigned int)" (??3nsTemplateMatch@@CAXPAXI@Z) referenced in function "public: void * __thiscall nsTemplateMatch::`scalar deleting destructor'(unsigned int)" (??_GnsTemplateMatch@@QAEPAXI@Z)

I don't know what a `scalar deleting destructor' is, so I just kept the bodies and put MOZ_ASSERT(0) in them.

Now, for full protection, we need to cover all variants of new and delete:  scalar, vector, throwing/non-throwing(?), and placement.  We could bundle it up in a macro.

The next step:  which classes are stack-only and deserve this treatment?  I guess Rooted is one.

Hmm, this doesn't protect against the class being a member of another class that gets heap-allocated, but it's still better than nothing.

Comment 1

[Terrence Cole [:terrence]]

The important ones I can remember off the top of my head are Iterators, Frames, and the Tokenizer.

Comment 2

[Jeff Walden [:Waldo]]

Given that we don't use |new| itself inside the engine, deleting |operator new| and |operator delete| doesn't really do much for us.  If we were to do anything, I think it would have to be akin to the NS_STACK_CLASS macro in XPCOM, checked and/or enforced by a static analysis outside the normal course of compilation.  (Which to my mind makes it more or less useless, until we start putting some sort of enforcing clang plugin in the tree or something.)

The scalar deleting destructor bit is this:

http://ehsanakhgari.org/blog/2012-12-11/c-deleting-destructors

Comment 3

[Nicholas Nethercote [inactive]]

> Given that we don't use |new| itself inside the engine, deleting |operator
> new| and |operator delete| doesn't really do much for us.

Oh, crap.  Yes.

Comment 4

[Terrence Cole [:terrence]]

js_new calls placement new. Surely there is an operator we can overload for that?

Comment 5

[Terrence Cole [:terrence]]

Alternatively we could switch |js_new| to using |new| and create a generic |void *::new| which calls je_malloc.

Comment 6

[Nicholas Nethercote [inactive]]

> Alternatively we could switch |js_new| to using |new| and create a generic
> |void *::new| which calls je_malloc.

Don't assume we're using jemalloc.  It's not true on all platforms even in the browser.  And then there's JS_USE_CUSTOM_ALLOCATOR for other embedders.(


> js_new calls placement new. Surely there is an operator we can overload for
> that?

There is!

    void * operator new (std::size_t, void *) throw();
    void * operator new[] (std::size_t, void *) throw();

So maybe this will work after all.  (We need to be careful with the |throw()| part;  I think it's changed to |noexcept| in C++11, and Gecko uses CPP_THROW_NEW to handle the inconsistencies.)

Fun fact: the following no-throw variants of new/new[]

    void * operator new (std::size_t, const std::nothrow_t &) throw();
    void * operator new[] (std::size_t, const std::nothrow_t &) throw();

are also called "placement new" operators because they take an additional argument, even though they don't give you control over the placement.  Huh.  (At least, so says http://en.wikipedia.org/wiki/Placement_syntax.)

Comment 7

[Nicholas Nethercote [inactive]]

Oh hey, thanks to bug 856108 we have the MOZ_STACK_CLASS annotation which is checked by a clang-based static analysis.  I don't know if the static analysis is enabled on TBPL, however.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Not yet, but that's being worked in.

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to Nicholas Nethercote [:njn] (away until June 3) from comment #7)
> Oh hey, thanks to bug 856108 we have the MOZ_STACK_CLASS annotation which is
> checked by a clang-based static analysis.  I don't know if the static
> analysis is enabled on TBPL, however.

Bug 851753.

Comment 10

[Terrence Cole [:terrence]]

We are marked as MOZ_NONHEAP_CLASS and MOZ_STACK_CLASS and the analysis bug is RESOLVED FIXED. What Try flags do I need to use to test that this is working for us?

Comment 11

[(no longer active)]

(In reply to comment #10)
> We are marked as MOZ_NONHEAP_CLASS and MOZ_STACK_CLASS and the analysis bug is
> RESOLVED FIXED. What Try flags do I need to use to test that this is working
> for us?

You can build with this mozconfig: <http://mxr.mozilla.org/mozilla-central/source/browser/config/mozconfigs/linux64/debug-static-analysis-clang>.  The magic flag there is --enable-clang-plugin.

Also, if you look at <https://tbpl.mozilla.org/?showall=1>, you can find automated static checking builds under "S" in Fedora64 debug.

Comment 12

[Terrence Cole [:terrence]]

And |S| is now visible by default on TBPL on mozilla-inbound. Thanks and congratulations to everyone who worked on that!