Re: [ddraw] Fix bug 3487 take 2
PS: and Raphael's patch while not fixing your bug is not technically wrong as Windows checks for the surface description pointer being non-NULL :-) I was trying to reproduce his bug but i always crashed on function with a null description :( Seem you are more rapide than me Regards, Raphael pgpxAm6yWBQab.pgp Description: PGP signature
Re: [ddraw] Fix bug 3487 take 2
On Mon, Oct 10, 2005 at 01:43:04AM +0200, Peter Berg Larsen wrote:
Could you try it and tell if it fixes the problem ?
It, the patch on the bug page, does (e.i. without the locking). Could I
suggest a comment in the code.
Well, you can suggest a comment or even send a patch adding comments if you
wish :-)
And as my patch is in CVS now, you can even do a regression test with the
'full blown' patch.
Nor did I say that; just that it had nothing to with bug 3487 as the
subject said it had.
Yeah, got confused too (it really took me a while to understand that we went
out of the Lock function before crashing).
Now back to your patch. The code looks like this:
/* __tosize can be set too large by some programs */
#define DD_STRUCT_COPY_BYSIZE(to,from)
do {
DWORD __tosize = (to)-dwSize;
DWORD __fromsize = (from)-dwSize;
if ((to) == (from))
break;
if (__tosize sizeof(*(to)))
ERR(To struct's size too large);
if (__fromsize sizeof(*(from)))
ERR(From struct's size too large);
if (__fromsize __tosize)
ERR(Copying too large struct);
memcpy(to,from,__fromsize);
memset(to+__fromsize,0,sizeof(*(to))-__fromsize);
(to)-dwSize = __tosize;/*restore size*/
} while (0)
I would first merge both our patches replacing the following lines with an
'assert(to != from)':
if ((to) == (from))
break;
This macro is only used to copy application data to Wine data or the
reverse. So if both pointers are the same, it is a Wine bug as we should
NEVER send any pointer to our private data to the application (they are not
const pointers = the application can modify them = all hell break loose).
After you also give a lot of warnings but you do not do as the previous
macro. Ie if 'fromsize' is bigger than 'tosize', you just print an error,
you do not 'correct' the size which would lead to a memory overflow.
Finally, 'to+__fromsize' is wrong, it should be '((char *) to)+__fromsize'
and why use 'sizeof(*(to))' as the basis for the 'memset' size and not
'tosize' ?
Lionel
--
Lionel Ulmer - http://www.bbrox.org/
Re: [ddraw] Fix bug 3487 take 2
On Mon, 10 Oct 2005, Lionel Ulmer wrote:
Nor did I say that; just that it had nothing to with bug 3487 as the
subject said it had.
Yeah, got confused too (it really took me a while to understand that we went
out of the Lock function before crashing).
Yep.
/* __tosize can be set too large by some programs */
#define DD_STRUCT_COPY_BYSIZE(to,from)
do {
DWORD __tosize = (to)-dwSize;
DWORD __fromsize = (from)-dwSize;
if ((to) == (from))
break;
if (__tosize sizeof(*(to)))
ERR(To struct's size too large);
if (__fromsize sizeof(*(from)))
ERR(From struct's size too large);
if (__fromsize __tosize)
ERR(Copying too large struct);
memcpy(to,from,__fromsize);
memset(to+__fromsize,0,sizeof(*(to))-__fromsize);
(to)-dwSize = __tosize;/*restore size*/
} while (0)
I would first merge both our patches replacing the following lines with an
'assert(to != from)':
if ((to) == (from))
break;
I think thats execellent idea; I hadnt though of asserts, did not new
wine used them..
This macro is only used to copy application data to Wine data or the
reverse. So if both pointers are the same, it is a Wine bug as we should
NEVER send any pointer to our private data to the application (they are not
const pointers = the application can modify them = all hell break loose).
Hmm, ok. I read the comments bug 2070, as we only did the wine data to
application data. Otherwise I think bug 2070 is missing handling too large
from dwSize and could corrupt mem by memcpy.
After you also give a lot of warnings but you do not do as the previous
macro. Ie if 'fromsize' is bigger than 'tosize', you just print an error,
you do not 'correct' the size which would lead to a memory overflow.
Yes. The latter 2, if ERR, should be asserts too. The first if ERR should
be a WARN, as some applications breaks this assumption.
Finally, 'to+__fromsize' is wrong, it should be '((char *) to)+__fromsize'
yes.
and why use 'sizeof(*(to))' as the basis for the 'memset' size and not
'tosize' ?
Because tosize is not to be trusted by bug 2070, it could be to large. One
of invariants was that tosize = sizeof(*to).
I still havent got the grasp of what dwSize is for if it does not reflect
the size allocated?, nor the size of the struct current in the mem.
Peter
Re: [ddraw] Fix bug 3487 take 2
On Mon, Oct 10, 2005 at 10:18:21PM +0200, Peter Berg Larsen wrote:
I would first merge both our patches replacing the following lines with an
'assert(to != from)':
if ((to) == (from))
break;
I think thats execellent idea; I hadnt though of asserts, did not new
wine used them..
Well, as Alexandre committed my 'assert' patch, it's already in the code and
there is just a merge to be done :-)
As for asserts, they work because Wine catches the 'assert' signal and
transforms it into a Win32 exception thus starting the Wine debugger.
I still havent got the grasp of what dwSize is for if it does not reflect
the size allocated?, nor the size of the struct current in the mem.
An example of the usage of the 'dwSize' parameter:
static HRESULT WINAPI
IDirectDrawSurface3Impl_Lock(LPDIRECTDRAWSURFACE3 This, LPRECT pRect,
LPDDSURFACEDESC pDDSD, DWORD dwFlags, HANDLE h)
{
return IDirectDrawSurface7_Lock(CONVERT(This), pRect,
(LPDDSURFACEDESC2)pDDSD, dwFlags, h);
}
As the DDSURFACEDESC2 structure starts exactly like the DDSURFACEDESC2 one,
the hack here is to use 'dwSize' to magically cast one structure to the
other to be able to share code.
This is what MS had in mind when they introduced this field: have
'extendable' structure wheer each new revisions only added fields = they
can all be filled by one common function, using the 'dwSize' field to
disctriminate between versions.
Note that next time I bring my Win32 laptop home, I will try to do some
tests in real Windows to see how Windows fills the given structure pointer.
Lionel
--
Lionel Ulmer - http://www.bbrox.org/
Re: [ddraw] Fix bug 3487 take 2
On Mon, 10 Oct 2005, Lionel Ulmer wrote: I still havent got the grasp of what dwSize is for if it does not reflect the size allocated?, nor the size of the struct current in the mem. As the DDSURFACEDESC2 structure starts exactly like the DDSURFACEDESC2 one, the hack here is to use 'dwSize' to magically cast one structure to the other to be able to share code. Hmm, then it does reflect the size of the struct currently in the memory. So in theory it's: assert(to != from); DWORD __copysize = MIN((to)-dwSize,(from)-dwSize); memcpy((to),(from),__copysize); in practise, because of debug and bug 2070: assert(to != from); DWORD __tosize = MIN(sizeof(*(to), ,(to)-dwSize); DWORD __fromsize = MIN(sizeof(*(from),(from)-dwSize); DWORD __copysize = MIN(__tosize,__fromsize); memcpy((to),(from),__copysize); memset((to)+__copysize,0,__tosize-__copysize); Or am I still off? Peter
Re: [ddraw] Fix bug 3487 take 2
So here is better patch that does not open 2070 again. Changelog: Bug in copying structs if to == from as to was memset first. Well, while your patch was lying in the moderation queue, I sent what I feel is a better solution to this problem (which fixes also a severe reference counting issue). Could you try it and tell if it fixes the problem ? Lionel PS: and Raphael's patch while not fixing your bug is not technically wrong as Windows checks for the surface description pointer being non-NULL :-) -- Lionel Ulmer - http://www.bbrox.org/
Re: [ddraw] Fix bug 3487 take 2
Peter Berg Larsen wrote:
Walking backwards the bug was introduced in
http://www.winehq.org/pipermail/wine-patches/2002-November/004161.html
and altered in
http://www.winehq.org/pipermail/wine-patches/2004-March/010091.html
which states
Apps should initialize correctly the dwSize member of ddraw structures.
However some of them do not do this and Windows seems to handle that
case without crashing.
Here is a patch that prevents clearing more that the struct size.
This fixes the bug 2070.
So here is better patch that does not open 2070 again.
Changelog:
Bug in copying structs if to == from as to was memset first.
diff -u Wine-20050930/dlls/ddraw/ddraw_private.h
Wine-20050930my/dlls/ddraw/ddraw_private.h
--- Wine-20050930/dlls/ddraw/ddraw_private.h2005-07-24
18:17:29.0 +0200
+++ Wine-20050930my/dlls/ddraw/ddraw_private.h2005-10-09
04:50:59.0 +0200
@@ -41,18 +41,22 @@
(x)-dwSize = sizeof(*x);\
} while (0)
+/* __tosize can be set too large by some programs */
#define DD_STRUCT_COPY_BYSIZE(to,from)\
do {\
-DWORD __size = (to)-dwSize;\
-DWORD __copysize = __size;\
-DWORD __resetsize = __size;\
-if (__resetsize sizeof(*to))\
-__resetsize = sizeof(*to);\
-memset(to,0,__resetsize); \
-if ((from)-dwSize __size) \
-__copysize = (from)-dwSize;\
-memcpy(to,from,__copysize);\
-(to)-dwSize = __size;/*restore size*/\
+DWORD __tosize = (to)-dwSize;\
+DWORD __fromsize = (from)-dwSize;\
+if ((to) == (from))\
+break;\
+if (__tosize sizeof(*(to)))\
+ERR(To struct's size too large);\
+if (__fromsize sizeof(*(from)))\
+ERR(From struct's size too large);\
+if (__fromsize __tosize)\
+ERR(Copying too large struct);\
+memcpy(to,from,__fromsize);\
+memset(to+__fromsize,0,sizeof(*(to))-__fromsize);\
+(to)-dwSize = __tosize;/*restore size*/\
} while (0)
#define MAKE_FOURCC(a,b,c,d) ((a 0) | (b 8) | (c 16) | (d
24))
This does not work if the destination size is smaller than the struct
size or source size is greater than destination size.
Even worst, if source size is greater than struct size,
sizeof(*(to))-__fromsize is 0.
Why don't you just add the if (to = from) break; statement ?
Christian
Re: [ddraw] Fix bug 3487 take 2
On Sun, 9 Oct 2005, Lionel Ulmer wrote:
Well, while your patch was lying in the moderation queue, I sent what I
feel is a better solution to this problem (which fixes also a severe
reference counting issue).
I had more than one goal with the patch, more below.
Could you try it and tell if it fixes the problem ?
It, the patch on the bug page, does (e.i. without the locking). Could I
suggest a comment in the code.
PS: and Raphael's patch while not fixing your bug is not technically
wrong as Windows checks for the surface description pointer being
non-NULL
Nor did I say that; just that it had nothing to with bug 3487 as the
subject said it had.
On Sun, 9 Oct 2005, Christian Costa wrote:
This does not work if the destination size is smaller than the struct size or
source size is greater than destination size.
Even worst, if source size is greater than struct size,
sizeof(*(to))-__fromsize is 0.
Why don't you just add the if (to = from) break; statement ?
I wrote the patch, not because of bug 3487, but because, as I
wrote in take 1:
Pure luck to find this one. I didnt understand what was going on;
and rewrote it. I am still uncertain about invariants wrt. sizeof
and dwSize.
It is impossible to follow. Ok; in the begining it was only a memcpy, then
because the debug was gabled an memset was added. Then bug 2070 states the
the (to)-dwSize could be too high. Rather than just adding a if
statement, I decided that it needed a complete rewrite.
The invariants then is:
sizeof(*to) = to-dwSize
sizeof(*from) == from-dwSize
Why is the old code testing the sizeof(*to) to-dwsize for the memset
case only? Why first memset and then copy over it? Why use 3 variables?
The goal was the do the memcpy and then memset the rest. The second goal
was to bark if the invariants were broken. The third goal was to have the
compiler remove any ifs when debugging was turned off.
I tried play around with
if(to != from) {
}
or
do {
if (to!=from) {
...
}
} while(0)
but found the
if (to!=from) break
cleanest.
The first ERR should have been a warn though.
Peter
