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.
Hi Nikolay,
Thanks for comments!
On Tue, Jul 9, 2013 at 9:01 PM, Nikolay Sivov bunglehead@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
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@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
Hello Nikolay,
On Wed, Jul 10, 2013 at 1:06 AM, Nikolay Sivov bunglehead@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!
Forgot to say, as described in the last post, this patch doesn't kill the culprit, please reject the patch, thanks.
-
Nikolay Sivov -
Qian Hong