Re: Another pack4 issue (fwd)

[Francois Gouget]

On Thu, 22 Nov 2001, Dmitry Timoshkov wrote:
> "Francois Gouget"  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         [e-mail]        http://fgouget.free.fr/
   Nouvelle version : les anciens bogues ont été remplacés par de nouveaux.

Another pack4 issue (fwd)

[Dmitry Timoshkov]

"Francois Gouget"  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.

Another pack4 issue (fwd)

[Ulrich Weigand]

> 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

Another pack4 issue (fwd)

[Dmitry Timoshkov]

"Ulrich Weigand"  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.

Another pack4 issue (fwd)

[Francois Gouget]

On Fri, 23 Nov 2001, Dmitry Timoshkov wrote:
> "Francois Gouget"  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         [e-mail]        http://fgouget.free.fr/
May your Tongue stick to the Roof of your Mouth with the Force of a Thousand Caramels.