Ryan Johnson
2011-08-18 00:57:49 UTC
Hi all (attn: Corinna),
I've been looking into the way stderr disappears when a cygwin process
is started by gdb or strace (64-bit win7 box), and it looks like
cygheap->fdtab[2] somehow gets initialized to NULL. The EBADF which
results kills off the rest of the write "syscall."
Looking deeper shows that, in dtable::stdio_init, GetStdHandle() returns
the same value for stdout and stderr, but being_debugged() and
both gdb and strace, without harming non-traced execution. However, I
doubt that's the correct thing to do, since the other checks are clearly
not accidental. Calls to not_open(1) and not_open(2) both return 1, so I
wonder if an assumption has become invalid (e.g. did it used to be that
stderr should have already been opened but may have been closed already
as well, but now stderr has not even been opened yet?).
Corinna, can you dredge up any useful memories about the issue? The code
in dtable::get_debugger_info definitely runs (gdb prints "warning:
cYgstd 28cc69 d 3"), but std[][] remains empty, so none of the std
handles was initialized in that way.
So, which of the following changes, if any, is a proper fix? The first
assumes that the whole !not_open(2) thing has become completely bogus
(or always was), while the second is a more conservative workaround. The
third assumes that a reverse-sense boolean just slipped in unnoticed.
All three changes seem to behave correctly under my limited testing...
- if (out == err && (!being_debugged () || !not_open (2)))
+ if (out == err)
+ if (out == err && (!being_debugged () || (not_open (1) && not_open
(2)) || !not_open (2)))
+ if (out == err && (!being_debugged () || not_open (2)))
Based on the code comments, I suspect #2 is the correct fix -- stderr
must be usable if there's no debugger, if the debugger explicitly
initialized stderr (but to a duplicate handle that needs fixup), or --
this is the new case -- if the debugger didn't initialize any handles
(so stderr needs initialized with a duplicated handle).
Thoughts?
Ryan
it) suggests that it was intended to be part of the else clause, but
it's not. It's not causing problems in this particular case because it's
unreached, but it does look confusing.
I've been looking into the way stderr disappears when a cygwin process
is started by gdb or strace (64-bit win7 box), and it looks like
cygheap->fdtab[2] somehow gets initialized to NULL. The EBADF which
results kills off the rest of the write "syscall."
Looking deeper shows that, in dtable::stdio_init, GetStdHandle() returns
the same value for stdout and stderr, but being_debugged() and
/* STD_ERROR_HANDLE has been observed to be the same as
STD_OUTPUT_HANDLE. We need separate handles (e.g. using pipes
to pass data from child to parent). */
/* CV 2008-10-17: Under debugger control, std fd's have been potentially
initialized in dtable::get_debugger_info (). In this case
init_std_file_from_handle is a no-op, so, even if out == err we don't
want to duplicate the handle since it will be unused. */
if (out == err && (!being_debugged () || !not_open (2)))
{
/* Since this code is not invoked for forked tasks, we don't have
to worry about the close-on-exec flag here. */
if (!DuplicateHandle (GetCurrentProcess (), out,
GetCurrentProcess (), &err,
0, TRUE, DUPLICATE_SAME_ACCESS))
{
/* If that fails, do this as a fall back. */
err = out;
system_printf ("couldn't make stderr distinct from stdout, %E");
}
}
init_std_file_from_handle (1, out);
init_std_file_from_handle (2, err);
Always duplicating the handle when out==err seems to fix the problem forSTD_OUTPUT_HANDLE. We need separate handles (e.g. using pipes
to pass data from child to parent). */
/* CV 2008-10-17: Under debugger control, std fd's have been potentially
initialized in dtable::get_debugger_info (). In this case
init_std_file_from_handle is a no-op, so, even if out == err we don't
want to duplicate the handle since it will be unused. */
if (out == err && (!being_debugged () || !not_open (2)))
{
/* Since this code is not invoked for forked tasks, we don't have
to worry about the close-on-exec flag here. */
if (!DuplicateHandle (GetCurrentProcess (), out,
GetCurrentProcess (), &err,
0, TRUE, DUPLICATE_SAME_ACCESS))
{
/* If that fails, do this as a fall back. */
err = out;
system_printf ("couldn't make stderr distinct from stdout, %E");
}
}
init_std_file_from_handle (1, out);
init_std_file_from_handle (2, err);
both gdb and strace, without harming non-traced execution. However, I
doubt that's the correct thing to do, since the other checks are clearly
not accidental. Calls to not_open(1) and not_open(2) both return 1, so I
wonder if an assumption has become invalid (e.g. did it used to be that
stderr should have already been opened but may have been closed already
as well, but now stderr has not even been opened yet?).
Corinna, can you dredge up any useful memories about the issue? The code
in dtable::get_debugger_info definitely runs (gdb prints "warning:
cYgstd 28cc69 d 3"), but std[][] remains empty, so none of the std
handles was initialized in that way.
So, which of the following changes, if any, is a proper fix? The first
assumes that the whole !not_open(2) thing has become completely bogus
(or always was), while the second is a more conservative workaround. The
third assumes that a reverse-sense boolean just slipped in unnoticed.
All three changes seem to behave correctly under my limited testing...
- if (out == err && (!being_debugged () || !not_open (2)))
+ if (out == err)
+ if (out == err && (!being_debugged () || (not_open (1) && not_open
(2)) || !not_open (2)))
+ if (out == err && (!being_debugged () || not_open (2)))
Based on the code comments, I suspect #2 is the correct fix -- stderr
must be usable if there's no debugger, if the debugger explicitly
initialized stderr (but to a duplicate handle that needs fixup), or --
this is the new case -- if the debugger didn't initialize any handles
(so stderr needs initialized with a duplicated handle).
Thoughts?
Ryan
if (!fh->open ((i ? (i == 2 ? O_RDWR : O_WRONLY) : O_RDONLY)
| O_BINARY, 0777))
release (i);
else
CloseHandle (h);
/* Copy to Windows' idea of a standard handle, otherwise
we have invalid standard handles when calling Windows
functions (small_printf and strace might suffer, too). */
SetStdHandle (std_consts[i], i ? fh->get_output_handle ()
: fh->get_handle ());
The indentation of the call to SetStdHandle (and the comment describing| O_BINARY, 0777))
release (i);
else
CloseHandle (h);
/* Copy to Windows' idea of a standard handle, otherwise
we have invalid standard handles when calling Windows
functions (small_printf and strace might suffer, too). */
SetStdHandle (std_consts[i], i ? fh->get_output_handle ()
: fh->get_handle ());
it) suggests that it was intended to be part of the else clause, but
it's not. It's not causing problems in this particular case because it's
unreached, but it does look confusing.