Add some additional instructions to x64 detours patcher

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

Trunk
mozilla47
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45+ wontfix, firefox46+ wontfix, firefox47 affected, firefox52 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
It can't handle CreateWindowExW
(Assignee)

Updated

3 years ago
Blocks: 1240320
status-firefox45: --- → affected
[Tracking Requested - why for this release]: Merge day bustage.
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
(Assignee)

Comment 2

3 years ago
Also: there's a problem with the x64 detours patcher where it will clobber r11 for a jmp even if the code in the trampoline is already using r11!

eg in the CreateWindowExW case, the trampoline code is already using r11 and eax, leaving r10 as the only volatile register available for writing without harming correctness!
(Assignee)

Comment 3

3 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8709654 - Flags: review?(ehsan)
(Assignee)

Comment 4

3 years ago
I opted to use a different type of jmp to prevent r11 clobbering. It was either that or add a bunch of bookkeeping to see which volatile registers had been clobbered in the trampoline.

Comment 5

3 years ago
Comment on attachment 8709654 [details] [diff] [review]
Patch

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

Thanks!  Please add a test too.
Attachment #8709654 - Flags: review?(ehsan) → review+
(Assignee)

Comment 6

3 years ago
Posted patch Patch (r2) (obsolete) — Splinter Review
Added test as requested, carrying forward r+
Attachment #8709654 - Attachment is obsolete: true
Attachment #8709745 - Flags: review+
(Assignee)

Comment 7

3 years ago
Posted patch Patch (inbound) (obsolete) — Splinter Review
Patch (r2) is for Aurora 45. This one is rebased to inbound 46. Carrying forward r+.
Attachment #8709750 - Flags: review+
(Assignee)

Comment 9

3 years ago
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

Approval Request Comment
[Feature/regressing bug #]: Windows DLL Interceptor + plugin async init
[User impact if declined]: Assertions, crashes, bad things on x64 with async init enabled
[Describe test coverage new/current, TreeHerder]: Updated regression test
[Risks and why]: Low, the patch works correctly as tested manually and with the unit test.
[String/UUID change made/needed]: None
Attachment #8709745 - Flags: approval-mozilla-aurora?

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a786af9186eb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Comment 11

3 years ago
Is there any reason to keep the test specific to 64-bit? <https://hg.mozilla.org/mozilla-central/rev/a786af9186eb#l1.12>
Flags: needinfo?(aklotz)
(Assignee)

Comment 12

3 years ago
I suppose not, though on 32-bit it'll just do a nop-space patch. I can land a follow-up.
Flags: needinfo?(aklotz)
(Assignee)

Comment 13

3 years ago
Actually I should force a detour in the 32-bit test case.
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

This makes RELEASE_BUILD angry. The latest Try push shows mass "application terminated with exit code 3221225477" test failures, and I can reproduce that locally when running the Try build via mozregression as well. On my end, it appeared to be a shutdown crash, though.
Attachment #8709745 - Flags: feedback-
(Assignee)

Comment 15

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> Comment on attachment 8709745 [details] [diff] [review]
> Patch (r2)
> 
> This makes RELEASE_BUILD angry. The latest Try push shows mass "application
> terminated with exit code 3221225477" test failures, and I can reproduce
> that locally when running the Try build via mozregression as well. On my
> end, it appeared to be a shutdown crash, though.

Was this only on 64-bit builds, by any chance?
Flags: needinfo?(ryanvm)
(Assignee)

Comment 16

3 years ago
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #15)
> Was this only on 64-bit builds, by any chance?

n/m, of course it is. This is interesting -- I was experiencing the same problem briefly when I first wrote the Optimus detection patch. I'm kind of worried that something bad is happening in the interceptor's destructor.
Flags: needinfo?(ryanvm)
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

Fix crashes, taking it.
Attachment #8709745 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox45: ? → +
tracking-firefox46: ? → +
I don't think we should take this patch on Aurora until comments 14+ are addressed or we're going to have across the board Win64 permafail on Beta after Monday's uplift.
Flags: needinfo?(sledru)
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

oups, sorry
Flags: needinfo?(sledru)
Attachment #8709745 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?

Comment 20

3 years ago
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #12)
> I suppose not, though on 32-bit it'll just do a nop-space patch. I can land
> a follow-up.

Yes please!
Backed out along with bug 1240977 under the suspicion that this was the cause of bug 1241921.
https://hg.mozilla.org/mozilla-central/rev/5f7c184ccd80
Status: RESOLVED → REOPENED
status-firefox46: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla46 → ---
(Assignee)

Comment 22

3 years ago
Comment on attachment 8709745 [details] [diff] [review]
Patch (r2)

Cancelling this for now
Attachment #8709745 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

3 years ago
Posted patch Patch for 45 beta (r3) (obsolete) — Splinter Review
Resubmitting for review. The main change from the previous rev is the writing out of trampoline bytes when pJmp32 >= 0. I was using nBytes as the base index when I should have been using pJmp32 as the base index.
Attachment #8709745 - Attachment is obsolete: true
Attachment #8716088 - Flags: review?(ehsan)

Updated

3 years ago
Attachment #8716088 - Flags: review?(ehsan) → review+
(Assignee)

Comment 25

3 years ago
This patch is the same as beta but merged for inbound / aurora landing.
Attachment #8709750 - Attachment is obsolete: true
Attachment #8717085 - Flags: review+
(Assignee)

Comment 27

3 years ago
Comment on attachment 8717085 [details] [diff] [review]
Patch (for inbound and aurora)

Approval Request Comment
[Feature/regressing bug #]: Windows DLL Interceptor + plugins
[User impact if declined]: Assertions, crashes, bad things on x64
[Describe test coverage new/current, TreeHerder]: Updated regression test
[Risks and why]: Low, the patch works correctly as tested manually and with the unit test.
[String/UUID change made/needed]: None
Attachment #8717085 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 28

3 years ago
Comment on attachment 8716088 [details] [diff] [review]
Patch for 45 beta (r3)

Approval Request Comment
[Feature/regressing bug #]: Windows DLL Interceptor + plugins
[User impact if declined]: Assertions, crashes, bad things on x64
[Describe test coverage new/current, TreeHerder]: Updated regression test
[Risks and why]: Low, the patch works correctly as tested manually and with the unit test.
[String/UUID change made/needed]: None
Attachment #8716088 - Flags: approval-mozilla-beta?

Comment 29

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e4e5e584c32
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8716088 [details] [diff] [review]
Patch for 45 beta (r3)

Fix a crash, taking.
Should be in 45 beta 5.
Attachment #8716088 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8717085 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 31

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d6412cfea15
status-firefox46: affected → fixed
backedout from mozilla-central and will back this also out from aurora/beta for comment #33
Status: RESOLVED → REOPENED
Flags: needinfo?(aklotz)
Resolution: FIXED → ---
backed out from beta and aurora now too
status-firefox45: fixed → affected
status-firefox46: fixed → affected
status-firefox47: fixed → affected
I suspect my bustage today is related to this, bug 1247741.
in danger of missing the beta cycle
Flags: needinfo?(sledru)
Benjamin, do we care about this with async plugin=off?
Flags: needinfo?(sledru) → needinfo?(benjamin)
Redirecting to aklotz. My understanding was that this patch was required to get correct Flash behavior on x86-64, but he's the expect.
Flags: needinfo?(benjamin)
(Assignee)

Comment 42

3 years ago
It's less likely to be an issue with async init off, but I think to be certain we should throw some QA at it on Win64. Basically general Flash usage, making sure there aren't any hangs or stability issues.

FWIW I don't think I can just pull a solution out of a hat on this one -- these NVIDIA crashes that are induced by these changes are very tricky to debug.
Flags: needinfo?(aklotz)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1241921
OK, let's wontfix it for 45 & 46 then.
status-firefox45: affected → wontfix
status-firefox46: affected → wontfix
Aaron recommends that this bug block the 64-bit Firefox roll-out.
Blocks: 558448
(Assignee)

Comment 46

3 years ago
Rebased patch, carrying forward r+.
Attachment #8716088 - Attachment is obsolete: true
Attachment #8717085 - Attachment is obsolete: true
Attachment #8800896 - Flags: review+
(Assignee)

Comment 47

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/609a153be0a59c7cdb79d06c0254dfdeb33dbaa4
Bug 1240848: Adds additional instructions to x64 detour patcher and prevents register clobbering in jmp from trampoline; r=ehsan

Comment 48

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/609a153be0a5
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
See Also: → bug 1376177
See Also: bug 1376177
You need to log in before you can comment on or make changes to this bug.