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: Vista Issue
Status: Closed
Resolution: Fixed
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: 2008-09-24 23:48
Message:
Could you try the msysCORE package?
It can be found at
http://sourceforge.net/project/showfiles.php?group_id=2435&package_id=24963
Unpack it in a empty directory and run msys.bat. Does it help?
Regards,
Cesar
----------------------------------------------------------------------
Comment By: eldonantonio (eldonantonio)
Date: 2008-09-23 16:42
Message:
Hey guys, I'm using msys 1.0.11 and I'm still having this problem in
Vista64. Trying to launch rxvt in a SysWOW64 cmd prompt opens 130 windows
before closing them all. Did I miss something?
Thanks,
Antoine
----------------------------------------------------------------------
Comment By: eldonantonio (eldonantonio)
Date: 2008-09-23 16:41
Message:
Hey guys, I'm using msys 1.0.11 and I'm still having this problem in
Vista64. Trying to launch rxvt in a SysWOW64 cmd prompt opens 130 windows
before closing them all. Did I miss something?
Thanks,
Antoine
----------------------------------------------------------------------
Comment By: Cesar Strauss (cstrauss)
Date: 2008-07-10 23:32
Message:
Logged In: YES
user_id=1369729
Originator: NO
The Vista patch seems to be successful, so I am closing this tracker item.
Thanks for all the reports and contributions.
Regards,
Cesar
----------------------------------------------------------------------
Comment By: Alexander J. Vincent (jslab)
Date: 2008-06-09 23:59
Message:
Logged In: YES
user_id=134163
Originator: NO
I can confirm that using the MSYS-1.0.11 tarballed files as described by
cstrauss fixes the bug I had launching bash, which was precisely as ibison
described it. Reference [1] for my observations.
[1]
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/2846365cf35a9b32/c8d07608e60e5467?lnk=gst&q=MozillaBuild+on+Vista+x64#c8d07608e60e5467
----------------------------------------------------------------------
Comment By: Cesar Strauss (cstrauss)
Date: 2007-11-04 18:23
Message:
Logged In: YES
user_id=1369729
Originator: NO
Did you try installing the following package?
http://downloads.sourceforge.net/mingw/MSYS-1.0.11-20070729.tar.bz2
It was just moved to a new location, you could have missed it.
Unpack it on your existing MSYS "bin" directory. It should overwrite these
files:
msys-1.0.dll
mount.exe
ps.exe
----------------------------------------------------------------------
Comment By: ibison (ibison)
Date: 2007-11-04 00:44
Message:
Logged In: YES
user_id=1928467
Originator: NO
I cannot get the various versions of MSYS that I've tried to work on 64
bit Vista. Both sh and rxvt fail. sh complains about a fork and then quits,
and MSYS continuously spawns multiple copies of rxvt until it crashes.
Seems like you may have fixed the problem for cygwin. Can I trouble you for
a fix for MSYS?
----------------------------------------------------------------------
Comment By: Cesar Strauss (cstrauss)
Date: 2007-07-28 19:49
Message:
Logged In: YES
user_id=1369729
Originator: NO
Oscar Bonilla wrote:
> Note that the isVistaWOW64 variable will be true for
> all vista versions, regardless of whether they are
> Vista 32 or Vista 64.
You are right, of course. Thanks for the heads-up.
Here is a new version (patch4.txt) that uses the IsWow64Process API call.
For this, I reused an interesting Cygwin technique called "auto-load
functions" (see autoload.cc). If I understand correctly, it transparently
intercepts the API call, loads the appropriate system DLL, and either
forwards the call to the API function, or returns a failure code if the API
doesn't exist at run-time. There is no need to use
GetModuleHandle/GetProcAddress anymore...
I also had to insert the prototype for IsWow64Process manually, because
the w32api which is used to build MSYS pre-dates that.
Regards,
Cesar
File Added: patch4.txt
----------------------------------------------------------------------
Comment By: Oscar Bonilla (obonilla)
Date: 2007-07-12 15:30
Message:
Logged In: YES
user_id=219610
Originator: NO
Note that the isVistaWOW64 variable will be true for all vista
versions, regardless of whether they are Vista 32 or Vista 64.
a better test would be:
if(os_version_info.dwMajorVersion == 6)
{
HMODULE handle = GetModuleHandle("Kernel32");
if (handle)
{
FARPROC ph = GetProcAddress(handle, "GetSystemWow64DirectoryW");
if (ph && ph(path, sizeof(path))) isVistaWOW64 = 1;
}
}
(typed in the browser, from memory since i don't have access to my windows
box
right now... so it needs to be tested.)
----------------------------------------------------------------------
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
|
