[WIN32SS][LPK] Add BiDi support to menus and window captions #738

peterooch · 2018-08-10T11:26:45Z

Purpose

Added callback function that redirects calls to GreExtTextOutW that didn't went through lpk BiDi processing, calls that are from the kernel.

Completely solves JIRA issue CORE-6910

Compiles and runs, until a menu is loaded...

Issue linger with last letter being corrupted

Now everything is being drawn properly

katahiromz · 2018-08-10T11:58:10Z

win32ss/gdi/ntgdi/freetype.c

+    /* Draw via lpk */
+    if (!(fuOptions & (ETO_IGNORELANGUAGE | ETO_GLYPH_INDEX)))
+    {
+        if(DrawUsingLPK(hDC, XStart, YStart, fuOptions, lprc, String, Count, &bLPKResult))


add space between 'if' and '('.

katahiromz · 2018-08-10T11:58:57Z

win32ss/gdi/ntgdi/freetype.c

+
+    Argument = IntCbAllocateMemory(ArgumentLength);
+
+    if(!Argument)


add space between 'if' and '('.

katahiromz · 2018-08-10T12:00:08Z

win32ss/gdi/ntgdi/freetype.c

+    /* copy rect */
+    if (lprc)
+    {
+        Argument->rect.left   = lprc->left;


maybe Argument->rect = *(LPRECT)lprc;

katahiromz · 2018-08-10T12:02:05Z

win32ss/gdi/ntgdi/freetype.c

+
+    if (!NT_SUCCESS(Status))
+    {
+        *bResult  = FALSE;


extra space.

katahiromz · 2018-08-10T12:16:17Z

win32ss/gdi/ntgdi/freetype.c

+    {
+        _SEH2_TRY
+        {
+            ProbeForRead(ResultPointer, sizeof(HMODULE), 1);


From where did HMODULE come?

This code is based on what is in co_IntClientLoadLibrary, which also a callback that uses string parameters. but will check if can be changed to BOOL

katahiromz · 2018-08-10T12:17:10Z

win32ss/gdi/ntgdi/freetype.c

+        _SEH2_TRY
+        {
+            ProbeForRead(ResultPointer, sizeof(HMODULE), 1);
+            *bResult = *(BOOL*)ResultPointer;


use LPBOOL.

katahiromz · 2018-08-10T12:19:38Z

win32ss/user/user32/misc/dllmain.c

+
+    Argument = (PLPK_CALLBACK_ARGUMENTS)Arguments;
+
+    Argument->lpString = (PWCHAR)((ULONG_PTR)Argument->lpString + (ULONG_PTR)Argument);


mysterious type casting.
what do you wanna do here?

See co_IntClientLoadLibrary and User32CallClientLoadLibraryFromKernel

katahiromz · 2018-08-10T12:20:31Z

win32ss/user/user32/misc/dllmain.c

+                          Argument->count,
+                          NULL);
+
+    return ZwCallbackReturn(&bResult, sizeof(HMODULE), STATUS_SUCCESS);


HMODULE ditto.

[WIN32SS] Changed some castings to make more sense, simplified the rect copy. [GDI32] Added code in case of lpk.dll loading failed.[LPK] Fixed the propsheet bug in lpk.dll

Keep freetype.c clear from non font related code, moved function to rtl\text, now only will activated upon window captions and menus.

peterooch · 2018-08-10T15:53:34Z

HBelusca · 2018-08-10T20:01:20Z

"Grey +" ? What's that?

peterooch · 2018-08-10T20:14:37Z

"Grey +" ? What's that?

A 7-zip menu thing...

HBelusca · 2018-08-10T20:45:38Z

Ah ok, so that's what they write instead of "Ctrl-+" etc...

Fixes crash while installing Notepad++

HBelusca · 2018-08-13T18:26:42Z

win32ss/user/rtl/text.c

+
+    return bResult;
+
+    fallback:


Please remove the indentation.

HBelusca · 2018-08-13T18:26:53Z

win32ss/user/rtl/text.c

+    return bResult;
+
+    fallback:
+        return GreExtTextOutW(hdc, x, y, flags, lprc, lpString, count, NULL, 0);


Reduce the indentation to 4 spaces.

HBelusca · 2018-08-13T18:27:25Z

win32ss/user/user32/misc/dllmain.c

+
+    Argument->lpString = (LPWSTR)((ULONG_PTR)Argument->lpString + (ULONG_PTR)Argument);
+
+    bResult = ExtTextOutW(Argument->hdc,


In the future this certainly will have to be generalized.

What do you mean by "generalization"?

Err forget what I've just said. I indeed see in the user32 symbols that there is indeed a special callback for ExtTextOutW (ClientExtTextOutW).
https://www.reactos.org/wiki/Techwiki:Win32k/apfnDispatch

ThFabba · 2018-08-15T06:52:15Z

win32ss/user/rtl/text.c

+    }
+    else
+    {
+        Argument->bRect = FALSE;


You're leaving rect uninitialized here, which leaks kernel memory contents to user mode.

ThFabba · 2018-08-15T06:55:56Z

win32ss/user/rtl/text.c

+    RtlStringCchCopyNW(Argument->lpString, count + 1, lpString, count);
+
+    pStringBuffer -= (ULONG_PTR)Argument;
+    Argument->lpString = (LPWSTR)(pStringBuffer);


This seems a little cumbersome. Why not set lpString to the correct value in the first place, and use pStringBuffer or some other value as the copy destination, e.g.

Argument->lpString = (LPWSTR)pStringBuffer; pStringBuffer += (ULONG_PTR)Argument; RtlStringCchCopyNW(pStringBuffer, ...);

yagoulas · 2018-08-15T08:48:17Z

Adding an extra callback is the way to go but morr changes are needed in the kernel mode part because doing a callback can have unpreidctable results. The thread coukd die in user mode and never return to free resources or the window pwnd could be destroyed while in user mode and we could end up with a stale pwnd pointrr this way.

ThFabba · 2018-08-15T09:06:24Z

Hmm I don't see any such issues looking at the code. Are you able to point to specific instances?
The memory allocation uses IntCbAllocateMemory which gets added to the thread list, so is safe for callouts, and there's no PWND to be seen, only an HDC, which should be safe as well as far as I understand.
Thanks!

Zero rect struct member if not to be used. Simplify pointer arithmetic (Thanks for the suggestion!)

learn-more · 2018-08-16T10:04:32Z

win32ss/user/rtl/text.c

+    PVOID ResultPointer;
+    ULONG ResultLength;
+    ULONG ArgumentLength;
+    ULONG_PTR pStringBuffer = 0;


This does not need to be zero-initialized.

learn-more · 2018-08-16T10:26:58Z

win32ss/user/rtl/text.c

+       mimicks code from co_IntClientLoadLibrary */
+    Argument->lpString = (LPWSTR)pStringBuffer;
+    pStringBuffer += (ULONG_PTR)Argument;
+    RtlStringCchCopyNW((LPWSTR)pStringBuffer, count + 1, lpString, count);


You should probably check the return from this.

Added documentation to explain the use of the callback function. Address comments.

peterooch · 2018-08-16T20:17:42Z

Any opinions on should I include the changes that are mentioned in:
https://www.reactos.org/forum/viewtopic.php?f=9&t=17378
or should I keep those for a later PR?

HBelusca · 2018-08-16T20:39:12Z

Hi! I think that the changes mentioned in the forum should be done in a separate PR.

peterooch added 5 commits August 5, 2018 14:47

Attempt to make kernel->user lpk call

19218cc

Compiles and runs, until a menu is loaded...

Kernel LPK callback is now functional

aeacc29

Issue linger with last letter being corrupted

Add +1 to string buffer

022856a

Now everything is being drawn properly

Fix whitespace

d8244bf

Put DPRINT1 and move code call

2ebfc8e

katahiromz reviewed Aug 10, 2018

View reviewed changes

peterooch added 3 commits August 10, 2018 15:53

[WIN32SS][LPK] Fixes

f0c2f93

[WIN32SS] Changed some castings to make more sense, simplified the rect copy. [GDI32] Added code in case of lpk.dll loading failed.[LPK] Fixed the propsheet bug in lpk.dll

PWCHAR -> LPWSTR

8564744

[WIN32SS] Move callback code to ntuser

9816642

Keep freetype.c clear from non font related code, moved function to rtl\text, now only will activated upon window captions and menus.

peterooch changed the title ~~[WIN32SS][LPK][WIP] Add BiDi support to menus and window captions~~ [WIN32SS][LPK] Add BiDi support to menus and window captions Aug 10, 2018

Copy N amount of characters

501ab44

Fixes crash while installing Notepad++

HBelusca reviewed Aug 13, 2018

View reviewed changes

Ident + delete whitespaces

0d55499

ThFabba reviewed Aug 15, 2018

View reviewed changes

[WIN32SS] Changes

e251960

Zero rect struct member if not to be used. Simplify pointer arithmetic (Thanks for the suggestion!)

learn-more reviewed Aug 16, 2018

View reviewed changes

[WIN32SS] Changes

51e338a

Added documentation to explain the use of the callback function. Address comments.

learn-more approved these changes Aug 17, 2018

View reviewed changes

HBelusca merged commit e7d2bbe into reactos:master Aug 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIN32SS][LPK] Add BiDi support to menus and window captions #738

[WIN32SS][LPK] Add BiDi support to menus and window captions #738

peterooch commented Aug 10, 2018 •

edited

katahiromz Aug 10, 2018

katahiromz Aug 10, 2018

katahiromz Aug 10, 2018 •

edited

peterooch Aug 10, 2018

katahiromz Aug 10, 2018

katahiromz Aug 10, 2018

peterooch Aug 10, 2018

katahiromz Aug 10, 2018

katahiromz Aug 10, 2018

peterooch Aug 10, 2018

katahiromz Aug 10, 2018

peterooch commented Aug 10, 2018

HBelusca commented Aug 10, 2018

peterooch commented Aug 10, 2018 •

edited

HBelusca commented Aug 10, 2018

HBelusca Aug 13, 2018

HBelusca Aug 13, 2018

HBelusca Aug 13, 2018

peterooch Aug 13, 2018

HBelusca Aug 13, 2018

ThFabba Aug 15, 2018

ThFabba Aug 15, 2018

yagoulas commented Aug 15, 2018

ThFabba commented Aug 15, 2018

learn-more Aug 16, 2018

learn-more Aug 16, 2018

peterooch commented Aug 16, 2018

HBelusca commented Aug 16, 2018


		Argument = IntCbAllocateMemory(ArgumentLength);

		if(!Argument)


		Argument = (PLPK_CALLBACK_ARGUMENTS)Arguments;

		Argument->lpString = (PWCHAR)((ULONG_PTR)Argument->lpString + (ULONG_PTR)Argument);


		Argument->lpString = (LPWSTR)((ULONG_PTR)Argument->lpString + (ULONG_PTR)Argument);

		bResult = ExtTextOutW(Argument->hdc,

[WIN32SS][LPK] Add BiDi support to menus and window captions #738

[WIN32SS][LPK] Add BiDi support to menus and window captions #738

Conversation

peterooch commented Aug 10, 2018 • edited

Purpose

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katahiromz Aug 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterooch commented Aug 10, 2018

HBelusca commented Aug 10, 2018

peterooch commented Aug 10, 2018 • edited

HBelusca commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yagoulas commented Aug 15, 2018

ThFabba commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterooch commented Aug 16, 2018

HBelusca commented Aug 16, 2018

peterooch commented Aug 10, 2018 •

edited

katahiromz Aug 10, 2018 •

edited

peterooch commented Aug 10, 2018 •

edited