Re: LockResource16 in ole32.dll
Ge van Geldorp [EMAIL PROTECTED] 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. -- Dmitry.
RE: LockResource16 in ole32.dll
From: Dmitry Timoshkov Ge van Geldorp [EMAIL PROTECTED] 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. actest.zip Description: Zip compressed data
Re: LockResource16 in ole32.dll
Ge van Geldorp [EMAIL PROTECTED] 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. -- Alexandre Julliard [EMAIL PROTECTED]
Re: LockResource16 in ole32.dll
Ge van Geldorp [EMAIL PROTECTED] 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. -- Dmitry.
Re: LockResource16 in ole32.dll
From: Dmitry Timoshkov
Casper Hornstrup [EMAIL PROTECTED] 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 - 1.48
+++ dlls/ole32/ole2.c 23 Jan 2004 14:17:06 -
@@ -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;
RE: LockResource16 in ole32.dll
From: Dmitry Timoshkov [mailto:[EMAIL PROTECTED] Ge van Geldorp [EMAIL PROTECTED] 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.
Re: LockResource16 in ole32.dll
Ge van Geldorp [EMAIL PROTECTED] 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. -- Dmitry.
RE: LockResource16 in ole32.dll
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.
Re: LockResource16 in ole32.dll
Ge van Geldorp [EMAIL PROTECTED] 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. -- Dmitry.
Re: LockResource16 in ole32.dll
Casper Hornstrup [EMAIL PROTECTED] 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). -- Dmitry.
Re: LockResource16 in ole32.dll
-Oprindelig meddelelse- Fra: Dmitry Timoshkov [mailto:[EMAIL PROTECTED] Sendt: 6. december 2003 14:00 Til: Casper Hornstrup Cc: [EMAIL PROTECTED] Emne: Re: LockResource16 in ole32.dll Casper Hornstrup [EMAIL PROTECTED] 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). Windows XP ole32.dll does not import LockResource16 (nor LockResource for that matter). Windows XP user32.dll does not import GlobalAlloc16, but it imports GlobalAlloc. I currently do not have access to a Win9X installation to check that. Casper
