518633 - Make the JSAuto* classes safe against non-brace-scoped lifetime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Consider this code:

  JSAutoTempValueRooter(cx, v);

Ruh-roh, Scooby!  You've just protected v for the duration of that statement only, against no possible ill effects.

People on stackoverflow have two suggestions on fixing this: use a macro, or force the ctor to take a pointer to itself with a runtime assertion.  (I suppose a static analysis is feasible as well, but I want something that will fail more eagerly than that, since most of us don't do s-a-enabled builds.)  The former doesn't forbid using the class directly, is less facially understandable, and involves macros.  The latter means we'd have to do:

  JSAutoTempValueRooter::JSAutoTempValueRooter(const JSAutoTempValueRooter& tvr,
                                               JSContext* cx,
                                               jsval v)
  {
    JS_ASSERT(this == &tvr);
  }

  JSAutoTempValueRooter tvr(tvr, cx, v);

...but I think I can stomach it.  Anyone object to the idea?

Comment 1

[Brendan Eich [:brendan]]

That's disgusting. :-/

I guess explicit would not do the trick? I'm not a C++ guru, but if it doesn't then what good is it?

How likely is this kind of error? If we can't fix it in C++, we could certainly use a treehydra script and the static analysis tinderbox will turn red in the rare event of such a thinko.

/be

Comment 2

[Brendan Eich [:brendan]]

dbaron may have solved this elsewhere.

My "That's disgusting. :-/" was indeed an objection. We shouldn't try to solve non-problems or marginal problems that have yet to bite, at the price of API and code complexity (source and binary). I'll leave it at that.

/be

Comment 3

[Jeff Walden [:Waldo]]

This isn't a non-problem or a marginal problem; I've made this typo a number of different times.  Sometimes I've caught it before posting a patch for review; other times reviewers have noticed the mistake.  This is very much a real problem.

Comment 4

[Luke Wagner [:luke]]

(In reply to comment #1)
> I guess explicit would not do the trick? I'm not a C++ guru, but if it doesn't
> then what good is it?

It protects you against implicitly constructing an object, like when you pass a 1 to a function taking a vector<int>, you might prefer an error instead of a vector of 1 element.  Unfortunately, waldo's code is being plenty explicit about constructing that object.

I'm also in favor of static analysis over gross-API.  It would be trivial to write.  I don't think its a problem that it doesn't fail eagerly, since its an unlikely error: all that matters is that we catch it at all.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This same problem has happened for a number of content classes.  It seems we could have some sort of "guard object" annotation we could make on various classes (JSTempValueRooter, nsAutoLock, nsCxPusher, nsAutoGCRoot, nsAutoScriptBlocker, etc., etc.), and verify some invariants about the use of such classes, such as that they're never constructed as temporaries.

(If C++ had const constructors (like const methods) we could fix this quite easily, though, by making the const constructor inaccessible, since temporaries are implicitly const... I think.)

Comment 6

[Benjamin Smedberg]

Bug 502775 has an analysis in-hand which can prevent certain classes from existing as a temporary, explicit or implied.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, I once had another idea for fixing this that *might* work but that I've never tried:  add an additional parameter to the constructor, ifdef DEBUG, that's another class (this could be the same class for all guard objects), with a default value.  Pass that object the address of a boolean member of the guard object.  That other object sets the boolean member in its destructor; the guard object asserts in its destructor that the boolean has already been so set.  If my memory of destruction order rules is correct, this might work, in that in the buggy case it would lead to an assert immediately followed by a likely-harmless write-after-destruction.

Might be worth trying...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That approach does seem to get working runtime assertions, at least with gcc.  Patch shortly.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for JSAutoTempValueRooter

This seems to work for me.

When I put:
    jsval silly = JSVAL_NULL;
    JSAutoTempValueRooter(cx, 1, &silly);
inside the main function in js.cpp, I get the expected assert when starting the JS shell.  (But if I insert " tvr", it starts up.)

I think the piece of the C++ standard that this is depending on is that the temporaries in a statement are destroyed in reverse order of creation.  (I think it says that.)  And the parameter to a temporary's constructor has to be constructed before it.


I'm actually not sure that my secondary assertion that the user initialized the boolean correctly is valid, though, since I think it may be allowed for the compiler to introduce extra temporaries in a construct like this.

Comment 10

[Luke Wagner [:luke]]

Attachment: example

(In reply to comment #7)
Inventive!  For fun, I whipped up what you said.  It would need to be macroized so that way all guard classes weren't super gross, but, yeah, you're right.  Also, from what I understand about sub-object lifetimes, this is well-defined to work across compilers (if they follow the rules...)

I'm still not so convinced that a nice little static analysis wouldn't be the way to go, though.  It would keep our guards crisp and clean, so that we could (as we should) apply RAII liberally.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, I think we should *also* definitely do a static analysis here; there are some existing problems in code that we may or may not execute all that often.

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #10)
On second thought, I access 'dead' after its object was destroyed, but that can be fixed... comment 9 obsoletes this anyhow.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for JSAutoTempValueRooter

I consider this version reviewable, although I'm expecting at the very least some quibbling about the names and the location of the code.  I expect we might want to duplicate this somewhere in XPCOM.

I think having assertions for this is useful (in addition to a static analysis) since it will help people catch these problems while they're developing the code (and potentially avoid a lot of wasted time trying to debug why a guard object wasn't working) rather than only catching problems after commit to mozilla-central.

Who ought to review?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for JSAutoTempValueRooter

Actually, I can avoid the potentially-bogus assertion by initializing to false earlier.  (I didn't start off that way because I started off thinking I was going to use a class and a boolean rather than two classes.)

And this fixes a whole bunch of deviations from local style as well.

Comment 17

[Brendan Eich [:brendan]]

Clearly I was wrong about this being a hypothetical problem! I stand corrected.

/be

Comment 19

[Brendan Eich [:brendan]]

Dbaron, can you refresh based on the commit for bug 518675.

/be

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #19)
> Dbaron, can you refresh based on the commit for bug 518675.

Is a new patch needed?  Just ignore the json.cpp and jsarray.cpp changes now.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, it actually turns out this doesn't work on MSVC; assertions don't fire since the temporary argument gets destroyed before the temporary JSAutoTempValueRooter.

It doesn't do anything harmful on MSVC; it just doesn't give any useful feedback.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for JSAutoTempValueRooter

And here's a patch that also works on MSVC, by using a const reference rather than passing by value.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, for the record, I think what MSVC was doing was valid; it was constructing the implicit temporary for pass-by-value that compilers are allowed to omit if they want to (and gcc does), and that one had a shorter lifetime.

Comment 25

[Brendan Eich [:brendan]]

Painful -- Luke, anyone: is there c++[01]x hope for a "explicit named" qualifier pair, or something like that?

/be

Comment 26

[Igor Bukanov]

There are more classes than the patch covers. JSAutoRequest from jsapi.h, JSAutoTempIdRooter and JSAutoEnumStateRooter from jscntxt.h. I will r+ the patch with this adderssed.

Comment 27

[Luke Wagner [:luke]]

(In reply to comment #25)
> Painful -- Luke, anyone: is there c++[01]x hope for a "explicit named"
> qualifier pair, or something like that?

No; this is actually the first time I've seen this error get such attention.

I'm still thinking a non-intrusive static analysis (which, as comment 6 explains, is already mostly done) would achieve the same bug-finding effect without the code mutilation.  Also, as a hypothetical custom-guard writer, I'm more likely to annotate my class with a simple "JS_nontemporary" than insert the three JS_GUARD_OBJECT_* macros in just the right places.

Comment 28

[Jeff Walden [:Waldo]]

Igor, I mentioned the same to dbaron yesterday (although I was thinking more broadly of JSAuto* in general), and he said he wanted to get it working on just the one to start.  (I also did a grep for JSAuto, and none of the other classes had mistakes.)  I'd personally do it all at once, but it seems reasonable to split it up as well.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #27)
> I'm still thinking a non-intrusive static analysis (which, as comment 6
> explains, is already mostly done) would achieve the same bug-finding effect
> without the code mutilation.

It would, for the bugs that are subtle enough to get checked in.

But it wouldn't for the more easily observed bugs that cause people to spend hours debugging something and then discover this tiny little typo that caused it.

Comment 30

[Brendan Eich [:brendan]]

Defense in depth: we want the assertion (ugly boilerplate as a pragmatic move; we are not purists) *and* the static analysis:

* Dbaron's point about prompt developer feedback via the assertion is good.

* The static analysis is justified because all-paths coverage is an exponential problem and we can't rely only on the assertion.

/be

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for JSAuto*

Comment 32

[Igor Bukanov]

Comment on attachment 402909 [details] [diff] [review]
patch for JSAuto*

>+class JSGuardObjectNotifier
>+{
>+private:
>+    JSBool* mStatementDone;
...
>+    ~JSGuardObjectNotifier() {
>+        *mStatementDone = JS_TRUE;
>+    }
...
>+    JSGuardObjectNotificationReceiver() : mStatementDone(JS_FALSE) {}

Nit: replace JSBool JS_TRUE JS_FALSE with bool true false as this is C++ code.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/tracemonkey/rev/d4f8378c0d6e

Perhaps we should mark this fixed-in-tracemonkey and spin off a second bug for the static analysis bit, if there isn't one already?

Comment 34

[Igor Bukanov]

(In reply to comment #33)
> Perhaps we should mark this fixed-in-tracemonkey and spin off a second bug for
> the static analysis bit, if there isn't one already?

bug 519171

Comment 35

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d4f8378c0d6e

Comment 36

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1c77f1438507