Potential fix: gdb/strace swallows stderr

[Ryan Johnson <ryan dot johnson at cs dot utoronto dot ca>]

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 
not_open(2) both return 1, with the result that this code doesn't run:
>   /* 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");
>         }
>     }
... with the result that err is invalid when this runs immediately after:
>   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 for 
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

P.S. this snippet from dtable::get_debugger_info looks suspicious:
>             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 
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.