Memory allocation issue in ime.cpp #3234

nvaccessAuto · 2013-05-14T13:54:18Z

Reported by nishimotz on 2013-05-14 13:54
Memory allocation issue in ime.cpp causes irrelevant garbage announces at Windows desktop operations, when IMM-based Japanese input methods are active, such as Google Japanese IME or ATOK.

As in the patch below, malloc(len) is obtaining wrong size of cand_str buffer, and the string may not be initialized if there are no candidates.

diff --git a/nvdaHelper/remote/ime.cpp b/nvdaHelper/remote/ime.cpp
index deeca61..fb41ab3 100644
--- a/nvdaHelper/remote/ime.cpp
+++ b/nvdaHelper/remote/ime.cpp
@@ -326,7 +326,19 @@ static bool handleCandidates(HWND hwnd) {
    }

    /* Concatenate currently shown candidates into a string */
+#if 0
    WCHAR* cand_str = (WCHAR*)malloc(len);
+#else
+   size_t buflen = 1; // make sure len != 0
+   for (DWORD n = list->dwPageStart;  n < pageEnd; ++n) {
+       DWORD offset = list->dwOffset[cand = (WCHAR*)(((char*)list) + offset);
+       size_t clen = wcslen(cand);
+       buflen += (clen + 1) * sizeof(WCHAR);
+   }
+   WCHAR* cand_str = (WCHAR*)malloc(buflen);
+   cand_str[0](n];
+       WCHAR*) = '\0';
+#endif
    WCHAR* ptr = cand_str;
    for (DWORD n = list->dwPageStart, count = 0;  n < pageEnd;  ++n) {
        DWORD offset = list->dwOffset[n];

The text was updated successfully, but these errors were encountered:

nvaccessAuto · 2013-05-14T23:16:04Z

Comment 1 by jteh on 2013-05-14 23:16
I assume the value of len is larger than it should be? Wouldn't this suggest a bug in the input method? (I'm not saying we shouldn't work around it, but am just trying to understand the issue.)

Mick, should this be fixed for 2013.1 or is this too risky at this stage?

nvaccessAuto · 2013-05-27T02:06:58Z

Comment 2 by mdcurran on 2013-05-27 02:06
There may be a bug in the input method itself, but I also think there's a bug in the way we're getting the length anyway.
It looks like we're getting the length spanning all the candidate lists, not just the first one (we only use the first).
Therefore we should get the length of the first candidate list instead. See attached patch.
Please test the patch and make sure this fixes the issue and that nothing breaks.

nvaccessAuto · 2013-05-27T02:08:29Z

Attachment 3234_IMELen.patch added by mdcurran on 2013-05-27 02:08
Description:

nvaccessAuto · 2013-05-28T07:10:20Z

Comment 3 by jteh on 2013-05-28 07:10
Changes:
Milestone changed from None to 2013.1.1

nvaccessAuto · 2013-05-28T13:02:13Z

Comment 4 by nishimotz on 2013-05-28 13:02
I agree that the input method has a bug, but your patch does not resolve the issue.

With Windows XP and ATOK 2013 (Japanese input method), garbage announce still occurs when focus is moving around the Windows start menu items.

The cand_str should be allocated for the sum of actual string lengths of candidates.
Strangely, total size of cand_str may be 0, and in such cases the event should be ignored.
I can't understand why handleCandidates() is called in such cases.

nvaccessAuto · 2013-05-28T20:04:41Z

Comment 5 by mdcurran on 2013-05-28 20:04
Is there any way I can get access to one of these input methods that shows this behavior? I accept that my patch may not fix the issue, but it still corrects a miss-use of the API, plus this now suggests more issues with the candidate list struct... as we should never have to depend on the existance of \0 characters in an arbitrary buffer. I would like to play with this myself a bit more.

nvaccessAuto · 2013-05-28T22:48:23Z

Comment 6 by nishimotz on 2013-05-28 22:48
Google Japanese Input, which is freely available, will reproduce the issue with Windows xp.
Installer is as follows:
https://dl.dropboxusercontent.com/u/62564469/GoogleJapaneseInputSetup.exe

Even garbage announces are hidden by succeeding announce, they can be seen in speech viewer.

nvaccessAuto · 2013-06-11T00:15:53Z

Comment 7 by mdcurran (in reply to comment 6) on 2013-06-11 00:15
Replying to nishimotz:

Google Japanese Input, which is freely available, will reproduce the issue with Windows xp.

Sorry, on XP for me the installation fails with an unknown IME error or something.
On Win7 it seems to install, but either never shows up as an installed IME, or does but causes issues with switching IMEs, including stopping my user account from logging off.
At one point I did see the random candidate chunk speaking issue, but things were not stable enough for me to test properly.
Also, its worth noting that the settings for this IME are rather inaccessible.

nvaccessAuto · 2013-06-11T00:54:15Z

Comment 8 by mdcurran on 2013-06-11 00:54
Since I can't directly test. Here are some more things to try in addition to my patch:
*Only allocate cand_str and do the for loop that fills it in, if (pageEnd-list->dwPageStart)>0 is true.

when allocating cand_str, use calloc instead of malloc (to ensure the memory is all 0).
Though I would hope that the above check would stop the need for this.

If none of those work, can I ask that you add some logging calls so that we can understand what is in the struct. Something like:

LOG_INFO(L"dwPageStart: "<<(list->dwPageStart)<<L", dwPageSize: "<<(list->dwPageSize)<<L", dwCount: "<<(list->dwCount)<<L", pageEnd "<<pageEnd);

nvaccessAuto · 2013-06-13T04:35:10Z

Attachment nvda-3234-130613.log added by nishimotz on 2013-06-13 04:35
Description:
log regarding ticket 3234 on Windows 7 and Google Japanese IME

nvaccessAuto · 2013-06-13T04:36:15Z

Attachment 3234_130613.patch added by nishimotz on 2013-06-13 04:36
Description:
patch regarding nvda-3234-130613.log

nvaccessAuto · 2013-06-13T04:46:58Z

Comment 9 by nishimotz on 2013-06-13 04:46
Attachment 3234_130613.patch is used for testing with Windows 7 32bit and Google Japanese Input.
In addition to your patch, I have replaced malloc with calloc.
No garbage announces are observed so far.

According to the log attached above, I agree that the allocated size of cand_str is always larger than the real usage in inputCandidateListUpdate.

It should be mentioned that the useless allocations of 16988 bytes are observed (when moving around focus within the explorer) as follows:

INFO - RPC process 2408 (explorer.exe) (13:24:22):
Thread 2504, nvdaHelper\build\x86\remote\ime.cpp, handleCandidates, 320:
list allocating bytes: 16988

INFO - RPC process 2408 (explorer.exe) (13:24:22):
Thread 2504, nvdaHelper\build\x86\remote\ime.cpp, handleCandidates, 333:
dwPageStart: 0, dwPageSize: 0, dwCount: 0, pageEnd 0

INFO - RPC process 2408 (explorer.exe) (13:24:22):
Thread 2504, nvdaHelper\build\x86\remote\ime.cpp, handleCandidates, 336:
cand_str allocating bytes: 16988

nvaccessAuto · 2013-06-13T06:14:20Z

Comment 10 by mdcurran on 2013-06-13 06:14
Attached a new patch: 3234_patch3.patch.
This one is a compromise between our two original patches. I.e. Fixes the bad use of the API, but also calculates the full length of all strings.
I'm thinking that calloc may be a bit expensive, and we really shouldn't be allocating all that memory when we're never going to use it.
Please test this patch with both Google Japanese on desktop, and other real Japanese IME candidate lists.
The patch is also based on master - I think that this change is too disruptive for 2013.1.1 as I'm still going to have to test with about 5 Chinese IMEs (many of them which break other rules).
Changes:
Milestone changed from 2013.1.1 to 2013.2

nvaccessAuto · 2013-06-13T12:05:27Z

Comment 11 by nishimotz on 2013-06-13 12:05
Excuse me, is patch3 already available?

nvaccessAuto · 2013-06-13T22:55:41Z

Attachment 3234_patch3.patch added by mdcurran on 2013-06-13 22:55
Description:
Merge of two original patches. Fixes to API usage and does string length calculation.

nvaccessAuto · 2013-06-13T22:57:13Z

Comment 12 by mdcurran (in reply to comment 11) on 2013-06-13 22:57
Replying to nishimotz:

Excuse me, is patch3 already available?

Hmm, its attached now. Sorry about that. It seems it did not attach the first time. Looks like if a description is not provided the file does not attach... or something.

nvaccessAuto · 2013-06-14T06:22:01Z

Comment 13 by nishimotz on 2013-06-14 06:22
Tested patch3 with Windows 7 sp1 (32bit) Japanese and Windows XP sp3 Japanese.

No garbage message is observed with ATOK 2013 and Google Japanese Input for both operating systems.

Thank you.

nvaccessAuto · 2013-06-14T06:25:09Z

Comment 14 by mdcurran on 2013-06-14 06:25
But I assume that testing with real candidate lists still worked okay? (i.e. nothing broke?)

nvaccessAuto · 2013-06-14T06:47:04Z

Comment 15 by nishimotz on 2013-06-14 06:47
Candidate lists of Japanese input, obtained by IMM32 API, are not broken with patch3.

It should be mentioned that we also worked for NVDAJP (2013.1jp) around #2730 and other issues regarding Japanese input. I will file them later.

nvaccessAuto · 2013-06-14T07:06:45Z

Comment 16 by Michael Curran <mick@... on 2013-06-14 07:06
In [4bac593]:

No longer announce garbage on the desktop when the Google Japanese or Atok IME input methods are in use. Fixes #3234.

Changes:
State: closed

nvaccessAuto added bug Asian character input labels Nov 10, 2015

nvaccessAuto assigned michaelDCurran Nov 10, 2015

nvaccessAuto added this to the 2013.2 milestone Nov 10, 2015

nvaccessAuto closed this as completed Nov 10, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory allocation issue in ime.cpp #3234

Memory allocation issue in ime.cpp #3234

nvaccessAuto commented May 14, 2013

nvaccessAuto commented May 14, 2013

nvaccessAuto commented May 27, 2013

nvaccessAuto commented May 27, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented Jun 11, 2013

nvaccessAuto commented Jun 11, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 14, 2013

nvaccessAuto commented Jun 14, 2013

nvaccessAuto commented Jun 14, 2013

nvaccessAuto commented Jun 14, 2013

Memory allocation issue in ime.cpp #3234

Memory allocation issue in ime.cpp #3234

Comments

nvaccessAuto commented May 14, 2013

nvaccessAuto commented May 14, 2013

nvaccessAuto commented May 27, 2013

nvaccessAuto commented May 27, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented May 28, 2013

nvaccessAuto commented Jun 11, 2013

nvaccessAuto commented Jun 11, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 13, 2013

nvaccessAuto commented Jun 14, 2013

nvaccessAuto commented Jun 14, 2013

nvaccessAuto commented Jun 14, 2013

nvaccessAuto commented Jun 14, 2013