Buffer size can be wrong in wxTextCtrl::StreamOut()

[wxtrac]

Issue migrated from trac ticket # 13233

component: wxMSW | priority: normal

2011-05-19 23:25:46: asbaii created the issue

---

Platform: msw, none unicode version. Version: 2.8.12

Symptom: When working on the MBCS (i.e. none unicode) MSW version, wxTextCtrl::StreamOut may return wrong value if some MBCS (e.g.: GBK for chinese or Shift-JIS for japanese) characters were in the text control.

Reason: In a MBCS release, GetWindowTextLength is mapping to GetWindowTextLengthA and this API will return the number of MBCS characters, NOT the number of WIDE characters. But wxTextCtrl::StreamOut using it as a WIDE character count even if it's in a MBCS release.

Solution: replace "const int len # GetWindowTextLength(GetHwnd());" to:"const int lenGetWindowTextLengthW(GetHwnd());"

More Info: MBCS (Multi-Byte Character Set) represent one complex character by using multi-byte. So, for a MBCS string, GetWindowTextLengthA's return value could be very different from GetWindowTextLengthW (because multi MBCS bytes may be convert to only one WIDE character).

[wxtrac]

2011-05-20 00:16:10: @troels-k commented

---

It is better to build your app in Unicode. Windows is a Unicode platform.

[wxtrac]

2011-05-20 00:26:57: @vadz changed status from new to confirmed

2011-05-20 00:26:57: @vadz changed title from wxTextCtrl::StreamOut - incorrect return value to Buffer size can be wrong in wxTextCtrl::StreamOut()

2011-05-20 00:26:57: @vadz commented

There is clearly a bug here but the problem is that if you really want to avoid the use of Unicode (e.g. because you wont to target Win9x without MSLU or something), then you can't use GetWindowTextLengthW(), of course. And if you use Unicode anyhow, then there is no problem. So the proposed fix isn't ideal.

OTOH I do wonder why do we use GetWindowTextLength() at all here, it could be very inefficient for large controls if we're only retrieving the selection. I think we should create a buffer of some reasonable fixed size instead (e.g. 4KB) and extend() it as needed from wxRichEditStreamOut(). I'd be glad to apply any [HowToSubmitPatches patches] implementing this.

[wxtrac]

2011-05-20 05:51:15: asbaii commented

---

1. Of course, the UNICODE version + MSLU is always the first choice for me on msw. But we have to use the MBCS version in some situations (e.g.: use wxWidgets from a old, MBCS only program).

2. Even if you don't use the Wide version of GetWindowTextLength system call (GetWindowTextLengthW) in the MBCS version, you are using other things that Win9x could not supported without MSLU, for example: MultiByteToWideChar and WideCharToMultiByte, etc. So, use GetWindowTextLengthW() explicitly in the MBCS version didn't making wxWidgets worse.

[wxtrac]

2011-05-20 16:08:44: @vadz commented

---

Are you sure that MultiByteToWideChar() and WideCharToMultiByte() are not supported under Win9x? MSDN says otherwise (even if it implies that the built-in versions of these functions are limited) and while I didn't test wx under Win9x recently, I did test them around 2.9.1 release time and they did work, or at least started up, back then when compiled in ANSI mode. OTOH I think that if we use GetWindowTextLengthW() here they wouldn't even start up any more because this function is genuinely not available and loading the EXE would fail when not using MSLU. Am I wrong about this?

[wxtrac]

2011-05-20 18:08:21: asbaii commented

---

From MSDN:

Windows 95/98/Me: MultiByteToWideChar is supported by the Microsoft Layer for Unicode. To use this version, you must add certain files to your application, as outlined in Microsoft Layer for Unicode on Windows 95/98/Me Systems.

Windows 95/98/Me: WideCharToMultiByte is supported by the Microsoft Layer for Unicode. To use this, you must add certain files to your application, as outlined in Microsoft Layer for Unicode on Windows 95/98/Me Systems.

Further more, I guess using some unicode flags, Like "SF_UNICODE" in wxTextCtrl::StreamOut may not emitted correct results. Especially while you are using MBCS characters in the text control.

[wxtrac]

2011-05-20 18:26:57: asbaii commented

---

BTW: Generally, I'd like to using thie simple skill to over come the API missing problem:

//! Load kernel32.dll
forceinline const CDllLoader*
_RetriveSysapi(void) throw(dlllExp)
{
  CFastSessionLock flk(g_fmxStaticLocalVarInit);

    static const CDllLoader s_dllSys(byST("kernel32"));
    return &s_dllSys;
}

//! If needed, locate the specified kernel32 api
template< class _APIFUNC >
forceinline void
TestAndLoadSysapi(IN OUT _APIFUNC volatile& func, IN const char* apiname)
{
    if (NULL == func)
    { CFastSessionLock flk(g_fmxStaticLocalVarInit);

        if (NULL != func) // already loaded by another thread
        {
            return;
        }

        func = (_APIFUNC)_RetriveSysapi()->GetProc(apiname);
    }
}

It is cleary g_fmxStaticLocalVarInit is a mutex object, and flk is a Locker confirm the RAII semantics. With these simple tools, you can write some codes like this:

//! Get Current Process PID, XP SP1（0x501）, 2k3 or laters
BAIY_LOCAL DWORD
Nt51sp1GetProcessId(IN HANDLE Process)
{
    typedef DWORD (WINAPI *LPFN_GETPROCESSID)(HANDLE);
    static LPFN_GETPROCESSID s_pfnApi;
    TestAndLoadSysapi(s_pfnApi, "GetProcessId");

    return (*s_pfnApi)(Process);
}

So, you can use it like this:

// ...
DWORD nPid;
if (IsNt51sp1OrLater())
{
    nPid = Nt51sp1GetProcessId();
}
else
{
    // ...
}

PS: Although the local static variable it self is not thread safe, but we can proof above codes are actually mutli-thread safe. But it is out of our topic here.

[wxtrac]

2011-05-20 18:42:51: @vadz commented

---

I'm not sure why are all the complications in the dynamic loading code needed, first of all the GUI code is always executed by the main thread and second with some care it shouldn't be a problem even if multiple threads execute it as at worse this will result in loading the same function pointer twice.

Anyhow, we already use wxDynamicLibrary in a lot of places in wxMSW and we could use it here too. But it still seems like a better idea to just avoid the use of this function here in the first place, at least in "only the selection" case.

[wxtrac]

2011-05-20 22:12:20: asbaii commented

---

1. As far as I known, wxWidgets has thread support. And in some special cases, on process chould (and need to) own mutiple GUI threads (a theard has a message queue belong to it) simultaneously.

2. The codes are only for exhibit purpose, they are listed here to showing the simple skill for overcome the API missing problem. So, if you don't need some mechanism in it, just remove and forget it.

[wxtrac]

2011-05-20 22:21:34: asbaii commented

---

“even if multiple threads execute it as at worse this will result in loading the same function pointer twice.”

No, if you are using a local static variable, like this:

void func(void)
{
    static CMyClass s_iMyObj;
    // ...
}

The compiler will generate codes like this:

void DestroyMyObj(void)
{
    `void func(void)::'s_iMyObj.~CMyClass();
}

void func(void)
{

    static int bCompilerInitFlag; 
    static int s_nMyVar;

    if (FALSE == bCompilerInitFlag)
    {
        s_iMyObj.CMyClass(&s_iMyObj); 
        atexit(&DestroyMyObj);
    }

    // ...
}

It's clearly if you don't use any synchronization mechanism, you may:

1. Construct a obj mutiple times.

2. Distruct a obj mutiple times.

3. Access a partial inited obj.

4. Access a obj which have not be init.

5. And may other race conditions.

[wxtrac]

2011-05-20 22:26:50: asbaii commented

---

“Anyhow, we already use wxDynamicLibrary in a lot of places in wxMSW and we could use it here too.”

Of course you should use it, as I have been sad, these codes are only a example to explain a simple skill. Although it is real code on my machine, you could read it as some pseudo-codes. :-)