This is the mail archive of the cygwin-developers mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Potential fix: gdb/strace swallows stderr


On 18/08/2011 5:14 AM, Corinna Vinschen wrote:
On Aug 17 20:57, Ryan Johnson wrote:
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. */

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
Ha ha ha, huh huh, good joke.  After three years, the comment is all
what's left of the entire scenario. :}
I know that feeling all too well... the wording of the question was no accident ;)

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).
I'm wondering how I could ever apply this.  The !not_open(2) is just
plain wrong (looks like a copy-paste bug).  If not_open(2), then we
*want* to initialize fd 2, no matter what.  If !not_open(2), then fd 2
has been initialized and we don't want to create a useless handle.
So the condition is just upside down.  I'll apply a patch.
Hmm. Looking at webcvs, it seems you went with this route:
if (out == err && (!being_debugged () || not_open (2)))

Maybe I'm just paranoid, but can it ever arise that out==err when !being_debugged()? If so, is it correct to create a duplicate handle in that case? I assume it is, given the code comment about needing different handles. But then, if out==err because the debugger initialized them that way, wouldn't we again need to duplicate the handles to avoid whatever trouble is presumably caused by not duplicating them?

Ryan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]