Re: [PATCH] imm32: Fixed crashing in ImmGetIMCCSize.

[Nikolay Sivov]

On 7/9/2013 16:50, Qian Hong wrote:
> Fixed a crash in MS Office 2010 Chinese version.
> Thanks Jiang Yike for the helps on testing :)
>
> ---
>  dlls/imm32/imm.c         |    6 ++++++
>  dlls/imm32/tests/imm32.c |   13 +++++++++++++
>  2 files changed, 19 insertions(+)
>
> +    if (IsBadReadPtr(internal, sizeof(HIMCC)))
> +    {
> +        SetLastError(ERROR_INVALID_HANDLE);
> +        return 0;
> +    }
> +

Do you really need this for Office 2010 or null check is enough? If it's passing 0 handle here
that's what you should check imho. If HIMCC is really a handle and not a struct pointer like in wine,
that's not how a handle validity check will look like.

[PATCH] imm32: Fixed crashing in ImmGetIMCCSize.

[Qian Hong]

Hi Nikolay,

Thanks for comments!

On Tue, Jul 9, 2013 at 9:01 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
> Do you really need this for Office 2010 or null check is enough? If it's
> passing 0 handle here that's what you should check imho. If HIMCC is really
> a handle and not a struct pointer like in wine, that's not how a handle
> validity check will look like.

null check is not enough, see the below quoted log:
--- snip ---
0029:Call imm32.ImmGetIMCCSize(bf9c73e5) ret=0d6d3545
--- snip ---

Another example:
--- snip ---
0029:Call imm32.ImmGetIMCCSize(00000190) ret=0a193545
--- snip ---

Could you provide more details for how to check the validity of the
handle in the right way?

Thanks a lot!


--
Regards,
Qian Hong

-
http://www.winehq.org

[PATCH] imm32: Fixed crashing in ImmGetIMCCSize.

[Nikolay Sivov]

On 7/9/2013 18:52, Qian Hong wrote:
> Hi Nikolay,
>
> Thanks for comments!
>
> On Tue, Jul 9, 2013 at 9:01 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
> > Do you really need this for Office 2010 or null check is enough? If it's
> > passing 0 handle here that's what you should check imho. If HIMCC is really
> > a handle and not a struct pointer like in wine, that's not how a handle
> > validity check will look like.
> > null check is not enough, see the below quoted log:
> > --- snip ---
> > 0029:Call imm32.ImmGetIMCCSize(bf9c73e5) ret=0d6d3545
> > --- snip ---
> > This could be some a different bug, and putting exception handler around it
> > is not necessary a right solution.
>
> Another example:
> --- snip ---
> 0029:Call imm32.ImmGetIMCCSize(00000190) ret=0a193545
> --- snip ---
>
> Could you provide more details for how to check the validity of the
> handle in the right way?
> Well, if it's really supposed to be a handle, meaning it's a table index 
> (with some offset or not),
> this check will simply check for index boundaries and whether slot is 
> allocated. This all need
> some tests.
>
> Thanks a lot!
>
> --
> Regards,
> Qian Hong
>
> -
> http://www.winehq.org

[PATCH] imm32: Fixed crashing in ImmGetIMCCSize.

[Qian Hong]

Hello Nikolay,

On Wed, Jul 10, 2013 at 1:06 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote:
> This could be some a different bug, and putting exception handler around it
> is not necessary a right solution.

Good catch, I think you are right.

I found ImmLockIMC()/ImmUnLockIMC()/etc are called even after
ImmDestroyContext(), the latter frees the input method context if the
call successes, but the former try to access the IMC.

I have a test case show that in some case native ImmDestroyContext()
fails but builtin ImmDestroyContext() success:
https://testbot.winehq.org/JobDetails.pl?Key=26499&log_206=1#k206

That is why Office die on Wine but live on Windows,
ImmDestroyContext() fails so following
ImmLockIMC()/ImmUnLockIMC()/ImmGetIMCCSize() are safe, but on Wine
ImmDestroyContext() success and free the memory of the IMC, and...
Boom!

--- snip ---
Call imm32.ImmDestroyContext(08a29b28) ret=3927e447
Ret  imm32.ImmDestroyContext() retval=00000001 ret=3927e447 /*
success, free the IMC (hIMC == 0x08a29b28) */
Call imm32.ImmLockIMC(08a29b28) ret=09ea2c54 /* would die sooner or later ... */
Ret  imm32.ImmLockIMC() retval=08a29b2c ret=09ea2c54
...
Call imm32.ImmGetIMCCSize(00000008) ret=09ea3545 /* 0x00000008 comes
from hIMC->IMC.hPrivate, unfortunately hIMC is freed... */
--- snip ---

I need more tests to figure out what is the necessary and sufficient
conditions to make ImmDestroyContext fails.

Thanks for the help!

-- 
Regards,
Qian Hong

-
http://www.winehq.org

[PATCH] imm32: Fixed crashing in ImmGetIMCCSize.

[Qian Hong]

Forgot to say, as described in the last post, this patch doesn't kill
the culprit, please reject the patch, thanks.