928507 - JS::Handle's assignment operator is not properly hidden

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Although the intention was for JS::Handle not to support assignment, the assignment operator is not properly hidden, and various bits of code do actually assign Handles.

At the moment, the Handle definition in RootingAPI.h reads:

private:
...
    template <typename S>
    void operator=(S v) MOZ_DELETE;

But according to C++11 [class.copy]:

    A user-declared copy assignment operator X::operator= is a
    non-static non-template member function ofclass X with exactly one
    parameter of type X, X&, const X&, volatile X& or const volatile
    X&."

So templates are never default assignment operators, and that template member function does not make Handle's assignment operator private.

Indeed, CompileOptions, C1Spewer, and IonSpewer all assign Handles.

Comment 1

[Jim Blandy :jimb]

Attachment: Properly hide JS::Handle's assignment operator; add 'repoint' method to deal with the fallout; fix C1Spewer, IonSpewer, and CompileOptions.

Comment 2

[Jim Blandy :jimb]

Build-only try push:
https://tbpl.mozilla.org/?tree=Try&rev=387820bc8079

Comment 3

[Jason Orendorff [:jorendorff]]

Yow! Good find.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 819179 [details] [diff] [review]
Properly hide JS::Handle's assignment operator; add 'repoint' method to deal with the fallout; fix C1Spewer, IonSpewer, and CompileOptions.

Review of attachment 819179 [details] [diff] [review]:
-----------------------------------------------------------------

More grey hair. r=me

Comment 5

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a92ac47a8ba7

Comment 6

[Jeff Walden [:Waldo]]

I'm concerned about the idea of a "repoint" method.  Consider:

http://logbot.glob.com.au/?c=mozilla%23jsapi&s=18+Oct+2013&e=18+Oct+2013

[08:24] <Waldo> not that anyone around here is possibly considering it because of the time, but Handle::repoint has the potential to introduce stack lifetime issues, just occurred to me
[08:25] * Ms2ger tries to exploit
[08:27] <Waldo> Handle h1 = something; { Rooted r(cx, blech); Handle h2 = r; h1 = h2; } /* h1 is garbage */
[08:27] <Waldo> or h1.repoint(h2) or however you might want to spell it

I think we should consider lifetime more carefully before we decide to go joyously forward using a "repoint" method, because it's only safe if the location referred to by the RHS handle will live longer than the target of the assignment.

Comment 7

[Jim Blandy :jimb]

I'm not in favor of joyously going forward with a repoint method either. Rather, we were already using it, but calling it operator= and not being aware of doing so. But, at least, we can quickly find and fix the places where it occurs, and take the traditional steps to prevent other accidental uses from being introduced.

Comment 8

[Terrence Cole [:terrence]]

Just midaired with Jim. What he said x2.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> I think we should consider lifetime more carefully before we decide to go
> joyously forward using a "repoint" method, because it's only safe if the
> location referred to by the RHS handle will live longer than the target of
> the assignment.

This is already true of several rooting API's. As this patch changes nothing but makes these dangerous sites trivially greppable, I think we should land ASAP to prevent further accidental usage and discuss fixing the existing places in a separate bug or bugs.

Comment 9

[Jim Blandy :jimb]

To underscore that comment 7 is vociferous agreement, and not petulant defensiveness, I have filed bug 929314, to remove Handle::repoint. I'm also working on the patches that would make that possible in bug 892643.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a92ac47a8ba7