1056410 - More missing callgraph edges involving destructors

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

Bug 1047696 looks to have revealed *another* gap in the rooting hazard analysis.

Before MOZ_FINAL, PrepareForWrapping called '_ZN14XPCCallContextD1Ev' aka |void XPCCallContext::~XPCCallContext()|, and the analysis didn't think that called anything. The analysis is aware of 4 different destructors:

  (#1) _ZN14XPCCallContextD1Ev|void XPCCallContext::~XPCCallContext()
  (#2) _ZN14XPCCallContextD1Ev *INTERNAL* |void XPCCallContext::~XPCCallContext(int32)
  (#3) _ZN14XPCCallContextD1Ev|void XPCCallContext::~XPCCallContext(int32)
  (#4) _ZN14XPCCallContextD0Ev|void XPCCallContext::~XPCCallContext()

which blows my mind -- notice how the mangled name _ZN14XPCCallContextD1Ev resolves to two different unmangled names, and the unmangled name 'void XPCCallContext::~XPCCallContext()' is associated with two different mangled names. *groan* Anyway, #3 above calls all kinds of stuff, including ~nsRefPtr which is what happens with MOZ_FINAL added as seen in the hazard output at https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9d622c91bed/try-linux64-br-haz/hazards.txt.gz

To continue, the *INTERNAL* destructors I already knew about -- they are "not-in-charge" destructors for use with virtual inheritance. When they are identical to the "in-charge" destructors, gcc aliases them to the same thing in some weird way that is not visible in the output provided to the analysis plugin. I previously fixed that problem by pretending that one of them calls the other, since the analysis plugin would not otherwise see a body for one of them at all. (To be honest, I forget which is which.)

But now we seem to have progressed to an even higher degree of insanity. The plugin sees bodies generated for #2 and #4 above, but not for #1 and #3. Without MOZ_FINAL, it sees an edge from PrepareForWrapping to #1. The existing aliasing workaround adds an artificial edge from #3 to #2. There is a natural edge from #4 to #1: the D0 in the mangled name means "deleting destructor", which is a destructor that finalizes and additional frees the actual memory. So gcc generates a body that basically calls #1 and then invokes operator delete.

So without MOZ_FINAL, we have:

  PrepareForWrapping -> #1
  #4 -> #1
  #3 -> #2
  #2 -> (important stuff)

It looks like we're missing an edge from #1 -> #3. Which are the two suspicious ones -- they have the same mangled name, but differ by an int32 parameter.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Add edge between internal destructors

Comment 2

[Andrew McCreight [:mccr8]]

Steve, is this ready to move forward?

Comment 3

[Steve Fink [:sfink] [:s:]]

I don't have a complete fix yet. I added in one edge, which fills in a bunch of the callgraph, but I'm still missing a destructor-related edge in a weird case that I haven't managed to reproduce in a minimal test case. I just need to spend a bit more time on it; I can use the full build as the test case.

Comment 4

[Andrew McCreight [:mccr8]]

Any progress here?  This is two months old now.  Thanks.

Comment 5

[Steve Fink [:sfink] [:s:]]

Update: I still hope to fix this, but there are other things the analysis is missing that I'm in the middle of fixing and backporting right now, and there are other missing edges that I am also working on fixing. So this problem is real, it still exists, but it's really just an example of a pile of things the analysis is missing currently. I have no reason to suspect there are more or less vulnerabilities from this bug than from the other gaps.

I will file a bug for the other main source of missing edges. It should be a little easier to fix, and fill in a few more of the missing edges than this bug.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

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

I'm not sure what a review here means, but adding edges can only be good, right?

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

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

Yeah, let's get it in since we know it finds at least one important path.

Comment 8

[Steve Fink [:sfink] [:s:]]

Ok, finally tracked this down. The edges added in the above patch are good and were missing before. However, they do not reveal any hazards (at least not now; some were fixed.)

The confusion arises from bug 1056410, which marks a whole bunch of classes as final. If nsXPConnect is marked final, then the analysis starts reporting a hazard. I mistakenly lumped this in with the missing destructors issue, since the new GC function in the hazard passes through a couple differently-named destructors. I was thinking that the new |final| designation simplified things enough for the analysis to see an edge through a destructor that it could not see previously.

But it turns out that the |final| designation is only serving to evade an analysis heuristic. Specifically, the analysis ignores all AddRef() and Release() virtual methods of any class inheriting from nsISupports. nsXPConnect implements its own Release() method, and if it is marked |final|, the compile devirtualizes it and the analysis heuristic no longer applies.

I could either spot-fix this by special-casing nsXPConnect::Release, or fix the slightly more general problem by applying the AddRef/Release heuristic for any function call, not just virtual ones.

Alternatively, I could see what it would take for the analysis to figure out by itself that Release calls don't GC. But from skimming those, that looks... challenging.

Comment 9

[Steve Fink [:sfink] [:s:]]

Oops, the bug # in the last comment should have been bug 1047696, as it says in comment 0.

Comment 10

[Steve Fink [:sfink] [:s:]]

Attachment: Mark more classes final

Bug 1047696 was blocked by the hazard here, so it ended up not marking problematic classes final. Mark them final.

Comment 11

[Steve Fink [:sfink] [:s:]]

Attachment: GetNameShared cannot GC

This cleans up a false positive from the Array handling. It prunes back the beginning of the live range for an XPCNativeMember[] variable, and annotates away a trivial accessor function.

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8585706 [details] [diff] [review]
GetNameShared cannot GC

Nope, just this one annotation isn't enough. There are 2 other calls involved.

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: GetNameShared cannot GC

Trying again with https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8d9f7aea44

Comment 14

[Steve Fink [:sfink] [:s:]]

Ugh. So that patch does no harm, but the analysis obnoxiously points out that GetConstant *can* GC for real:

GC Function: _ZN18xptiInterfaceEntry11GetConstantEtN2JS13MutableHandleINS0_5ValueEEEPPc|uint32 xptiInterfaceEntry::GetConstant(uint16, class JS::MutableHandle<JS::Value>, int8**)
    void mozilla::AutoJSContext::~AutoJSContext(int32)
    void mozilla::AutoJSContext::~AutoJSContext(int32)
    void mozilla::dom::AutoJSAPI::~AutoJSAPI()
    void mozilla::dom::AutoJSAPI::~AutoJSAPI(int32)
    void xpc::ErrorReport::LogToConsole()
    FieldCall: nsIScriptError.InitWithWindowID

So this isn't exactly a false positive, since we don't move strings yet. But it's lying in wait. (Well, I don't know if this path really can GC or not, but error reporting stuff typically can.)

I'll have to do something else here. Maybe even root things for real. With an external rooter? An overlaid stack-only class? Something.

Comment 15

[Steve Fink [:sfink] [:s:]]

Attachment: Root XPCNativeMember.name

Sadly, I don't see a way around just rooting this thing. GetConstant can legitimately GC, or at least it's really tough to prove otherwise.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6db8094e78f
https://hg.mozilla.org/mozilla-central/rev/fffed2b68c8e

Leaving this open since it appears there are still patches pending.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8586520 [details] [diff] [review]
Root XPCNativeMember.name

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

mName is interned, right? Given that, I don't see why we need to root it. See XPCNativeInterface::NewInstance.

Comment 18

[Steve Fink [:sfink] [:s:]]

Nothing more is needed here.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I assume things need uplift here? :)

Comment 20

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

Approval Request Comment
[Feature/regressing bug #]: A bit of a mixture. Bug 963738 exposed some problems.

[User impact if declined]: None. The hazard analysis tests will be slightly less accurate for older branches. Backporting later fixes may be a little harder.

[Describe test coverage new/current, TreeHerder]: This has been running as part of the analysis for a month.

[Risks and why]: The only risk is that we will end up with a mismatched set of backports, which would probably require further work to make the analysis happy again on each branch, and in the worst case would cause actual exploitable hazards to be briefly visible in the TH view of these branches. The hazards this bug is dealing with haven't been of that flavor, though.

[String/UUID change made/needed]: None

Comment 21

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8585830 [details] [diff] [review]
GetNameShared cannot GC

Approval Request Comment
[Feature/regressing bug #]: see previous approval request, but this specific patch doesn't fix any real issues at all. It's just for ease of backporting.

[User impact if declined]: None. Approval requested to make backports easier.

[Risks and why]: very low

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

Approval Request Comment
[Feature/regressing bug #]: see above. This patch is also not fixing any real issue.

[User impact if declined]: Probably none, some possibility of a very slight perf hit though I doubt it. Approval is requested for ease of backports.

[Risks and why]: Very low. Changes code generation very very slightly, so I guess anything is theoretically possible.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

Approving for uplift to aurora and ESR38, since this is sec-high and may help with future backporting.    We have already built ESR 38.0 so this would be released in ESR 38.1.0.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Steve, I just notice that this doesn't have sec approval. Could you please request it? 

Kyle can you hold off on uplift to aurora until we have sec approval? Thanks.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Why did I just say "Kyle"? Argh. I was thinking of Kwierso. Also, too many tabs open at once. 

Wes, can you hold off on uplift for these patches till we have sec approval?

Comment 26

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

Can do.

Comment 27

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Highly unlikely

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? back to esr38, I guess?

If not all supported branches, which bug introduced the flaw? This bug is for a category of problems, at this point none of which are serious (due to other fixes previously landing). Tracking this down to specific flaws would be possible, but annoying, and I don't think it really buys us anything here.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I think this patch should apply to all affected branches.

How likely is this patch to cause regressions; how much testing does it need? Given that this only adds edges, it should only be able to cause more hazards to be reported, so the existing hazard analysis tests should be fine for detecting regressions.

Comment 28

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Highly unlikely

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? None. This would be backported only to make other backporting easier.

If not all supported branches, which bug introduced the flaw? N/A. This patch does not fix any flaws (though it does introduce something that the previous code would not have caught; it's just that the same patch fixes that too.)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should backport cleanly.

How likely is this patch to cause regressions; how much testing does it need? I can't think of a way for this to regress anything, barring PGO wackiness.

Comment 29

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8585830 [details] [diff] [review]
GetNameShared cannot GC

[Security approval request comment]
How easily could an exploit be constructed based on the patch? N/A; this patch does not fix anything.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? All of them or none of them, depending on your perspective. I think the relevant code is about the same in all supported branches.

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I think it should work without change.

How likely is this patch to cause regressions; how much testing does it need? If this messes up, it will break the compile, not introduce subtle problems.

The risk with the hazard analysis change here is that we would cover up real hazards. That is very unlikely; it would have to be a flaw introduced by a binary extension that implemented nsIInterfaceInfo.GetMethodInfo or .GetConstant or .GetNameShared with something that can GC, which would be pretty weird. (If in-tree code implements any of them with code that can GC, the analysis won't be fooled by this annotation and will accurately report any hazards thus introduced.)

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Looking at comment 16, is this one of those situations where I'm being asked for sec-approval+ *after* things have been checked in? Because, if it is, I might as well give it since the point of such approval is to avoid early disclosure and we've already done that.

Otherwise, you'll get approval to check in on 5/26, two weeks into the next dev cycle.

Comment 31

[Steve Fink [:sfink] [:s:]]

(In reply to Al Billings [:abillings] from comment #30)
> Looking at comment 16, is this one of those situations where I'm being asked
> for sec-approval+ *after* things have been checked in? Because, if it is, I
> might as well give it since the point of such approval is to avoid early
> disclosure and we've already done that.

Doh! Yes, sorry. And because I keep doing this, I filed bug 1164063.

Comment 32

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8585830 [details] [diff] [review]
GetNameShared cannot GC

On second thought, this patch never landed on m-c and no longer matters. So it's not going to help backporting. I'll just drop it.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

Approved for uplift to beta (39). This already landed on 40 so it no longer needs uplift to aurora.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

Approved for uplift to beta (39). This already landed on 40 so it no longer needs uplift to aurora.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0296484f4f84
https://hg.mozilla.org/releases/mozilla-beta/rev/b964c317dbff

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/6a9a086db35d
https://hg.mozilla.org/releases/mozilla-esr38/rev/7fd4fa3ef5c1

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/af7478dd5b99
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bb9c9ed29fdd

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/49873a7cf403
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ed0520adaf8c

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/49873a7cf403
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/ed0520adaf8c