1438955 - Crash @ IMMReleaseContext

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jeromie Clark]

While investigating a crash reported in our public forums, we observed what we believe is a crash in Firefox's native 64-bit NPAPI sandbox on Windows.

Out of an abundance of caution, I've tagged the issue as security, just so that it doesn't get posted publicly before you get a chance to look at it.

Here's a representative crash report:
https://crash-stats.mozilla.com/report/index/a62fb573-3282-4e70-bc5b-e73e50180118

If you have any questions, or need additional details like actual Flash Player symbols, feel free to shoot me an email.

Our developer's analysis: 

This could very well be an issue with Mozilla's sandbox. The last call on the stack from us before the win32 dll level stuff is:

DoImmAssociateContextEx

In the 32-bit Firefox sandbox, this call DoImmAssociateContextEx is something that we marshal over to the broker process for handling. I imagine that Mozilla may have to do some similar kind of interception/marshaling or else they could run into crash/hang issues.

Worth filing a bug with them to let them know that this call is causing an issue, and that they may need to modify their handling of that particular API call. FWIW, we don't appear to have any fancy logic for this call in our broker. We simply call the DoImmAssociateContextEx API from the broker process with the params passed in as-is.

Comment 1

[Jeromie Clark]

Since he provided a crash dump, we just looked at a number of representative crash reports to conduct the analysis.

Here are the repro steps, as provided by the user, just in case they come in handy:  

I play on online game call War commander through a website called kixeye.com. In the last few weeks (as of 2nd December) when i play on line and when I go full screen within the game the game crashes. I can play the game in normal screen with out any problems. The error message states 'plug in container for fire fox has stopped working'. And i get a message to close.

Comment 2

[:Gijs (he/him)]

Looks like the XUL code on the stack is from bug 1208944. :masayuki or :m_kato, do you have any ideas about what's going on here?

Comment 3

[Makoto Kato [:m_kato]]

This might be on Windows 8 only.  We cannot hook This API on Windows 10 because API entry is too short.  (Windows 10 will be mov rax, 0 and ret only).   But I need check the code on Windows 8 next week.

Comment 4

[Makoto Kato [:m_kato]]

Do you know wow to reproduce this?  It seems to be on Windows 8 only.

Comment 5

[Makoto Kato [:m_kato]]

s/wow/how/

Comment 6

[Makoto Kato [:m_kato]]

(Gecko cannot get current window class of HWND, so we calls imm32!ImmGetContext simply with original parameter...)

Comment 7

[Makoto Kato [:m_kato]]

And we can hook ImmAssociateContextEx when sHookIMC is HIMC, but I want to test it whether it is fixed.

Comment 8

[Jeromie Clark]

I haven't been able to reproduce this myself, but I think it's because I don't have representative hardware.  

As an aside, along the lines of the strong correlation to Win8, it's almost always amd64, not intel x64.  

Here's the reporter's original bug.  He includes repro steps and a video: 
https://tracker.adobe.com/#/view/FP-4198730

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Jeromie Clark from comment #8)
> I haven't been able to reproduce this myself, but I think it's because I
> don't have representative hardware.  

Hmm, I cannot reproduce this on my test.

Our hook method (ImmGetContextProc) doesn't hook ImmGetContext, so it calls ImmGetContext directly.  But this crash occurs in ImmLockClientImc ...

> As an aside, along the lines of the strong correlation to Win8, it's almost
> always amd64, not intel x64.

bp-a62fb573-3282-4e70-bc5b-e73e50180118 is Intel(R) Core(TM) i7-3770K CPU.
 
> Here's the reporter's original bug.  He includes repro steps and a video: 
> https://tracker.adobe.com/#/view/FP-4198730

Thanks.  I am still investigating this...

Comment 10

[Makoto Kato [:m_kato]]

Ah, maybe, ImmLockClinetImc'a first parameter is null.  So this might occurs.

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Remove ImmReleaseContext hook

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8956300 [details] [diff] [review]
Remove ImmReleaseContext hook

Root code is near Windows 8's ImmReleaseContext code.

ImmReleaseContext:
  mov eax, 1
  ret
ImmReleaseContext+0x6 (ImmLockCLientImc+0x10d)
  xor eax, eax
  add rsp, 20h
  pop rsi
  ret

After injecting ImmReleaseContext by our hook, we override ImmReleaseContext code like the following.

imm32!ImmReleaseContext:
000007ff`0a2419f0 49bb44c079e9fe070000 mov r11,offset xul!mozilla::plugins::PluginInstanceChild::ImmReleaseContextProc (000007fe`e979c044)
000007ff`0a2419fa 41ffe3          jmp     r11
000007ff`0a2419fd c3              ret
000007ff`0a2419fe 4c8bfb          mov     r15,rbx
000007ff`0a241a01 e98b040000      jmp     imm32!ImmSetActiveContext+0x181 (00000

So ImmLockCLientImc+0x10d (ImmReleaseContext+6) becomes the following after injection.  So when ImmLockCLientImc calls ImmLockClientImc+0x10d, it runs illegal code.

imm32!ImmLockClientImc+0x10d:
000007ff`0a2419f6 fe07            inc     byte ptr [rdi]
000007ff`0a2419f8 0000            add     byte ptr [rax],al
000007ff`0a2419fa 41ffe3          jmp     r11


ImmReleaseContext hook required on Windows XP since it destroyed internal HIMC object.  But Windows 7+'s implementation is the following

0:007> u imm32!ImmReleaseContext
IMM32!ImmReleaseContext:
00007ffe`bf5c5340 b801000000      mov     eax,1
00007ffe`bf5c5345 c3              ret

It means that

BOOL ImmReleaseContext(HWND, HIMC)
{
  return TRUE;
}

So ImmReleseContext doesn't destroy/release HIMC object and this API keeps for old compatibility, so we should remove this hook since this is unnecessary.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8956300 [details] [diff] [review]
Remove ImmReleaseContext hook

(In reply to Makoto Kato [:m_kato] from comment #12)
> After injecting ImmReleaseContext by our hook, we override ImmReleaseContext
> code like the following.
> 
> imm32!ImmReleaseContext:
> 000007ff`0a2419f0 49bb44c079e9fe070000 mov r11,offset
> xul!mozilla::plugins::PluginInstanceChild::ImmReleaseContextProc
> (000007fe`e979c044)
> 000007ff`0a2419fa 41ffe3	  jmp	  r11
> 000007ff`0a2419fd c3		  ret
> 000007ff`0a2419fe 4c8bfb	  mov	  r15,rbx
> 000007ff`0a241a01 e98b040000	  jmp	  imm32!ImmSetActiveContext+0x181
> (00000

Hmm, I still don't understand *why* injecting ImmReleaseContext breaks existing implementation like this. Isn't it implemented like vtable?

> 0:007> u imm32!ImmReleaseContext
> IMM32!ImmReleaseContext:
> 00007ffe`bf5c5340 b801000000      mov     eax,1
> 00007ffe`bf5c5345 c3              ret
> 
> It means that
> 
> BOOL ImmReleaseContext(HWND, HIMC)
> {
>   return TRUE;
> }
> 
> So ImmReleseContext doesn't destroy/release HIMC object and this API keeps
> for old compatibility, so we should remove this hook since this is
> unnecessary.

Okay, but please check on Win8.1 and Win10 too. Perhaps, WinXP must have returned FALSE or crashed if given address is illegal. So, should we have such automated test (or runtime check only in debug build like IMENotification::TextChangeDataBase::Test())?

Additionally, how about if we run the binary in WinXP compatible mode?

>@@ -2125,27 +2112,26 @@ PluginInstanceChild::InitImm32Hook()
>         return;
>     }
> 
>     if (sImm32ImmGetContextStub) {
>         return;
>     }
> 
>     // When using windowless plugin, IMM API won't work due ot OOP.
>+    //
>+    // ImmReleaseContext on Windows 7+ just returns TRUE only, so we don't
>+    // hook this.

s/we don't hook/we don't need to hook

And according to this conclusion, can we remove calls of ImmReleaseContext() from our code too?

Comment 14

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13)
> Comment on attachment 8956300 [details] [diff] [review]
> Remove ImmReleaseContext hook
> 
> (In reply to Makoto Kato [:m_kato] from comment #12)
> > After injecting ImmReleaseContext by our hook, we override ImmReleaseContext
> > code like the following.
> > 
> > imm32!ImmReleaseContext:
> > 000007ff`0a2419f0 49bb44c079e9fe070000 mov r11,offset
> > xul!mozilla::plugins::PluginInstanceChild::ImmReleaseContextProc
> > (000007fe`e979c044)
> > 000007ff`0a2419fa 41ffe3	  jmp	  r11
> > 000007ff`0a2419fd c3		  ret
> > 000007ff`0a2419fe 4c8bfb	  mov	  r15,rbx
> > 000007ff`0a241a01 e98b040000	  jmp	  imm32!ImmSetActiveContext+0x181
> > (00000
> 
> Hmm, I still don't understand *why* injecting ImmReleaseContext breaks
> existing implementation like this. Isn't it implemented like vtable?

No, I guess that it depends on compiler.  Compiler looks for empty area (function entry has to be aligned, so if it is small code, it fills a lot of nop) for code size optimization, then it will insert generated code by PGO (Windows OS uses PGO (LEGO as their word)) for nop's area.

 
> > 0:007> u imm32!ImmReleaseContext
> > IMM32!ImmReleaseContext:
> > 00007ffe`bf5c5340 b801000000      mov     eax,1
> > 00007ffe`bf5c5345 c3              ret
> > 
> > It means that
> > 
> > BOOL ImmReleaseContext(HWND, HIMC)
> > {
> >   return TRUE;
> > }
> > 
> > So ImmReleseContext doesn't destroy/release HIMC object and this API keeps
> > for old compatibility, so we should remove this hook since this is
> > unnecessary.
> 
> Okay, but please check on Win8.1 and Win10 too. Perhaps, WinXP must have
> returned FALSE or crashed if given address is illegal. So, should we have
> such automated test (or runtime check only in debug build like
> IMENotification::TextChangeDataBase::Test())?

Yes, I check 7/8/8.1 and 10 (32bit and 64bit).  This issue is 64-bit only (due to injection), so it is unnecessary for XP.  (we don't unsupport XP/2003 64bit at all time)

> 
> Additionally, how about if we run the binary in WinXP compatible mode?

Same.  (I tested on nodepad.exe)

> 
> >@@ -2125,27 +2112,26 @@ PluginInstanceChild::InitImm32Hook()
> >         return;
> >     }
> > 
> >     if (sImm32ImmGetContextStub) {
> >         return;
> >     }
> > 
> >     // When using windowless plugin, IMM API won't work due ot OOP.
> >+    //
> >+    // ImmReleaseContext on Windows 7+ just returns TRUE only, so we don't
> >+    // hook this.
> 
> s/we don't hook/we don't need to hook
> 
> And according to this conclusion, can we remove calls of ImmReleaseContext()
> from our code too?

Possilble, but no good manners...  (Maybe, some static analytics tools may show a leak even if ImmGetContext doesn't allocate new object and adding reference count)

Comment 15

[Makoto Kato [:m_kato]]

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

Comment 16

[Makoto Kato [:m_kato]]

no crash on esr52 according to crash report

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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