On Thu, 22 Nov 2001, Dmitry Timoshkov wrote:
"Francois Gouget" <fgouget(a)free.fr> wrote:
After the recent patch sent by Dmitry Timoshkov I checked for other similar issues. AFAICS, at this time there is only one place where a pack4 is really necessary.
Well, I don't know why you are so sure. My testing revealed the following differences in structure sizes between Wine and Windows:
Well, I had just performed a search on 'pshpck4.h' and checked that against Wine's headers to see whether there would be a difference. Your method seems much more thorough. I am a bit surprised that we have so many differences. We should see tons of crashes... (maybe that's good news) [...]
What is really fun, that usage of FILETIME instead of LARGE_INTEGER leads to differences in size of structures.
That's very strange. It would be good to understand why because this should not happen. diff -u cvs/hq/wine/dlls/advapi32/registry.c wine/dlls/advapi32/registry.c --- cvs/hq/wine/dlls/advapi32/registry.c Mon Jul 23 22:19:41 2001 +++ wine/dlls/advapi32/registry.c Thu Nov 22 17:52:05 2001 @@ -287,7 +287,7 @@ DWORD len = info->NameLength / sizeof(WCHAR); DWORD cls_len = info->ClassLength / sizeof(WCHAR); - if (ft) *ft = info->LastWriteTime; + if (ft) *ft = *(FILETIME *)&info->LastWriteTime; This cast should not be necessary. Both sides are FILETIMEs. --- cvs/hq/wine/dlls/ntdll/reg.c Thu Nov 8 16:15:30 2001 +++ wine/dlls/ntdll/reg.c Thu Nov 22 17:45:36 2001 @@ -161,9 +161,9 @@ WCHAR *class_ptr = (WCHAR *)((char *)name_ptr + name_size); int class_size = server_data_size(req) - sizeof(WCHAR) - name_size; int fixed_size; - FILETIME modif; + LARGE_INTEGER modif; - RtlSecondsSince1970ToTime( req->modif, &modif ); + RtlSecondsSince1970ToTime( req->modif, (FILETIME *)&modif ); Here I don't understant why you changed modif to a LARGE_INTEGER since RtlSecondsSince1970ToTime takes a FILETIME* switch(info_class) { diff -u cvs/hq/wine/include/commctrl.h wine/include/commctrl.h --- cvs/hq/wine/include/commctrl.h Wed Aug 8 17:37:12 2001 +++ wine/include/commctrl.h Thu Nov 22 16:50:13 2001 @@ -744,6 +744,8 @@ LPARAM lParam; INT iImage; INT iOrder; + UINT type; + LPVOID pvFilter; } HDITEMA, *LPHDITEMA; I don't see these two fields on Windows. Same thing for the W version. That's with the VC 6 headers. @@ -2959,9 +2958,11 @@ typedef struct { DWORD cb; - BYTE DeviceName[32]; - BYTE DeviceString[128]; + CHAR DeviceName[32]; + CHAR DeviceString[128]; DWORD StateFlags; + CHAR DeviceID[128]; + CHAR DeviceKey[128]; } DISPLAY_DEVICEA,*PDISPLAY_DEVICEA,*LPDISPLAY_DEVICEA; Hmm, no. It is 'BYTE' on Windows. And I don't see the two extra fields either. diff -u cvs/hq/wine/include/winnt.h wine/include/winnt.h --- cvs/hq/wine/include/winnt.h Mon Aug 13 00:51:56 2001 +++ wine/include/winnt.h Thu Nov 22 18:00:57 2001 @@ -396,8 +396,6 @@ typedef a *P##a, *LP##a #endif /*STRICT*/ - -#include "pshpack1.h" /* Defines */ /* Argument 1 passed to the DllEntryProc. */ @@ -576,6 +574,8 @@ DWORD Cr0NpxState; } FLOATING_SAVE_AREA, *PFLOATING_SAVE_AREA; +#define MAXIMUM_SUPPORTED_EXTENSION 512 + typedef struct _CONTEXT86 { DWORD ContextFlags; @@ -612,6 +612,8 @@ DWORD EFlags; DWORD Esp; DWORD SegSs; + + BYTE ExtendedRegisters[MAXIMUM_SUPPORTED_EXTENSION]; } CONTEXT86; I don't have this field in CONTEXT86, but it is there in CONTEXT. It seems that it's our definition of CONTEXT which is wrong: typedef CONTEXT86 CONTEXT; @@ -2923,11 +2923,15 @@ WORD Reserved; } IMAGE_BOUND_FORWARDER_REF, *PIMAGE_BOUND_FORWARDER_REF; +#include "pshpack2.h" + typedef struct _IMAGE_BASE_RELOCATION { DWORD VirtualAddress; DWORD SizeOfBlock; +#ifdef __WINE__ WORD TypeOffset[1]; +#endif } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION; Do we handle this correctly in the code? I.e. is there APIs where the application gives us a pointer to a struct without the field and we write to it? I wonder how this is supposed to work. +#include "pshpack4.h" typedef struct _LUID_AND_ATTRIBUTES { LUID Luid; DWORD Attributes; } LUID_AND_ATTRIBUTES; +#include "poppack.h" Is this really necessary? LUID is 8 bytes so the DWORD should be aligned just fine with or without the pack4... -- Francois Gouget fgouget(a)free.fr http://fgouget.free.fr/ Nouvelle version : les anciens bogues ont été remplacés par de nouveaux.
"Francois Gouget" <fgouget(a)free.fr> wrote:
[...]
What is really fun, that usage of FILETIME instead of LARGE_INTEGER leads to differences in size of structures.
That's very strange. It would be good to understand why because this should not happen.
Any ideas, why does it happen? Is it due to the LONGLONG usage?
- if (ft) *ft = info->LastWriteTime; + if (ft) *ft = *(FILETIME *)&info->LastWriteTime;
This cast should not be necessary. Both sides are FILETIMEs.
It's because according to ntddk.h from NT4 DDK info->LastWriteTime is LARGE_INTEGER. Please look at my patch a bit carefully.
- FILETIME modif; + LARGE_INTEGER modif;
- RtlSecondsSince1970ToTime( req->modif, &modif ); + RtlSecondsSince1970ToTime( req->modif, (FILETIME *)&modif );
Here I don't understant why you changed modif to a LARGE_INTEGER since RtlSecondsSince1970ToTime takes a FILETIME*
Same comment as above.
+ UINT type; + LPVOID pvFilter; } HDITEMA, *LPHDITEMA;
I don't see these two fields on Windows. Same thing for the W version. That's with the VC 6 headers.
It's in July 2000 PlatformSDK as well as in MSDN online.
typedef struct { DWORD cb; - BYTE DeviceName[32]; - BYTE DeviceString[128]; + CHAR DeviceName[32]; + CHAR DeviceString[128]; DWORD StateFlags; + CHAR DeviceID[128]; + CHAR DeviceKey[128]; } DISPLAY_DEVICEA,*PDISPLAY_DEVICEA,*LPDISPLAY_DEVICEA;
Hmm, no. It is 'BYTE' on Windows. And I don't see the two extra fields either.
Same comment as above.
+ + BYTE ExtendedRegisters[MAXIMUM_SUPPORTED_EXTENSION]; } CONTEXT86;
I don't have this field in CONTEXT86, but it is there in CONTEXT. It seems that it's our definition of CONTEXT which is wrong: typedef CONTEXT86 CONTEXT;
Same comment as above.
typedef struct _IMAGE_BASE_RELOCATION { DWORD VirtualAddress; DWORD SizeOfBlock; +#ifdef __WINE__ WORD TypeOffset[1]; +#endif } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
Do we handle this correctly in the code? I.e. is there APIs where the application gives us a pointer to a struct without the field and we write to it? I wonder how this is supposed to work.
In winnt.h from the PlatformSDK it is just commented, but as you could assume Wine loader uses this field to relocate images.
+#include "pshpack4.h" typedef struct _LUID_AND_ATTRIBUTES { LUID Luid; DWORD Attributes; } LUID_AND_ATTRIBUTES; +#include "poppack.h"
Is this really necessary? LUID is 8 bytes so the DWORD should be aligned just fine with or without the pack4...
Without pshpack4.h sizeof(LUID_AND_ATTRIBUTES)=16 instead of 12. Don't ask me why. Both NT4 DDK (ntddk.h) and PlatformSDK (winnt.h) headers surround LUID_AND_ATTRIBUTES definition by "pshpack4.h"/"poppack.h" pair. Thanks for your comments. -- Dmitry.
Without pshpack4.h sizeof(LUID_AND_ATTRIBUTES)=16 instead of 12. Don't ask me why.
Because the size of a structure is always padded to the next multiple of its alignment (so that in an array of structures, every element is properly aligned iff the array is). By changing the alignment, you therefore also sometimes change the size of a structure ... Bye, Ulrich -- Dr. Ulrich Weigand weigand(a)informatik.uni-erlangen.de
"Ulrich Weigand" <weigand(a)immd1.informatik.uni-erlangen.de> wrote:
Without pshpack4.h sizeof(LUID_AND_ATTRIBUTES)=16 instead of 12. Don't ask me why.
Because the size of a structure is always padded to the next multiple of its alignment (so that in an array of structures, every element is properly aligned iff the array is). By changing the alignment, you therefore also sometimes change the size of a structure ...
Ulrich, thanks for explanations. I believe that's the case for the FILETIME vs. LARGE_INTEGER issue too. -- Dmitry.
On Fri, 23 Nov 2001, Dmitry Timoshkov wrote:
"Francois Gouget" <fgouget(a)free.fr> wrote: [...] It's because according to ntddk.h from NT4 DDK info->LastWriteTime is LARGE_INTEGER. Please look at my patch a bit carefully.
Sorry, I missed that part (I was in a hurry at the time, it's always bad).
+ UINT type; + LPVOID pvFilter; } HDITEMA, *LPHDITEMA;
I don't see these two fields on Windows. Same thing for the W version. That's with the VC 6 headers.
It's in July 2000 PlatformSDK as well as in MSDN online.
It means that the Visual C++ 6.0 SP3 headers are too old and cannot be trusted anymore. Still, this kind of change (adding new fields) worries me. Of course we have to add these fields but it means we must be very careful not to access the extra fields unless we know they are present. For instance in HEADER_GetItemA we return information in an HDITEMA structure allocated by the application. Currently it is correct, but let's just imagine that someone adds a 'memset(phdi,0,sizeof(*phdi))' (as is quite commonly done). If the application has been compiled with an old version of the headers then we will have a buffer overflow -> crash. So the handling of such structures requires special care. It may be a good idea to record somewhere that the size of the structure has changed. Maybe in the header as follows: ... /* Fields added for IE 5 */ UINT type; LPVOID pvFilter; } HDITEMA, *LPHDITEMA; or even full-blown '#if _WIN32_IE >= xxx' though we don't do this sort of thing usually. Well, it's a pretty good work in any case and is quite likely to fix a lot of mysterious crashes. -- Francois Gouget fgouget(a)free.fr http://fgouget.free.fr/ May your Tongue stick to the Roof of your Mouth with the Force of a Thousand Caramels.
participants (3)
-
Dmitry Timoshkov -
Francois Gouget -
Ulrich Weigand