ntdll: Fix SIGTRAP handling

[Petr Tesarik]

Hi,

the handling of SIGTRAP is still broken in several points:

1. It is not the task of the handler to reset the TF flag in the
   exception context.  For the execution of the exception handler, the
   TF bit is reset in setup_exception(), and after returning from the
   exception handler we do want to keep the flag set. (Otherwise the
   debugging stops unless the exception handler explicitly re-enables
   the TF bit in the context.)

2. The x86 spec states that a single-step interrupt (INT 1) is
   generated if the TF bit was set when the execution of the
   instruction BEGAN.  This means that if the instruction changes the
   TF bit (e.g. popf), there is an INT 1, but the TF bit is not set
   any more.  The only way to check whether the interrupt was
   generated because of the TF bit, is to check bit 15 in DR6.

3. Although the current code makes use of the DR6 already, it does not
   clear it.  That means that unless the exception handler (most
   likely a debugger) clears it, the reason of the next debugging
   exception cannot be determined.

4. The SIGTRAP handler assumes that TRAP_sig is sufficient to get the
   reason of the signal.  However, Linux only sets the trap code in
   interrupt handlers.  In particular, this means that it is not
   modified for user-generated signals (kill(), tkill() and friends)
   and it still holds the code of the last generated hardware
   interrupt for that task (not connected with the current signal in
   any way).

5. Since TRAP_sig() is not defined on some architectures, we need to
   check both conditions if get_trap_sig() returns TRAP_x86_UNKNOWN.

The following patch fixes all of these issues.

ChangeLog:

* Fix TF bit handling by the SIGTRAP handler
* Clear DR6 before returning from the SIGTRAP handler
* Improve the distinction between hardware-generated SIGTRAP and a
  result of kill(SIGTRAP)
-------------- next part --------------
--- signal_i386.c	2006-03-27 11:07:30.000000000 +0200
+++ signal_i386.c.tfbit	2006-03-27 14:40:38.000000000 +0200
@@ -1103,31 +1103,121 @@
 
 
 /**********************************************************************
+ *		is_singlestep_context
+ */
+static int is_singlestep_context( CONTEXT *context )
+{
+    DWORD saved_flags;
+
+    saved_flags = context->ContextFlags;
+    context->ContextFlags = CONTEXT_DEBUG_REGISTERS;
+    NtGetContextThread(GetCurrentThread(), context);
+    context->ContextFlags |= saved_flags;
+
+    /* check all possible reasons for an INT1 */
+    return (context->Dr6 & 0x0000D00F);
+}
+
+
+/**********************************************************************
+ *		is_breakpoint_address
+ */
+static int is_breakpoint_address( PBYTE address )
+{
+    int ret = 0;
+
+    __TRY
+    {
+        /* Check for a preceding INT3 or INT 03 instruction */
+        if (address[-1] == 0xcc ||
+            (address[-2] == 0xcd && address[-1] == 0x03))
+            ret = 1;
+    }
+    __EXCEPT_PAGE_FAULT
+    {
+        /* Empty (return code is 0 already) */
+    }
+    __ENDTRY
+
+    return ret;
+}
+
+
+/**********************************************************************
+ *		restore_trap_context
+ */
+static void restore_trap_context( CONTEXT *context )
+{
+    /* Note that the bits in DR6 are never cleared by the processor,
+     * so unless we clear DR6 now, we might not be able to identify
+     * the next debugging exception correctly.
+     */
+    if (context->ContextFlags & CONTEXT_DEBUG_REGISTERS)
+    {
+        DWORD saved_flags;
+
+        saved_flags = context->ContextFlags;
+        context->ContextFlags = CONTEXT_DEBUG_REGISTERS;
+        NtGetContextThread( GetCurrentThread(), context );
+        context->ContextFlags = saved_flags;
+        context->Dr6 = 0;
+    }
+
+    NtSetContextThread( GetCurrentThread(), context );
+}
+
+
+/**********************************************************************
+ *		raise_singlestep_exception
+ */
+static void WINAPI raise_singlestep_exception( EXCEPTION_RECORD *rec, CONTEXT *context )
+{
+    rec->ExceptionCode = is_singlestep_context( context )
+        ? EXCEPTION_SINGLE_STEP
+        : EXCEPTION_BREAKPOINT;
+
+    __regs_RtlRaiseException( rec, context );
+    restore_trap_context( context );
+}
+
+
+/**********************************************************************
+ *		raise_breakpoint_exception
+ */
+static void WINAPI raise_breakpoint_exception( EXCEPTION_RECORD *rec, CONTEXT *context )
+{
+    /* back up over the int3 instruction if necessary */
+    if (is_breakpoint_address( (PBYTE)rec->ExceptionAddress ))
+        rec->ExceptionAddress = (char *)rec->ExceptionAddress - 1;
+
+    rec->ExceptionCode = EXCEPTION_BREAKPOINT;
+
+    __regs_RtlRaiseException( rec, context );
+    restore_trap_context( context );
+}
+
+
+/**********************************************************************
  *		raise_trap_exception
  */
 static void WINAPI raise_trap_exception( EXCEPTION_RECORD *rec, CONTEXT *context )
 {
-    if (rec->ExceptionCode == EXCEPTION_SINGLE_STEP)
+    /* Check for INT1 conditions first in case a hardware
+     * debugging interrupt immediately follows an INT3.
+     */
+    if (is_singlestep_context( context ))
+        rec->ExceptionCode = EXCEPTION_SINGLE_STEP;
+    else
     {
-        if (context->EFlags & 0x100)
-        {
-            context->EFlags &= ~0x100;  /* clear single-step flag */
-        }
-        else  /* hardware breakpoint, fetch the debug registers */
-        {
-            context->ContextFlags = CONTEXT_DEBUG_REGISTERS;
-            NtGetContextThread(GetCurrentThread(), context);
-            /* do we really have a bp from a debug register ?
-             * if not, then someone did a kill(SIGTRAP) on us, and we
-             * shall return a breakpoint, not a single step exception
-             */
-            if (!(context->Dr6 & 0xf)) rec->ExceptionCode = EXCEPTION_BREAKPOINT;
-            context->ContextFlags |= CONTEXT_FULL;  /* restore flags */
-        }
+        /* Only back up over the int3 if there is one */
+        if (is_breakpoint_address( (PBYTE)rec->ExceptionAddress ))
+            rec->ExceptionAddress = (char *)rec->ExceptionAddress - 1;
+
+        rec->ExceptionCode = EXCEPTION_BREAKPOINT;
     }
 
     __regs_RtlRaiseException( rec, context );
-    NtSetContextThread( GetCurrentThread(), context );
+    restore_trap_context( context );
 }
 
 
@@ -1262,17 +1352,22 @@
  */
 static HANDLER_DEF(trap_handler)
 {
-    EXCEPTION_RECORD *rec = setup_exception( HANDLER_CONTEXT, raise_trap_exception );
+    EXCEPTION_RECORD *rec;
 
     switch(get_trap_code(HANDLER_CONTEXT))
     {
+    case TRAP_x86_UNKNOWN:  /* TRAP_code not available */
+        rec = setup_exception( HANDLER_CONTEXT, raise_trap_exception );
+        break;
     case TRAP_x86_TRCTRAP:  /* Single-step exception */
-        rec->ExceptionCode = EXCEPTION_SINGLE_STEP;
+        rec = setup_exception( HANDLER_CONTEXT, raise_singlestep_exception );
         break;
     case TRAP_x86_BPTFLT:   /* Breakpoint exception */
-        rec->ExceptionAddress = (char *)rec->ExceptionAddress - 1;  /* back up over the int3 instruction */
-        /* fall through */
+        rec = setup_exception( HANDLER_CONTEXT, raise_breakpoint_exception );
+        break;
     default:
+        /* unexpected, so it will have been a user-generated signal */
+        rec = setup_exception( HANDLER_CONTEXT, raise_exception );
         rec->ExceptionCode = EXCEPTION_BREAKPOINT;
         break;
     }