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

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Yow! Good find.
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
Attachment #819179 - Flags: review?(terrence) → review+
Blocks: 773686
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92ac47a8ba7
Flags: in-testsuite-
Target Milestone: --- → mozilla27
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.
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.
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.
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.
https://hg.mozilla.org/mozilla-central/rev/a92ac47a8ba7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.