1121829 - Support redirection of kernel32.dll for hooking function

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Makoto Kato [:m_kato]]

follow up bug 1112709.

kernel32.dll and advapi32.dll from Windows 7 redirect to kernelbase.dll etc using  jmp [disp32].

We should support this redirection of NoSpacePatcher and DetourPatcher.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Support rediretion

We should resolve redirected address if function entry is jmp [disp32 (import table)].  It can hook by NoSpacePatcher.

Comment 2

[Benjamin Smedberg]

As a continuation of bug 1112709, this means the Flash protected-mode hook doesn't work yet on Windows 8.1.

Comment 3

[(Away)]

Comment on attachment 8549381 [details] [diff] [review]
Support rediretion

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

I agree that this sounds better than hooking both the -A and -W functions.

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +205,5 @@
>      return true;
>    }
> +
> +private:
> +  byteptr_t ResolveRedirectedAddress(const byteptr_t aOriginalFunction)

Could be static

Comment 4

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dcfe4ca9f1

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/d8dcfe4ca9f1

Comment 6

[Andrei Vaida [:avaida]]

This issue is verified fixed on Nightly 38.0a1 (2015-01-19) using:
 * Windows 8.1 32-bit,
 * Windows 8.1 64-bit (Surface Pro 2).

I had mixed results while testing Windows 8 32-bit though. I used two machines to test it - on one of them the fix didn't work at all, on the other it worked 100% of the times. Detailed information is available in this etherpad [1], line 33.

Makoto, what's your take on this?

[1] https://etherpad.mozilla.org/FlashProtectedMode

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Andrei Vaida, QA [:avaida] from comment #6)
> This issue is verified fixed on Nightly 38.0a1 (2015-01-19) using:
>  * Windows 8.1 32-bit,
>  * Windows 8.1 64-bit (Surface Pro 2).
> 
> I had mixed results while testing Windows 8 32-bit though. I used two
> machines to test it - on one of them the fix didn't work at all, on the
> other it worked 100% of the times. Detailed information is available in this
> etherpad [1], line 33.
> 
> Makoto, what's your take on this?
> 
> [1] https://etherpad.mozilla.org/FlashProtectedMode

I cannot reproduce this on my Windows 8 VM.  Is reproduced environment clean environment (no application or anti-virus install etc)?

Or if you can reproduce this on virtual machine, could you share it?

Comment 8

[Andrei Vaida [:avaida]]

(In reply to Makoto Kato (:m_kato) from comment #7)
> I cannot reproduce this on my Windows 8 VM.  Is reproduced environment clean
> environment (no application or anti-virus install etc)?
> 
> Or if you can reproduce this on virtual machine, could you share it?

The test environments I used were not clean, other applications were installed on them but they were *not in use* at the time. I tested this fix again on two additional machines as well, on both of them Nightly 38.0a1 (2015-01-20) was *still affected*.

Here are the complete test results for Windows 8, across all the test machines I used:
- Windows 8 32-bit (AMD Radeon HD 7700 Series): FAIL
- Windows 8 32-bit (NVIDIA GeForce 620): PASS
- Windows 8 32-bit (AMD Radeon HD 6450): FAIL
- Windows 8 32-bit (ATI Radeon 3000): FAIL

I don't have a vm installed at this point, but I can setup one if you think it's necessary - nevertheless, I think that the most relevant results are those from environments that are as close to a random end user's machine as possible.

Comment 9

[(Away)]

Could you force a crash with the CrashMe extension [1] on the failing machine and share the crash ID. It will tell us the exact version of your system DLLs and may provide other clues also.

[1] http://ted.mielczarek.org/mozilla/crashme.html

Comment 10

[Andrei Vaida [:avaida]]

(In reply to David Major [:dmajor] (UTC+13) from comment #9)
> Could you force a crash with the CrashMe extension [1] on the failing
> machine and share the crash ID. It will tell us the exact version of your
> system DLLs and may provide other clues also.
> 
> [1] http://ted.mielczarek.org/mozilla/crashme.html

Sure, here are the crash IDs:
 * bp-b566a752-f5e0-4ba9-9f91-cecdb2150122
 * bp-392382ec-cb1c-436e-be11-6da942150122
 * bp-2aeb1e66-695b-4e7a-9348-91df32150122

Let me know if you need anything else.

Comment 11

[(Away)]

I don't see anything obvious that would lead to a failure of the hook. The opcode patterns are fine. It's going to be difficult to get much further without a live debugger.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Makoto, could you fill the uplift requests to aurora & beta? Thanks

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8549381 [details] [diff] [review]
Support rediretion

Approval Request Comment
[Feature/regressing bug #]:N/A
[User impact if declined]:
dom.ipc.plugins.flash.disable-protected-mode doesn't work on Windows 8.1.
This preference is turned on from Firefox 37.

[Describe test coverage new/current, TreeHerder]:
landed in m-c and verified by QA team (comment #6)

[Risks and why]: 
too low.  This adds anylze jmp [disp32] only.

[String/UUID change made/needed]:
None

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8549381 [details] [diff] [review]
Support rediretion

Approval Request Comment
[Feature/regressing bug #]:
Related bug is bug 1108035, 1119941, and 1120993.

[User impact if declined]:
dom.ipc.plugins.flash.disable-protected-mode doesn't work on Windows 8.1.
This preference is turn off on Firefox 36, but this discussion is bug 1120993

[Describe test coverage new/current, TreeHerder]:
Landed in m-c and verified by QA team (comment #6)

[Risks and why]: 
too low.  This adds anylze jmp [disp32] only.

[String/UUID change made/needed]:
None

Comment 15

[Andrei Vaida [:avaida]]

Since uplift requests were approved for this patch, I assume that the Windows 8 related issue (see Comment 6) will be treated in a separate bug. Should I file one or do we have it already?

Comment 16

[Makoto Kato [:m_kato]]

for aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/b29971ada024

Comment 17

[Makoto Kato [:m_kato]]

for beta
https://hg.mozilla.org/releases/mozilla-beta/rev/d340f3d3439d

Comment 18

[Makoto Kato [:m_kato]]

(In reply to Andrei Vaida, QA [:avaida] from comment #15)
> Since uplift requests were approved for this patch, I assume that the
> Windows 8 related issue (see Comment 6) will be treated in a separate bug.
> Should I file one or do we have it already?

Could you file new bug?

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Makoto Kato (:m_kato) from comment #18)
> Could you file new bug?

Sure, I've filed Bug 1126185 on this matter.

I'm gonna go ahead and mark this fix as verified on Windows 8.1, per Comment 6. I'll follow up with test results for m-a and m-b as soon as possible.

Comment 20

[Andrei Vaida [:avaida]]

Verified fixed on Firefox 36.0b4 (build1: 20150126151838) and Aurora 37.0a2 (2015-01-26), using:
 - Windows 8.1 32 bit,
 - Windows 8.1 64 bit (Surface Pro 2),
with Shockwave Flash 16.0 r0 (16.0.0.296).