1442304 - make already_AddRefed returnable in registers in non-DEBUG Unix builds

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Nathan Froyd [:froydnj]]

This change saves ~300k (!) of binary size on x86-64 Linux.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: make already_AddRefed returnable in registers in non-DEBUG Unix builds

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.

Comment 2

[Nathan Froyd [:froydnj]]

CC'ing people who are interested in codesize wins.

Comment 3

[Bobby Holley (:bholley)]

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 4

[Mike Hommey [:glandium]]

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?

Comment 5

[Bobby Holley (:bholley)]

(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 6

[Tom Ritter [:tjr]]

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

Comment 7

[(Away)]

(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?

Comment 8

[Bobby Holley (:bholley)]

(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 9

[Gerald Squelart (he/him)]

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.

Comment 10

[Nathan Froyd [:froydnj]]

(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.

Comment 11

[Nathan Froyd [:froydnj]]

Also, the measured win is smaller on automation; PGO builds get ~150k smaller.  Still nice!

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: make already_AddRefed a MOZ_TEMPORARY_CLASS

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.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

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

Comment 14

[Nathan Froyd [:froydnj]]

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?

Comment 15

[Bobby Holley (:bholley)]

I prefer #3, FWIW, along with prioritizing a use-after-move static analysis. I really dislike platform divergence in core code.

Comment 16

[Tom Ritter [:tjr]]

Sure, with the static analysis preventing misuse, plus some wins on Windows, not diverging on Windows sounds fine.

Comment 17

[Mike Hommey [:glandium]]

With static analysis in place, I agree to go with #3.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

bug 1186706 is the move static analysis bug, FTR.

Comment 19

[Bobby Holley (:bholley)]

(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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c5d61d51ed
make already_AddRefed returnable in registers in non-DEBUG Unix builds; r=glandium,tjr

Comment 21

[Bobby Holley (:bholley)]

(In reply to Pulsebot from comment #20)
> Pushed by [e-mail]:
> 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?

Comment 22

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #21)
> (In reply to Pulsebot from comment #20)
> > Pushed by [e-mail]:
> > 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?

Comment 23

[Bobby Holley (:bholley)]

(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 [e-mail]:
> > > 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.

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/32c5d61d51ed