make already_AddRefed returnable in registers in non-DEBUG Unix builds

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Depends on 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

This change saves ~300k (!) of binary size on x86-64 Linux.
Ideally the extensive code comment makes it clear what we're trying to do here.

This change *does* mean that we're not clearing out the pointer in moved-from
values.  I'm a little worried about that; people could touch moved-from
already_AddRefed objects in non-DEBUG modes and do UAF-ish things.  But this
concern is balanced by already_AddRefed objects typically being temporary
objects: it's unlikely that users would have access to the moved-from object to
do bad things anyway.
Attachment #8955199 - Flags: review?(mh+mozilla)
CC'ing people who are interested in codesize wins.
Nice! The assertions were hard-won, so it's important to preserve them, but I think it's worth losing the null-out on release builds for a codesize (and presumably also perf) win like this.
Comment on attachment 8955199 [details] [diff] [review]
make already_AddRefed returnable in registers in non-DEBUG Unix builds

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

It seems to me removing the aOther.take() (which effectively nulls out aOther) smells like it could open the door to exploits. Tom, what do you think?
Attachment #8955199 - Flags: review?(mh+mozilla) → review?(tom)
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8955199 [details] [diff] [review]
> make already_AddRefed returnable in registers in non-DEBUG Unix builds
> 
> Review of attachment 8955199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems to me removing the aOther.take() (which effectively nulls out
> aOther) smells like it could open the door to exploits.

I'm unconvinced. Fundamentally, aOther is not reachable if it came from a bonafide rvalue, and thus is only reachable if somebody Move()d a named already_AddRefed<Foo> to coerce it into an rvalue-reference. And that's an anti-pattern which I've never seen in Gecko - already_AddRefed is used for return values, and anybody who wants to store a temporary reference assigns it into a named RefPtr. RefPtrs often get Move()d, but that doesn't concern us here.
Comment on attachment 8955199 [details] [diff] [review]
make already_AddRefed returnable in registers in non-DEBUG Unix builds

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

Yea, the UAF concerns Nathan pointed out are definitely present. I'm not sure how common uses that we might be hit by are though. Bobby's mentioned that it'd really only affects uses not used as a return value. SF regexes like *.cpp : 'already_addRefed([^\(\)]+);' and similar fiddling definitely don't show much non-return-value use (although it's non-zero).

So if it's not going to affect any major use cases, it seems ok?  I have a few hesitations, but I don't want to block it, especially given the argument from Bobby..

I would request one thing: since this doesn't help anything on Windows, can we keep the old behavior there?

There had been discussions about an --enable-application-hardening flag that would let distros/downstreams change characteristics in the browser beyond just compilation flags like --enable-hardening. None of the other things we could stick under it have been finished yet, but this seems like something that could, so I think I'll file a followup to do that.

::: mfbt/AlreadyAddRefed.h
@@ +91,5 @@
> +  // since the assert in question is compiled only for DEBUG, we can make the
> +  // destructor trivial in non-DEBUG modes by simply defining it with `=default`.
> +  //
> +  // We now have to make the move constructor trivial as well.  It is normally
> +  // non-trivial, because the incoming object is cleared out to satisfy the

"because the incoming object is cleared out to satisfy the"

I got confused by this reading through, I'd suggest:

We now have to make the move constructor trivial as well.  It is normally non-trivial, because the incoming object has its pointer null-ed during the process. This to satisfy the assert in its destructor. But since that destructor has no assert in non-Debug builds, the clearing is unnecessary there; all we really need to perform is a copy of the pointer from the incoming object.

@@ +92,5 @@
> +  // destructor trivial in non-DEBUG modes by simply defining it with `=default`.
> +  //
> +  // We now have to make the move constructor trivial as well.  It is normally
> +  // non-trivial, because the incoming object is cleared out to satisfy the
> +  // assert in the dstructor.  But that clearing is unnecessary in non-DEBUG

nit: Typo
Attachment #8955199 - Flags: review?(tom)
(In reply to Bobby Holley (:bholley) from comment #5)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Comment on attachment 8955199 [details] [diff] [review]
> > make already_AddRefed returnable in registers in non-DEBUG Unix builds
> > 
> > Review of attachment 8955199 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It seems to me removing the aOther.take() (which effectively nulls out
> > aOther) smells like it could open the door to exploits.
> 
> I'm unconvinced. Fundamentally, aOther is not reachable if it came from a
> bonafide rvalue, and thus is only reachable if somebody Move()d a named
> already_AddRefed<Foo> to coerce it into an rvalue-reference. And that's an
> anti-pattern which I've never seen in Gecko - already_AddRefed is used for
> return values, and anybody who wants to store a temporary reference assigns
> it into a named RefPtr. RefPtrs often get Move()d, but that doesn't concern
> us here.

Is there any static analysis that could help make sure people are doing the right thing with already_AddRefed?
(In reply to David Major [:dmajor] from comment #7)
> Is there any static analysis that could help make sure people are doing the
> right thing with already_AddRefed?

I think the clang static analysis could likely be augmented to check for this without too much trouble.
Comment on attachment 8955199 [details] [diff] [review]
make already_AddRefed returnable in registers in non-DEBUG Unix builds

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

One thought:

::: mfbt/AlreadyAddRefed.h
@@ +104,5 @@
> +  // more strigent and are basically impossible for already_AddRefed to
> +  // satisfy[2].
> +  //
> +  // [1] https://itanium-cxx-abi.github.io/cxx-abi/abi.html#non-trivial
> +  // [2] https://docs.microsoft.com/en-us/cpp/build/return-values-cpp

Reading the MS docs, it doesn't seen impossible to optimize on Windows.
However, it would probably make it easier to defeat already_AddRefed defenses, by making it pretty much a dumb struct.
The main question would then be: Is this optimization worth the extra risks? And can we mitigate these with more static analysis? -- As may be necessary on other platforms anyway.
(In reply to David Major [:dmajor] from comment #7)
> Is there any static analysis that could help make sure people are doing the
> right thing with already_AddRefed?

I believe that we can just annotate already_AddRefed as a MOZ_TEMPORARY_CLASS, which surprisingly, causes no build errors (the Valgrind failure is unrelated):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51528055f3840ce146dd7d3a1b929ba777290ccd

(In reply to Gerald Squelart [:gerald] from comment #9)
> ::: mfbt/AlreadyAddRefed.h
> @@ +104,5 @@
> > +  // more strigent and are basically impossible for already_AddRefed to
> > +  // satisfy[2].
> > +  //
> > +  // [1] https://itanium-cxx-abi.github.io/cxx-abi/abi.html#non-trivial
> > +  // [2] https://docs.microsoft.com/en-us/cpp/build/return-values-cpp
> 
> Reading the MS docs, it doesn't seen impossible to optimize on Windows.

The "no user-defined constructor" requirement seems potentially painful.  (I think it can be done, but it would necessitate a lot of source code changes.)

(In reply to Tom Ritter [:tjr] from comment #6)
> I would request one thing: since this doesn't help anything on Windows, can
> we keep the old behavior there?

I didn't mention this before because I didn't think about it, but there is a size win on Windows, because we're not zeroing out members in the destructor, even if there's not a change in the parameter passing.  I don't know if that's translatable to numbers in Talos or similar. 

> ::: mfbt/AlreadyAddRefed.h
> @@ +91,5 @@
> > +  // since the assert in question is compiled only for DEBUG, we can make the
> > +  // destructor trivial in non-DEBUG modes by simply defining it with `=default`.
> > +  //
> > +  // We now have to make the move constructor trivial as well.  It is normally
> > +  // non-trivial, because the incoming object is cleared out to satisfy the
> 
> "because the incoming object is cleared out to satisfy the"
> 
> I got confused by this reading through, I'd suggest:
> 
> We now have to make the move constructor trivial as well.  It is normally
> non-trivial, because the incoming object has its pointer null-ed during the
> process. This to satisfy the assert in its destructor. But since that
> destructor has no assert in non-Debug builds, the clearing is unnecessary
> there; all we really need to perform is a copy of the pointer from the
> incoming object.

This is better, thank you.

(In reply to Bobby Holley (:bholley) from comment #5)
> I'm unconvinced. Fundamentally, aOther is not reachable if it came from a
> bonafide rvalue, and thus is only reachable if somebody Move()d a named
> already_AddRefed<Foo> to coerce it into an rvalue-reference. And that's an
> anti-pattern which I've never seen in Gecko - already_AddRefed is used for
> return values, and anybody who wants to store a temporary reference assigns
> it into a named RefPtr. RefPtrs often get Move()d, but that doesn't concern
> us here.

Named already_AddRefed objects come about through arguments, and either assigned to a RefPtr, or Move()'d into a RefPtr, e.g.:

https://searchfox.org/mozilla-central/source/parser/html/nsHtml5DocumentBuilder.h#33-36
https://searchfox.org/mozilla-central/source/layout/style/nsCSSValue.cpp#3121-3129

I think these sort of usages are still safe because they go through ::take(), which would perform the zeroing.
Also, the measured win is smaller on automation; PGO builds get ~150k smaller.  Still nice!
This change makes the mental model that we have of already_AddRefed match up
with the mental model the static analysis has of already_AddRefed.
Attachment #8956100 - Flags: review?(nika)
Comment on attachment 8956100 [details] [diff] [review]
make already_AddRefed a MOZ_TEMPORARY_CLASS

\o/! Last time I tried to add this annotation it didn't work because we actually stored these on the heap! I'm glad that's fixed now :-).

r=me
Attachment #8956100 - Flags: review?(nika) → review+
I'm not 100% clear on what the reviews decided.  I think the options advanced were:

1. Do nothing.
2. Change already_AddRefed to not null out on destruction, but continue to null out on Windows.
3. Change already_AddRefed to not null out on destruction everywhere.

I'm in favor of #2 or #3, leaning towards #3, especially with the static analysis in place to (mostly) stop people from doing weird things.  (I think you can do something like:

  f(already_AddRefed<T> aTemp)
  {
    // The prototype here is g(already_AddRefed<T>).
    g(Move(aTemp));

    // aTemp is still holding a pointer, even though it's probably been consumed by g.

    // Then, later, do this, which takes the pointer and nulls out aTemp.
    RefPtr p(aTemp);
  }

That'd be non-idiomatic Gecko code, but it's certainly possible to do.  We talked about a use-after-move static analysis that would complain about the above case, but I don't think we actually built one?)

I think it's a little weird to single out Windows for a data structure behavior, even if we're making this particular change for the benefit of non-Windows platforms.

Tom, glandium, in light of the static analysis and previous discussion, which of the above options do you favor?
Flags: needinfo?(tom)
Flags: needinfo?(mh+mozilla)
I prefer #3, FWIW, along with prioritizing a use-after-move static analysis. I really dislike platform divergence in core code.
Depends on: 1443265
Sure, with the static analysis preventing misuse, plus some wins on Windows, not diverging on Windows sounds fine.
Flags: needinfo?(tom)
With static analysis in place, I agree to go with #3.
Flags: needinfo?(mh+mozilla)
bug 1186706 is the move static analysis bug, FTR.
(In reply to Nika Layzell [:mystor] from comment #18)
> bug 1186706 is the move static analysis bug, FTR.

I would like to see that prioritized, but IMO the MOZ_TEMPORARY_CLASS annotation is enough to land the patch in this bug.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c5d61d51ed
make already_AddRefed returnable in registers in non-DEBUG Unix builds; r=glandium,tjr
(In reply to Pulsebot from comment #20)
> Pushed by nfroyd@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/32c5d61d51ed
> make already_AddRefed returnable in registers in non-DEBUG Unix builds;
> r=glandium,tjr

To be clear, this now applies to windows as well, right?
Flags: needinfo?(nfroyd)
(In reply to Bobby Holley (:bholley) from comment #21)
> (In reply to Pulsebot from comment #20)
> > Pushed by nfroyd@mozilla.com:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/32c5d61d51ed
> > make already_AddRefed returnable in registers in non-DEBUG Unix builds;
> > r=glandium,tjr
> 
> To be clear, this now applies to windows as well, right?

already_AddRefed will not be returned in registers on Windows, even after this patch.  There is a codesize win from this patch on Windows, though.  Does that answer your question?
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #22)
> (In reply to Bobby Holley (:bholley) from comment #21)
> > (In reply to Pulsebot from comment #20)
> > > Pushed by nfroyd@mozilla.com:
> > > https://hg.mozilla.org/integration/mozilla-inbound/rev/32c5d61d51ed
> > > make already_AddRefed returnable in registers in non-DEBUG Unix builds;
> > > r=glandium,tjr
> > 
> > To be clear, this now applies to windows as well, right?
> 
> already_AddRefed will not be returned in registers on Windows, even after
> this patch.  There is a codesize win from this patch on Windows, though. 
> Does that answer your question?

Yes. My point is that the commit message is ambiguous as to whether the patch impacts windows or not. It does impact windows, but the specific benefit highlighted in the commit message does not.
https://hg.mozilla.org/mozilla-central/rev/32c5d61d51ed
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.