Menu
▾
▴
[Mingw-notify] [ mingw-Bugs-1674783 ] Problem with Vista 64 bit (
|
From: SourceForge.net <no...@so...> - 2007-07-12 18:12:30
|
Bugs item #1674783, was opened at 2007-03-06 10:01 Message generated for change (Comment added) made by cstrauss You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=102435&aid=1674783&group_id=2435 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: msys Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: jdrasch (jdrasch) Assigned to: Cesar Strauss (cstrauss) Summary: Problem with Vista 64 bit ( Initial Comment: Hi, Msys didn't work on Vista 64 bit in the 32 bit emulation environment (<windir>/syswow64/cmd.exe) After a research I found the following in the nabble forums. http://www.nabble.com/MSYS-and-Vista-64-tf3292881.html#a9183478 The cygwin folks have figured out that Vista's WOW64 works differently than XP's WOW64. This patch should fix it. #### ChangeSet #### 2007-01-22 22:12:41-08:00, ob@... Work around what appears to be a bug in Vista 64-bits. The problem is related to how fork passes information to the child. Basically the child_info struct * is put in the lpReserved2 field of the STARTUPINFO struct that gets passed to CreateProcess(). In all versions of Windows, the size of the data to be passed is in the cbReserved2 field of the same structure, excep on Vista 64-bits. It seems that WOW64 (the 32-bit emmulation layer of 64-bit Vista) expects the size of the data passed in lpReserved2 to be the first word in that same pointer, only it expects the size in "elements" of a BYTE[] and HANDLE[] arrays (the MS C runtime has this convention). We work around it by filling in the size as (sizeof struct)/5 and zero padding the end of the child_info_* structs. ==== rt/src/winsup/cygwin/child_info.h ==== 2007-01-22 22:12:39-08:00, ob@... +4 -1 pad the end of the child_info_* structs and change the zero at the beginning for a placeholder for the size. --- 1.3/rt/src/winsup/cygwin/child_info.h 2001-10-15 15:22:32 -07:00 +++ 1.4/rt/src/winsup/cygwin/child_info.h 2007-01-22 22:12:39 -08:00 @@ -29,7 +29,7 @@ enum class child_info { public: - DWORD zero[1]; // must be zeroed + DWORD msv_cnt; // size of struct on vista64, zero on others DWORD cb; // size of this record DWORD type; // type of record int cygpid; // cygwin pid of child process @@ -53,6 +53,7 @@ public: jmp_buf jmp; // where child will jump to void *stacktop; // location of top of parent stack void *stackbottom; // location of bottom of parent stack + char filler[4]; }; class fhandler_base; @@ -67,6 +68,7 @@ public: int envc; char **envp; HANDLE myself_pinfo; + char filler[4]; }; class child_info_spawn: public child_info @@ -74,6 +76,7 @@ class child_info_spawn: public child_inf public: cygheap_exec_info *moreinfo; HANDLE hexec_proc; + char filler[4]; child_info_spawn (): moreinfo (NULL) {} ~child_info_spawn () ==== rt/src/winsup/cygwin/dcrt0.cc ==== 2007-01-22 22:12:40-08:00, ob@... +1 -3 the child_info_* structs no longer have a zero at the beginning. --- 1.10/rt/src/winsup/cygwin/dcrt0.cc 2004-03-15 03:51:36 -08:00 +++ 1.11/rt/src/winsup/cygwin/dcrt0.cc 2007-01-22 22:12:40 -08:00 @@ -903,7 +903,6 @@ _dll_crt0 () if (GetEnvironmentVariable ("CYGWIN_TESTING", envbuf, sizeof (envbuf) - 1)) _cygwin_testing = 1; - char zeros[sizeof (fork_info->zero)] = {0}; #ifdef DEBUGGING strace.microseconds (); #endif @@ -924,8 +923,7 @@ _dll_crt0 () &hMainThread, 0, FALSE, DUPLICATE_SAME_ACCESS); GetStartupInfo (&si); - if (si.cbReserved2 >= EXEC_MAGIC_SIZE && - memcmp (fork_info->zero, zeros, sizeof (zeros)) == 0) + if (si.cbReserved2 >= EXEC_MAGIC_SIZE) { switch (fork_info->type) { ==== rt/src/winsup/cygwin/fork.cc ==== 2007-01-22 22:12:40-08:00, ob@... +2 -0 fill in the size of child_info_fork --- 1.4/rt/src/winsup/cygwin/fork.cc 2002-10-31 06:32:21 -08:00 +++ 1.5/rt/src/winsup/cygwin/fork.cc 2007-01-22 22:12:40 -08:00 @@ -430,6 +430,8 @@ fork_parent (HANDLE& hParent, dll *&firs init_child_info (PROC_FORK1, &ch, 1, subproc_ready); + ch.msv_cnt = (sizeof ch)/5; + ch.forker_finished = forker_finished; stack_base (ch); ==== rt/src/winsup/cygwin/sigproc.cc ==== 2007-01-22 22:12:40-08:00, ob@... +1 -0 fill in the size of child_info --- 1.5/rt/src/winsup/cygwin/sigproc.cc 2003-02-12 07:50:27 -08:00 +++ 1.6/rt/src/winsup/cygwin/sigproc.cc 2007-01-22 22:12:40 -08:00 @@ -849,6 +849,7 @@ init_child_info (DWORD chtype, child_inf { TRACE_IN; memset (ch, 0, sizeof *ch); + ch->msv_cnt = (sizeof *ch)/5; ch->cb = sizeof *ch; ch->type = chtype; ch->cygpid = pid; ==== rt/src/winsup/cygwin/spawn.cc ==== 2007-01-22 22:12:40-08:00, ob@... +1 -0 fill in the size of child_info_spawn --- 1.22/rt/src/winsup/cygwin/spawn.cc 2003-10-11 07:35:26 -07:00 +++ 1.23/rt/src/winsup/cygwin/spawn.cc 2007-01-22 22:12:40 -08:00 @@ -346,6 +346,7 @@ spawn_guts (HANDLE hToken, const char * STARTUPINFO si = {0, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL}; child_info_spawn ciresrv; + ciresrv.msv_cnt = (sizeof ciresrv)/5; si.lpReserved2 = (LPBYTE) &ciresrv; si.cbReserved2 = sizeof (ciresrv); ---------------------------------------------------------------------- >Comment By: Cesar Strauss (cstrauss) Date: 2007-07-12 15:12 Message: Logged In: YES user_id=1369729 Originator: NO madwizard wrote: > The calculated size should be greater than the > actual size determined by cbReserved2; ... > I think if cbReserved2 is changed to size-4 > it might work and you don't have to worry about > reading past the data structure. Seems reasonable. See the attached patch (patch3.txt). Other changes to the original patch.txt are: 1) Move both count initializations to a single place (easier to maintain, less code duplication). 2) Added the "ChangeSet" text as a comment to the code. 3) ChangeLog is included (inline in the patch file). This new patch still needs someone to test it, since I don't have Vista 64. Regards, Cesar Strauss File Added: patch3.txt ---------------------------------------------------------------------- Comment By: madwizard (madwizard) Date: 2007-06-08 04:29 Message: Logged In: YES user_id=1729144 Originator: NO I understand the filler bytes now and it seems a logical choice. However it simply does not work on all systems. I think it might work if cbReserved2 is adjusted to not include the filler bytes. On the MSDN forum someone mentioned: "The actual value may be different from (((sizeof_the_buffer - 4) / 5) + 2) but it should satisfy two conditions: 1. The calculated size should be greater than the actual size determined by cbReserved2; 2. The calculated size should be as close to cbReserved2 as possible otherwise memory will be waisted." The size / 5 formula does not satisfy condition 1. I think if cbReserved2 is changed to size-4 it might work and you don't have to worry about reading past the data structure. ---------------------------------------------------------------------- Comment By: Cesar Strauss (cstrauss) Date: 2007-06-07 21:40 Message: Logged In: YES user_id=1369729 Originator: NO jdrasch, I think I like your first patch (patch.txt) better, with the "size / 5" formula and the filler bytes. First, I wish to remind you all that the child-info data is being copied in chunks of 5 bytes, and that the formula is for determining the number of chunks to be copied. With the new formula, (size - 4) / 5 + 2, the number of bytes copied is always greater than the size of the data. This is a bit dangerous, I think, because you are reading past the end of the structure. You could have the bad luck of walking into a protected memory region, or copying sensitive data from another variable. On the other hand, I believe the original "size / 5" formula does work correctly. Sure, you can lose up to 4 bytes at the end of the structure when doing so. But this is what the filler bytes are for. Besides, this was the solution adopted by the Cygwin project. About this change: - STARTUPINFO si = {0, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, NULL}; + STARTUPINFO si; + memset (&si, 0, sizeof (si)); Is it part of the fix, or just a style change? Finally, I like the fact that the fix only is applied to Vista, which is an improvement over the original patch. Regards, Cesar ---------------------------------------------------------------------- Comment By: Keith Marshall (keithmarshall) Date: 2007-06-04 18:45 Message: Logged In: YES user_id=823908 Originator: NO Thanks for the patch. This needs to be accompanied by a properly formatted ChangeLog, before it can be committed. I'd suggest that most of the detail provided in the so called ChangeSet should be folded into the source files, in the form of comments. The ChangeLog should provide a *complete* itemised list of the changes made, formatted per GNU Coding Standards; it is not the place to explain the rationale for the changes. ---------------------------------------------------------------------- Comment By: jdrasch (jdrasch) Date: 2007-05-30 06:03 Message: Logged In: YES user_id=1723996 Originator: YES File Added: patch2.txt ---------------------------------------------------------------------- Comment By: jdrasch (jdrasch) Date: 2007-05-30 05:50 Message: Logged In: YES user_id=1723996 Originator: YES I've also tried the suggested changes to the patch and they work fine on Vista 64 bit. > Also, I do not see the point of those 4 byte 'filler' members in the > different structures. Why are they included? I removed those as well as > they did not seem to cause or solve any problems. I originally used the above mentioned cygwin patch and "ported" it to MSYS, thats why they were introduced. > Seeing as size / 5 produces a number less than (size - 4) / 5 + 2 I wonder > if anyone has had success with this patch. In any case I think it should be > corrected to the right formula. Well at least I had success with this patch :-) But the suggested formular works for me as well so I strongly suggest using it. ---------------------------------------------------------------------- Comment By: madwizard (madwizard) Date: 2007-05-28 08:56 Message: Logged In: YES user_id=1729144 Originator: NO Because the WinAVR package uses some tools (make, sh, rm, mkdir etc.) built with MSYS I ran into the same bug on Vista 64. Because WinAVR uses MSYS 1.0.8 I modified the patch slightly but the code is largely the same in those files. However, this patch did not fix the problem and I found out the formula used for the msv_cnt field is wrong. See this topic on the MSDN forums: http://forums.microsoft.com/MSDN/ShowPost.aspx?PostID=998172&SiteID=1 A quote: "A workaround (just as you have said) is writing (((sizeof_the_buffer - 4) / 5) + 2) as a DWORD value to the first 4 bytes of the buffer. Then the buffer will be copied without any alterations." I modified the patch to use (size - 4) / 5 + 2 and the problem was solved. Also, I do not see the point of those 4 byte 'filler' members in the different structures. Why are they included? I removed those as well as they did not seem to cause or solve any problems. Seeing as size / 5 produces a number less than (size - 4) / 5 + 2 I wonder if anyone has had success with this patch. In any case I think it should be corrected to the right formula. ---------------------------------------------------------------------- Comment By: jdrasch (jdrasch) Date: 2007-03-09 08:27 Message: Logged In: YES user_id=1723996 Originator: YES Hi, I finally set up an environment to build MSYS, so I did the patch myself. The originally described patch is not simply possible, as it is based on the actual cygwin code, therefore I did some research and changed the according sourcecode. I also fixed this so that only for Vista the needed changes in the child-info are applied. Tested on Win XP SP2, Vista 32bit and Vista 64bit. Patch is attached for Files: Index: src/winsup/cygwin/spawn.cc Index: src/winsup/cygwin/fork.cc Index: src/winsup/cygwin/winsup.h Index: src/winsup/cygwin/dcrt0.cc Index: src/winsup/cygwin/sigproc.cc Index: src/winsup/cygwin/child_info.h File Added: patch.txt ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=102435&aid=1674783&group_id=2435 |