[patch] fix spurious SIGSEGV faults under Cygwin

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] fix spurious SIGSEGV faults under Cygwin

Brian Dessent

Recently Cygwin has changed the way that it internally checks arguments for bad
pointers.  Where before it used IsBad{Read,Write}Ptr() from the win32 API, now
it installs its own temporary SEH fault handler.  However, gdb is not aware of
this and so it leads to the program stopping on a SIGSEGV on perfectly
legitimate code -- when not run under the debugger, Cygwin's fault handler
catches the attempt and returns the appropriate error.  Here is a minimal
example:

$ cat ss.cc
#include <string>

int main()
{
  std::string foo;
 
  return 0;
}

$ g++ -g ss.cc -o ss

$ gdb --quiet ./ss
(gdb) r
Starting program: /tmp/false_sigsegv/ss.exe

Program received signal SIGSEGV, Segmentation fault.
0x610af8b8 in pthread_key_create (key=0x482c08, destructor=0) at
/usr/src/sourceware/winsup/cygwin/thread.cc:129
129       if ((*object)->magic != magic)
(gdb) c
Continuing.

Program exited normally.
(gdb)

Here pthread_key_create() was simply checking to see if 'key' happened to be a
previously-initiaized object, which in most cases is not true, and so the fault
is expected and handled.  However, it's very inconvenient and unintuitive that
gdb still stops the program, and it makes the user think there is something
wrong with his program and/or the Cygwin library.

The attached patch adds a very simple mechanism by which the Cygwin program
shall signal to gdb that it has temporarily installed its own fault handler for
EXCEPTION_ACCESS_VIOLATION, so that gdb can deal with this more gracefully by
ignoring these faults.  This uses the existing WriteDebugString() mechanism that
Cygwin already uses to communicate to the debugger, but just extends it by
defining two new "magic" string values.

I've split the patch into two parts since it requires changes to both cygwin and
gdb.

Brian

winsup/cygwin:
2006-02-02  Brian Dessent  <[hidden email]>

        * cygtls.h: Include sys/cygwin.h.
        (myfault::~myfault): If a debugger is present, inform it that our fault
        handler has been removed.
        (myfault::faulted): Likewise for when the fault handler is installed.
        * include/sys/cygwin.h (_CYGWIN_FAULT_IGNORE_STRING): Add define.
        (_CYGWIN_FAULT_NOIGNORE_STRING): Ditto.


gdb:
2006-02-02  Brian Dessent  <[hidden email]>

        * win32-nat.c (_CYGWIN_SIGNAL_STRING): Remove duplicated definition.
        (ignoring_faults): Define new static global.
        (handle_output_debug_string): Check for fault ignore/noignore signals
        from the inferior.
        (handle_exception): Do not process the fault if 'ignoring_faults' is
        set.
        (do_initial_win32_stuff): Initialize 'ignoring_faults'.
Index: cygtls.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/cygtls.h,v
retrieving revision 1.42
diff -u -p -r1.42 cygtls.h
--- cygtls.h 23 Dec 2005 22:50:20 -0000 1.42
+++ cygtls.h 2 Feb 2006 11:43:23 -0000
@@ -1,6 +1,6 @@
 /* cygtls.h
 
-   Copyright 2003, 2004, 2005 Red Hat, Inc.
+   Copyright 2003, 2004, 2005, 2006 Red Hat, Inc.
 
 This software is a copyrighted work licensed under the terms of the
 Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
@@ -22,6 +22,7 @@ details. */
 #include <netinet/in.h>
 typedef unsigned int SOCKET;
 #endif
+#include <sys/cygwin.h>
 
 #define CYGTLS_INITIALIZED 0x43227
 #define CYGTLSMAGIC "D0Ub313v31nm&G1c?";
@@ -242,9 +243,16 @@ class myfault
   jmp_buf buf;
   san sebastian;
 public:
-  ~myfault () __attribute__ ((always_inline)) { _my_tls.reset_fault (sebastian); }
+  ~myfault () __attribute__ ((always_inline))
+  {
+    _my_tls.reset_fault (sebastian);
+    if (being_debugged ())
+      OutputDebugString (_CYGWIN_FAULT_NOIGNORE_STRING);
+  }
   inline int faulted (int myerrno = 0) __attribute__ ((always_inline))
   {
+    if (being_debugged ())
+      OutputDebugString (_CYGWIN_FAULT_IGNORE_STRING);
     return _my_tls.setup_fault (buf, sebastian, myerrno);
   }
 };
Index: include/sys/cygwin.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/include/sys/cygwin.h,v
retrieving revision 1.61
diff -u -p -r1.61 cygwin.h
--- include/sys/cygwin.h 1 Nov 2005 05:55:30 -0000 1.61
+++ include/sys/cygwin.h 2 Feb 2006 11:43:23 -0000
@@ -1,6 +1,6 @@
 /* sys/cygwin.h
 
-   Copyright 1997, 1998, 2000, 2001, 2002 Red Hat, Inc.
+   Copyright 1997, 1998, 2000, 2001, 2002, 2006 Red Hat, Inc.
 
 This file is part of Cygwin.
 
@@ -18,6 +18,8 @@ extern "C" {
 #endif
 
 #define _CYGWIN_SIGNAL_STRING "cYgSiGw00f"
+#define _CYGWIN_FAULT_IGNORE_STRING "cYgfAuLtIg"
+#define _CYGWIN_FAULT_NOIGNORE_STRING "cYgNofAuLtIg"
 
 extern pid_t cygwin32_winpid_to_pid (int);
 extern void cygwin32_win32_to_posix_path_list (const char *, char *);


Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.119
diff -u -p -r1.119 win32-nat.c
--- win32-nat.c 24 Jan 2006 22:09:28 -0000 1.119
+++ win32-nat.c 2 Feb 2006 11:44:08 -0000
@@ -83,12 +83,6 @@ static unsigned dr[8];
 static int debug_registers_changed;
 static int debug_registers_used;
 
-/* The string sent by cygwin when it processes a signal.
-   FIXME: This should be in a cygwin include file. */
-#ifndef _CYGWIN_SIGNAL_STRING
-#define _CYGWIN_SIGNAL_STRING "cYgSiGw00f"
-#endif
-
 #define CHECK(x) check (x, __FILE__,__LINE__)
 #define DEBUG_EXEC(x) if (debug_exec) printf_unfiltered x
 #define DEBUG_EVENTS(x) if (debug_events) printf_unfiltered x
@@ -126,6 +120,12 @@ static DEBUG_EVENT current_event; /* The
 static HANDLE current_process_handle; /* Currently executing process */
 static thread_info *current_thread; /* Info on currently selected thread */
 static DWORD main_thread_id; /* Thread ID of the main thread */
+static int ignoring_faults;             /* If true, the inferior has signaled
+                                           that it is doing parameter checking
+                                           and has supplied its own
+                                           EXCEPTION_ACCESS_VIOLATION handler,
+                                           and there is no need to act on
+                                           them here.  */
 
 /* Counts of things. */
 static int exception_count = 0;
@@ -905,12 +905,7 @@ handle_output_debug_string (struct targe
       || !s || !*s)
     return gotasig;
 
-  if (strncmp (s, _CYGWIN_SIGNAL_STRING, sizeof (_CYGWIN_SIGNAL_STRING) - 1) != 0)
-    {
-      if (strncmp (s, "cYg", 3) != 0)
- warning (("%s"), s);
-    }
-  else
+  if (strncmp (s, _CYGWIN_SIGNAL_STRING, sizeof (_CYGWIN_SIGNAL_STRING) - 1) == 0)
     {
       char *p;
       int sig = strtol (s + sizeof (_CYGWIN_SIGNAL_STRING) - 1, &p, 0);
@@ -919,6 +914,21 @@ handle_output_debug_string (struct targe
       if (gotasig)
  ourstatus->kind = TARGET_WAITKIND_STOPPED;
     }
+  else if (strncmp (s, _CYGWIN_FAULT_IGNORE_STRING,
+    sizeof (_CYGWIN_FAULT_IGNORE_STRING) - 1) == 0)
+    {
+      ignoring_faults = TRUE;
+    }
+  else if (strncmp (s, _CYGWIN_FAULT_NOIGNORE_STRING,
+    sizeof (_CYGWIN_FAULT_NOIGNORE_STRING) - 1) == 0)
+    {
+      ignoring_faults = FALSE;
+    }
+  else
+    {
+      if (strncmp (s, "cYg", 3) != 0)
+ warning (("%s"), s);
+    }
 
   xfree (s);
   return gotasig;
@@ -1066,10 +1076,11 @@ handle_exception (struct target_waitstat
       ourstatus->value.sig = TARGET_SIGNAL_SEGV;
       {
  char *fn;
- if (find_pc_partial_function ((CORE_ADDR) current_event.u.Exception
-      .ExceptionRecord.ExceptionAddress,
-      &fn, NULL, NULL)
-    && strncmp (fn, "KERNEL32!IsBad", strlen ("KERNEL32!IsBad")) == 0)
+ if (ignoring_faults
+    || (find_pc_partial_function ((CORE_ADDR) current_event.u.Exception
+  .ExceptionRecord.ExceptionAddress,
+  &fn, NULL, NULL)
+ && strncmp (fn, "KERNEL32!IsBad", strlen ("KERNEL32!IsBad")) == 0))
   return 0;
       }
       break;
@@ -1434,6 +1445,7 @@ do_initial_win32_stuff (DWORD pid)
   exception_count = 0;
   debug_registers_changed = 0;
   debug_registers_used = 0;
+  ignoring_faults = FALSE;
   for (i = 0; i < sizeof (dr) / sizeof (dr[0]); i++)
     dr[i] = 0;
   current_event.dwProcessId = pid;

Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Brian Dessent
Brian Dessent wrote:

>  #define _CYGWIN_SIGNAL_STRING "cYgSiGw00f"
> +#define _CYGWIN_FAULT_IGNORE_STRING "cYgfAuLtIg"
> +#define _CYGWIN_FAULT_NOIGNORE_STRING "cYgNofAuLtIg"

Sigh, this breaks strace under Cygwin, I should have tested more.  Sorry
about that.  Apparently strace expects anything starting with the 'cYg'
prefix to be followed by a hex number.  I thought that since
_CYGWIN_SIGNAL_STRING already existed and didn't follow that format it
was safe to add more, but that's not the case.

So, should I pick another prefix that's not 'cYg'?  Or instead use
something like "cYg0 ..." since strace seems to just ignore the string
if its value is 0?  Or something else?

Brian
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Christopher Faylor-2
On Thu, Feb 02, 2006 at 08:00:01AM -0800, Brian Dessent wrote:

>Brian Dessent wrote:
>
>>  #define _CYGWIN_SIGNAL_STRING "cYgSiGw00f"
>> +#define _CYGWIN_FAULT_IGNORE_STRING "cYgfAuLtIg"
>> +#define _CYGWIN_FAULT_NOIGNORE_STRING "cYgNofAuLtIg"
>
>Sigh, this breaks strace under Cygwin, I should have tested more.  Sorry
>about that.  Apparently strace expects anything starting with the 'cYg'
>prefix to be followed by a hex number.  I thought that since
>_CYGWIN_SIGNAL_STRING already existed and didn't follow that format it
>was safe to add more, but that's not the case.
>
>So, should I pick another prefix that's not 'cYg'?  Or instead use
>something like "cYg0 ..." since strace seems to just ignore the string
>if its value is 0?  Or something else?

Brian,
Thanks for the patch but I've been working on this too and, so far, I think
it is possible to have a very minimal way of dealing with this problem.  I
haven't had time to delve into it too deeply but I have been exploring this
problem on and off for a couple of weeks.  If the situation at work calms
down a little I may be able to finish up what I've been working on.

OTOH, if what I have is really not working then I'll take a look at what
you've done.

Again, thanks for the patch.  I probably should have sent a heads up that
I was working on this.

cgf
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Brian Dessent
Christopher Faylor wrote:

> Thanks for the patch but I've been working on this too and, so far, I think
> it is possible to have a very minimal way of dealing with this problem.  I
> haven't had time to delve into it too deeply but I have been exploring this
> problem on and off for a couple of weeks.  If the situation at work calms
> down a little I may be able to finish up what I've been working on.
>
> OTOH, if what I have is really not working then I'll take a look at what
> you've done.

Okay, no rush.  FWIW after noticing that strace was broken I tested a
version that used

 #define _CYGWIN_SIGNAL_STRING "cYgSiGw00f"
+#define _CYGWIN_FAULT_IGNORE_STRING "cYg0 faultig"
+#define _CYGWIN_FAULT_NOIGNORE_STRING "cYg0 nofaultig"

...which seems to fix the problem since the strtold() just picks up 0
after 'cYg' and strace ignores the rest.

The main problem I see with this approach is the extra call to
IsDebuggerPresent() every time a 'myfault' is created/destroyed, which
potentially could be a lot.  I'm presuming this is a relatively cheap
call so it wasn't something I worried too much about.  But then I didn't
actually try to measure it.

If it turns out that it's expensive, I was thinking that the inferior
could maintain this information in some variable, and just communicate
its location to gdb once at startup, then gdb could simply read that
variable in the process' memory before deciding whether to handle the
fault.  Or it could try to look at the SEH chain and see if a handler
residing in cygwin1.dll is setup to handle the fault, and if so just
silently pass it along.  But that seemed really complicated when there
already exists this mechanism for the process to communicate with the
debugger.

Brian
Reply | Threaded
Open this post in threaded view
|

RE: [patch] fix spurious SIGSEGV faults under Cygwin

Dave Korn
On 02 February 2006 17:21, Brian Dessent wrote:

> The main problem I see with this approach is the extra call to
> IsDebuggerPresent() every time a 'myfault' is created/destroyed, which
> potentially could be a lot.  I'm presuming this is a relatively cheap
> call so it wasn't something I worried too much about.  But then I didn't
> actually try to measure it.  
>
> If it turns out that it's expensive, I was thinking that the inferior
> could maintain this information in some variable, and just communicate
> its location to gdb once at startup, then gdb could simply read that
> variable in the process' memory before deciding whether to handle the
> fault.

  ?????!

  I'm having a conceptual difficulty here: Under what circumstances would you expect there *not* to be a debugger attached to the
inferior to which the debugger is attached?  That's a bit zen, isn't it?

  Or IOW if a debugger is going to read a variable from its inferior that says if there's a debugger attached, well... it might as
well be #defined to 1 in the gdb source code, mightn't it?


    cheers,
      DaveK
--
Can't think of a witty .sigline today....

Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Daniel Jacobowitz-2
On Thu, Feb 02, 2006 at 05:30:23PM -0000, Dave Korn wrote:
>   ?????!
>
>   I'm having a conceptual difficulty here: Under what circumstances
> would you expect there *not* to be a debugger attached to the
> inferior to which the debugger is attached?  That's a bit zen, isn't
> it?

You missed that half the patch was for Cygwin - you even sent your
reply to cygwin-patches.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Brian Dessent
In reply to this post by Dave Korn
Dave Korn wrote:

>   I'm having a conceptual difficulty here: Under what circumstances would you expect there *not* to be a debugger attached to the
> inferior to which the debugger is attached?  That's a bit zen, isn't it?

The code in question here runs many times in the normal course of any
Cygwin program -- debugger or no.  The idea behind guarding the call to
OutputDebugString() with "if (being_debugged())" was that the call to
IsDebuggerPresent() was cheaper than the call to OutputDebugString(),
and that a well-behaived, non-debug build of a binary should not
needlessly send tons and tons of nonsense to OutputDebugString unless
it's actually being debugged and there is something there to interpret
the nonsense.

Brian
Reply | Threaded
Open this post in threaded view
|

RE: [patch] fix spurious SIGSEGV faults under Cygwin

Dave Korn
In reply to this post by Daniel Jacobowitz-2
On 02 February 2006 17:38, Daniel Jacobowitz wrote:

> On Thu, Feb 02, 2006 at 05:30:23PM -0000, Dave Korn wrote:
>>   ?????!
>>
>>   I'm having a conceptual difficulty here: Under what circumstances
>> would you expect there *not* to be a debugger attached to the inferior
>> to which the debugger is attached?  That's a bit zen, isn't it?
>
> You missed that half the patch was for Cygwin - you even sent your reply
> to cygwin-patches.


  Nope, I didn't miss that bit.  I can see the point in the cygwin side of the patch, for when there _isn't_ a debugger attached,
and the cygwin library has to know whether the exception will be handled by a debugger or whether it has to wrap the access in a
SEH.

  What I can't see is any point in gdb reading a variable from the inferior that tells gdb if there is a debugger attached to the
inferior, because I can't see how gdb could read that variable except by attaching to the inferior, at which point the value in the
variable should always be 'true', shouldn't it?


    cheers,
      DaveK
--
Can't think of a witty .sigline today....

Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Daniel Jacobowitz-2
On Thu, Feb 02, 2006 at 05:43:51PM -0000, Dave Korn wrote:
>   What I can't see is any point in gdb reading a variable from the
> inferior that tells gdb if there is a debugger attached to the
> inferior, because I can't see how gdb could read that variable except
> by attaching to the inferior, at which point the value in the
> variable should always be 'true', shouldn't it?

He wrote:

> If it turns out that it's expensive, I was thinking that the inferior
> could maintain this information in some variable, and just communicate
> its location to gdb once at startup, then gdb could simply read that
> variable in the process' memory before deciding whether to handle the
> fault.

"this information" does not refer to "if there is a debugger attached".

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

RE: [patch] fix spurious SIGSEGV faults under Cygwin

Dave Korn
In reply to this post by Brian Dessent
On 02 February 2006 17:42, Brian Dessent wrote:

> Dave Korn wrote:
>
>>   I'm having a conceptual difficulty here: Under what circumstances
>> would you expect there *not* to be a debugger attached to the inferior
>> to which the debugger is attached?  That's a bit zen, isn't it?
>
> The code in question here runs many times in the normal course of any
> Cygwin program -- debugger or no.  The idea behind guarding the call to
> OutputDebugString() with "if (being_debugged())" was that the call to
> IsDebuggerPresent() was cheaper than the call to OutputDebugString(), and
> that a well-behaived, non-debug build of a binary should not needlessly
> send tons and tons of nonsense to OutputDebugString unless it's actually
> being debugged and there is something there to interpret the nonsense.  


  Um, that's two people now who thought I was referring to the cygwin side of the patch.

  No, this is the bit of your post that I was replying to:

"then gdb could simply read that variable in the process' memory before deciding whether to handle the fault. "

  Is it the case that IsDebuggerPresent doesn't detect when gdb is attached?

    cheers,
      DaveK
--
Can't think of a witty .sigline today....

Reply | Threaded
Open this post in threaded view
|

RE: [patch] fix spurious SIGSEGV faults under Cygwin

Dave Korn
In reply to this post by Daniel Jacobowitz-2
On 02 February 2006 17:46, 'Daniel Jacobowitz' wrote:

> "this information" does not refer to "if there is a debugger attached".


  Ah!  Thank you for solving my parsing problem!

  I still can't tell if that's because "this information" does not refer to the return value of IsDebuggerPresent, or because it
does but that return value does not detect gdb, but the feeling of utter bewilderment is subsiding somewhat!


    cheers,
      DaveK
--
Can't think of a witty .sigline today....

Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Daniel Jacobowitz-2
On Thu, Feb 02, 2006 at 05:51:54PM -0000, Dave Korn wrote:
> On 02 February 2006 17:46, 'Daniel Jacobowitz' wrote:
>
> > "this information" does not refer to "if there is a debugger attached".
>
>
>   Ah!  Thank you for solving my parsing problem!
>
>   I still can't tell if that's because "this information" does not refer to the return value of IsDebuggerPresent, or because it
> does but that return value does not detect gdb, but the feeling of utter bewilderment is subsiding somewhat!

I assume it refers to whether Cygwin is handling a validity check at
the moment.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fix spurious SIGSEGV faults under Cygwin

Christopher Faylor-2
In reply to this post by Christopher Faylor-2
On Thu, Feb 02, 2006 at 12:05:58PM -0500, Christopher Faylor wrote:

>On Thu, Feb 02, 2006 at 08:00:01AM -0800, Brian Dessent wrote:
>>Brian Dessent wrote:
>>
>>>  #define _CYGWIN_SIGNAL_STRING "cYgSiGw00f"
>>> +#define _CYGWIN_FAULT_IGNORE_STRING "cYgfAuLtIg"
>>> +#define _CYGWIN_FAULT_NOIGNORE_STRING "cYgNofAuLtIg"
>>
>>Sigh, this breaks strace under Cygwin, I should have tested more.  Sorry
>>about that.  Apparently strace expects anything starting with the 'cYg'
>>prefix to be followed by a hex number.  I thought that since
>>_CYGWIN_SIGNAL_STRING already existed and didn't follow that format it
>>was safe to add more, but that's not the case.
>>
>>So, should I pick another prefix that's not 'cYg'?  Or instead use
>>something like "cYg0 ..." since strace seems to just ignore the string
>>if its value is 0?  Or something else?
>
>Brian,
>Thanks for the patch but I've been working on this too and, so far, I think
>it is possible to have a very minimal way of dealing with this problem.  I
>haven't had time to delve into it too deeply but I have been exploring this
>problem on and off for a couple of weeks.  If the situation at work calms
>down a little I may be able to finish up what I've been working on.
>
>OTOH, if what I have is really not working then I'll take a look at what
>you've done.
>
>Again, thanks for the patch.  I probably should have sent a heads up that
>I was working on this.

Actually, my minimal solution died in annoying ways.  I don't really
understand why.

So, I opted to push forward on my work to make cygwin signals recognized
(using _CYGWIN_SIGNAL_STRING) by gdb.  I have something now which
ignores exceptions in the cygwin DLL when they are based on a myfault
interrupt and it has the added benefit of potentially allowing SIGABRT,
SIGQUIT, and other signals to be noticed by gdb.

So, thanks again for the patch and sorry for the duplication of effort.

cgf