From: Dmitry Timoshkov
"Casper Hornstrup" chorns@users.sourceforge.net wrote:
I would like to have the call to the Win16 API LockResource16 removed from ole32.dll. I guess there is a reason for it being LockResource16 and not LockResource. What is the reason?
Because LoadAcceleratorsA/W returns a value allocated by GlobalAlloc16.
How can the call be removed?
Only if you or someone else can prove that under real windows LoadAcceleratorsA/W behaves differently (taking into account Win9x weirdness).
Would the attached patch be an acceptable solution? Basically it does a GetProcAddress on LockResource16 and uses it if found (Wine case). If it's not found, it uses LockResource().
Ge van Geldorp.
Index: dlls/ole32/ole2.c =================================================================== RCS file: /home/wine/wine/dlls/ole32/ole2.c,v retrieving revision 1.48 diff -u -r1.48 ole2.c --- dlls/ole32/ole2.c 8 Dec 2003 22:46:08 -0000 1.48 +++ dlls/ole32/ole2.c 23 Jan 2004 14:17:06 -0000 @@ -1422,9 +1422,34 @@ /* YES, Accel16! */ LPACCEL16 lpAccelTbl; int i; + HMODULE Kernel32; + LPVOID (WINAPI *LockResource16Ptr)(HGLOBAL16 ResData);
if(!lpMsg) return FALSE; - if (!hAccel || !(lpAccelTbl = (LPACCEL16)LockResource16(HACCEL_16(hAccel)))) + if (!hAccel) + { + WARN_(accel)("NULL accel handle\n"); + return FALSE; + } + Kernel32 = GetModuleHandleA("kernel32.dll"); + if (NULL != Kernel32) + { + LockResource16Ptr = (LPVOID (WINAPI *)(HGLOBAL16)) + GetProcAddress(Kernel32, "LockResource16"); + } + else + { + LockResource16Ptr = NULL; + } + if (NULL != LockResource16Ptr) + { + lpAccelTbl = (LPACCEL16) LockResource16Ptr(HACCEL_16(hAccel)); + } + else + { + lpAccelTbl = (LPACCEL16) LockResource(hAccel); + } + if (NULL == lpAccelTbl) { WARN_(accel)("invalid accel handle=%p\n", hAccel); return FALSE;
"Ge van Geldorp" ge@gse.nl wrote:
Would the attached patch be an acceptable solution? Basically it does a GetProcAddress on LockResource16 and uses it if found (Wine case). If it's not found, it uses LockResource().
That will not work. 32-bit LockResource can't be used on a memory block allocated by GlobalAlloc16, as I explained before.
From: Dmitry Timoshkov [mailto:dmitry@baikal.ru]
"Ge van Geldorp" ge@gse.nl wrote:
Would the attached patch be an acceptable solution? Basically it does a GetProcAddress on LockResource16 and uses it if found (Wine case). If it's not found, it uses LockResource().
That will not work. 32-bit LockResource can't be used on a memory block allocated by GlobalAlloc16, as I explained before.
No, not in Wine. But Wine will still use LockResource16, so there's no problem. I can assure you that the memory block won't be allocated by GlobalAlloc16 in ReactOS, simply because there is no GlobalAlloc16 in ReactOS. Since there is also no LockResource16 in ReactOS it would take the other code path. I'm just trying to find a solution which allows umodified source code to be used by both Wine and ReactOS.
Ge van Geldorp.
"Ge van Geldorp" ge@gse.nl wrote:
That will not work. 32-bit LockResource can't be used on a memory block allocated by GlobalAlloc16, as I explained before.
No, not in Wine. But Wine will still use LockResource16, so there's no problem. I can assure you that the memory block won't be allocated by GlobalAlloc16 in ReactOS, simply because there is no GlobalAlloc16 in ReactOS. Since there is also no LockResource16 in ReactOS it would take the other code path. I'm just trying to find a solution which allows umodified source code to be used by both Wine and ReactOS.
Then it's better to fix LoadAcceleratorsA/W to use a proper allocator and use the same aproach in both Wine and ReactOS.
But in that case you have to write a test case which works at least on NT/2000 and Wine.
From: Dmitry Timoshkov
Then it's better to fix LoadAcceleratorsA/W to use a proper allocator and use the same aproach in both Wine and ReactOS.
But in that case you have to write a test case which works at least on NT/2000 and Wine.
Another idea just popped up: the basic problem we're having is translating the handle passed in to a table containing the accelerator entries. How about using CopyAcceleratorTableA/W to do that? This function is designed to do that job and is located in user32. That dll is not shared between Wine and ReactOS anyway, so we can mess around in it anyway we like without disturbing Wine.
Ge van Geldorp.
"Ge van Geldorp" ge@gse.nl wrote:
Another idea just popped up: the basic problem we're having is translating the handle passed in to a table containing the accelerator entries. How about using CopyAcceleratorTableA/W to do that? This function is designed to do that job and is located in user32. That dll is not shared between Wine and ReactOS anyway, so we can mess around in it anyway we like without disturbing Wine.
IMHO it would be much better to fix LoadAcceleratorsA/W and avoid to use LockResource16 at all in all callers in Wine. Yes, it's slightly more work, but that's a usual case with all Wine bugs.
From: Dmitry Timoshkov
"Ge van Geldorp" ge@gse.nl wrote:
Another idea just popped up: the basic problem we're having is translating the handle passed in to a table containing the accelerator entries. How about using CopyAcceleratorTableA/W to do that? This function is designed to do that job and is located in user32. That dll is not shared between Wine and ReactOS anyway, so we can mess around in it anyway we like without disturbing Wine.
IMHO it would be much better to fix LoadAcceleratorsA/W and avoid to use LockResource16 at all in all callers in Wine. Yes, it's slightly more work, but that's a usual case with all Wine bugs.
Just to be sure: if I understand you correctly you suggest doing the LockResource16 in LoadAcceleratorsA/W, then GlobalAlloc a new block of memory, copy the table to the new block and return the global memory handle as the HACCEL. Then in IsAccelerator use GlobalLock to get at the contents. Please correct me if I'm wrong.
Doesn't that still violate DLL separation? Ole32.dll "knows" that a HACCEL is actually a HGLOBAL and even "knows" how the accelerator table is stored internally. IMHO that stuff should only be known to user32. Put another way: if you run the proposed ole32 IsAccelerator code on MS Windows it would fail. Using CopyAcceleratorTable (a user32 function) fixes this, knowledge of the internal structure is limited to user32. Incidentally, MS ole32 (NT4/2K/XP) imports CopyAcceleratorTableW.
Somewhat to my surprise, it turns out that HACCEL handles can be shared between processes. I've attached a simple test program which uses an accelerator table with one entry, for the Alt-T key. Pressing Alt-T while the programs window has input focus displays a message box. If you run this program for the first time, the accelerator table is loaded from a resource. It will display the handle in hex. While the program is still running, start it a second time, with the 8 hex-digit handle as command line argument. The second copy will then use that number as the HACCEL to pass to IsAccelerator, without loading a new accelerator table. When running on NT4/2K/XP the second instance will also flag Alt-T as an accelerator. If you close the first instance (destroying the accelerator table it loaded) the second instance will no longer flag Alt-T as an accelerator.
I guess this means that at some point the accelerator implementation should be moved to the server. When that happens HACCEL can no longer be a HGLOBAL and as a result the code making that assumption in ole32 will cease working. Why not fix the ole32 code right away to use CopyAcceleratorTable? Obviously CopyAcceleratorTable itself will have to be changed when the implementation is moved, but that needs to be done anyhow.
So, my vote still is for the CopyAcceleratorTable approach.
Ge van Geldorp.
"Ge van Geldorp" ge@gse.nl writes:
Just to be sure: if I understand you correctly you suggest doing the LockResource16 in LoadAcceleratorsA/W, then GlobalAlloc a new block of memory, copy the table to the new block and return the global memory handle as the HACCEL. Then in IsAccelerator use GlobalLock to get at the contents. Please correct me if I'm wrong.
Doesn't that still violate DLL separation? Ole32.dll "knows" that a HACCEL is actually a HGLOBAL and even "knows" how the accelerator table is stored internally. IMHO that stuff should only be known to user32. Put another way: if you run the proposed ole32 IsAccelerator code on MS Windows it would fail. Using CopyAcceleratorTable (a user32 function) fixes this, knowledge of the internal structure is limited to user32.
Yes, that sounds like the right way. Dmitry is right that the accelerator support needs to be fixed to not allocate 16-bit memory, but as far as ole32 is concerned it shouldn't matter, because ole32 has no business knowing what's behind an accelerator handle.
"Ge van Geldorp" ge@gse.nl wrote:
Just to be sure: if I understand you correctly you suggest doing the LockResource16 in LoadAcceleratorsA/W, then GlobalAlloc a new block of memory, copy the table to the new block and return the global memory handle as the HACCEL. Then in IsAccelerator use GlobalLock to get at the contents. Please correct me if I'm wrong.
I was suggesting to avoid using GlobalAlloc16 in LoadAcceleratorsW at all, and use 32-bit GlobalAlloc instead.
Alexandre answered another part of your message. And yes, you are right that support for sharing accelerator handles between processes will require complete rewrite of that code.
-
Alexandre Julliard -
Dmitry Timoshkov -
Ge van Geldorp