Thanks for working on this. I assume you already saw the new test failures, those should be fixed of course. Besides that I have some more remarks on the patch:
On 12.02.2016 18:52, Henri Verbeet wrote:
Signed-off-by: Henri Verbeet hverbeet@codeweavers.com
dlls/ntdll/signal_i386.c | 14 +++++++++++++- dlls/ntdll/tests/exception.c | 38 +++++++++++++++++++++++--------------- 2 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c index a3abbff..46f932a 100644 --- a/dlls/ntdll/signal_i386.c +++ b/dlls/ntdll/signal_i386.c @@ -2072,8 +2072,20 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) case TRAP_x86_PROTFLT: /* General protection fault */ case TRAP_x86_UNKNOWN: /* Unknown fault code */ {
CONTEXT *win_context = get_exception_context( rec ); WORD err = get_error_code(context);
if (!err && (rec->ExceptionCode = is_privileged_instr( get_exception_context(rec) ))) break;
if (!err && (rec->ExceptionCode = is_privileged_instr( win_context ))) break;if ((err & 7) == 2 && (err >> 3) == 0x2d) /* int $0x2d */{
Int 0x2d is not just a special way to raise a breakpoint, its actually a kernel interface to send commands to the debugger. EAX contains the command value, other registers contain the arguments. An proper implementation is not necessary I guess, but at least for small values of EAX which are assigned to commands we would need better test coverage. Most anti- debugger tricks set EAX = 0 which is not tested at all so far.
win_context->Eip += 3;rec->ExceptionCode = EXCEPTION_BREAKPOINT;rec->ExceptionAddress = (char *)win_context->Eip;rec->NumberParameters = is_wow64 ? 1 : 3;rec->ExceptionInformation[0] = EAX_sig(context);rec->ExceptionInformation[1] = ECX_sig(context);rec->ExceptionInformation[2] = EDX_sig(context);break;} rec->ExceptionCode = EXCEPTION_ACCESS_VIOLATION; rec->NumberParameters = 2; rec->ExceptionInformation[0] = 0;diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index d6a610e..57b201a 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -219,6 +219,11 @@ static const struct exception
{ { 0xf1, 0x90, 0xc3 }, /* icebp; nop; ret */ 1, 1, FALSE, STATUS_SINGLE_STEP, 0 },
- { { 0xb8, 0xb8, 0xb8, 0xb8, 0xb8, /* mov $0xb8b8b8b8, %eax */
0xb9, 0xb9, 0xb9, 0xb9, 0xb9, /* mov $0xb9b9b9b9, %ecx */0xba, 0xba, 0xba, 0xba, 0xba, /* mov $0xbabababa, %edx */0xcd, 0x2d, 0xc3 }, /* int $0x2d; ret */17, 0, FALSE, STATUS_BREAKPOINT, 3, { 0xb8b8b8b8, 0xb9b9b9b9, 0xbabababa } },};
Could you maybe also provide a testcase when a debugger is attached? I would expect that it behaves different then. Actually it should not even produce an exception then because the exception is transparently translated into a debug event, like wine already does for debug strings for example.
static int got_exception; @@ -473,7 +478,7 @@ static DWORD handler( EXCEPTION_RECORD *rec, EXCEPTION_REGISTRATION_RECORD *fram CONTEXT *context, EXCEPTION_REGISTRATION_RECORD **dispatcher ) { const struct exception *except = *(const struct exception **)(frame + 1);
- unsigned int i, entry = except - exceptions;
unsigned int i, parameter_count, entry = except - exceptions;
got_exception++; trace( "exception %u: %x flags:%x addr:%p\n",
@@ -482,20 +487,23 @@ static DWORD handler( EXCEPTION_RECORD *rec, EXCEPTION_REGISTRATION_RECORD *fram ok( rec->ExceptionCode == except->status || (except->alt_status != 0 && rec->ExceptionCode == except->alt_status), "%u: Wrong exception code %x/%x\n", entry, rec->ExceptionCode, except->status );
- ok( rec->ExceptionAddress == (char*)code_mem + except->offset,
"%u: Wrong exception address %p/%p\n", entry,rec->ExceptionAddress, (char*)code_mem + except->offset );- if (except->alt_status == 0 || rec->ExceptionCode != except->alt_status)
- {
ok( rec->NumberParameters == except->nb_params,"%u: Wrong number of parameters %u/%u\n", entry, rec->NumberParameters, except->nb_params );- }
- ok( context->Eip == (DWORD_PTR)code_mem + except->offset,
"%u: Unexpected eip %#x/%#lx\n", entry,context->Eip, (DWORD_PTR)code_mem + except->offset );- ok( rec->ExceptionAddress == (char*)context->Eip ||
(rec->ExceptionCode == STATUS_BREAKPOINT && rec->ExceptionAddress == (char*)context->Eip + 1),
I would prefer an exact test for ExceptionAddress here, off-by-one errors in exception handling would be kinda odd.
"%u: Unexpected exception address %p/%p\n", entry,rec->ExceptionAddress, (char*)context->Eip );- if (except->status == STATUS_BREAKPOINT && is_wow64)
parameter_count = 1;- else if (except->alt_status == 0 || rec->ExceptionCode != except->alt_status)
parameter_count = except->nb_params;
- {
ok( rec->NumberParameters == except->alt_nb_params,"%u: Wrong number of parameters %u/%u\n", entry, rec->NumberParameters, except->nb_params );- }
parameter_count = except->alt_nb_params;ok( rec->NumberParameters == parameter_count,
"%u: Unexpected parameter count %u/%u\n", entry, rec->NumberParameters, parameter_count );/* Most CPUs (except Intel Core apparently) report a segment limit violation */ /* instead of page faults for accesses beyond 0xffffffff */
@@ -530,7 +538,7 @@ static DWORD handler( EXCEPTION_RECORD *rec, EXCEPTION_REGISTRATION_RECORD *fram
skip_params: /* don't handle exception if it's not the address we expected */
- if (rec->ExceptionAddress != (char*)code_mem + except->offset) return ExceptionContinueSearch;
if (context->Eip != (DWORD_PTR)code_mem + except->offset) return ExceptionContinueSearch;
context->Eip += except->length; return ExceptionContinueExecution;
On 12 February 2016 at 22:40, Sebastian Lackner sebastian@fds-team.de wrote:
Thanks for working on this. I assume you already saw the new test failures, those should be fixed of course. Besides that I have some more remarks on the patch:
Thank you for trying to review patches; doing it well takes real effort. As you noted yourself in https://www.winehq.org/pipermail/wine-devel/2016-February/111682.html, the icebp and and fpu exception failures are existing failures.
Int 0x2d is not just a special way to raise a breakpoint, its actually a kernel interface to send commands to the debugger.
Yes, but that statement is somewhat misleading. Most of the special behaviour of int $0x2d only happens when Windows is in kernel debug mode. Wine makes no attempt to implement kernel debug mode.
EAX contains the command value, other registers contain the arguments. An proper implementation is not necessary I guess, but at least for small values of EAX which are assigned to commands we would need better test coverage. Most anti- debugger tricks set EAX = 0 which is not tested at all so far.
win_context->Eip += 3;rec->ExceptionCode = EXCEPTION_BREAKPOINT;rec->ExceptionAddress = (char *)win_context->Eip;rec->NumberParameters = is_wow64 ? 1 : 3;rec->ExceptionInformation[0] = EAX_sig(context);rec->ExceptionInformation[1] = ECX_sig(context);rec->ExceptionInformation[2] = EDX_sig(context);break;} rec->ExceptionCode = EXCEPTION_ACCESS_VIOLATION; rec->NumberParameters = 2; rec->ExceptionInformation[0] = 0;diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index d6a610e..57b201a 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -219,6 +219,11 @@ static const struct exception
{ { 0xf1, 0x90, 0xc3 }, /* icebp; nop; ret */ 1, 1, FALSE, STATUS_SINGLE_STEP, 0 },
- { { 0xb8, 0xb8, 0xb8, 0xb8, 0xb8, /* mov $0xb8b8b8b8, %eax */
0xb9, 0xb9, 0xb9, 0xb9, 0xb9, /* mov $0xb9b9b9b9, %ecx */0xba, 0xba, 0xba, 0xba, 0xba, /* mov $0xbabababa, %edx */0xcd, 0x2d, 0xc3 }, /* int $0x2d; ret */17, 0, FALSE, STATUS_BREAKPOINT, 3, { 0xb8b8b8b8, 0xb9b9b9b9, 0xbabababa } },};
Could you maybe also provide a testcase when a debugger is attached? I would expect that it behaves different then. Actually it should not even produce an exception then because the exception is transparently translated into a debug event, like wine already does for debug strings for example.
I did of course test the behaviour with a debugger attached, although mostly to validate the %eip and exception address adjustments. The behaviour is consistent with what's in this patch. Different values of %eax (e.g. 0-5) don't seem to have any obvious effect outside of kernel debug mode.
- ok( rec->ExceptionAddress == (char*)context->Eip ||
(rec->ExceptionCode == STATUS_BREAKPOINT && rec->ExceptionAddress == (char*)context->Eip + 1),I would prefer an exact test for ExceptionAddress here, off-by-one errors in exception handling would be kinda odd.
Well, yes, but as you can see from the test results, the behaviour is not consistent between different Windows versions.
On 16.02.2016 10:43, Henri Verbeet wrote:
On 12 February 2016 at 22:40, Sebastian Lackner sebastian@fds-team.de wrote:
Thanks for working on this. I assume you already saw the new test failures, those should be fixed of course. Besides that I have some more remarks on the patch:
Thank you for trying to review patches; doing it well takes real effort. As you noted yourself in https://www.winehq.org/pipermail/wine-devel/2016-February/111682.html, the icebp and and fpu exception failures are existing failures.
Sorry, I didn't recognize them as the usual failures because they were renamed by the patch. That doesn't invalidate the other remarks of my review though.
Int 0x2d is not just a special way to raise a breakpoint, its actually a kernel interface to send commands to the debugger.
Yes, but that statement is somewhat misleading. Most of the special behaviour of int $0x2d only happens when Windows is in kernel debug mode. Wine makes no attempt to implement kernel debug mode.
Actually my tests confirms that it works in user mode. The only blocker is wow64, which is not really surprising. Kernel mode seems to assume that valid values are stored in 64-bit registers, which is not the case, so a breakpoint exception is raised. Under normal circumstances calling a valid debug service (for example EAX=1 with valid arguments in other registers) raises no exception at all.
EAX contains the command value, other registers contain the arguments. An proper implementation is not necessary I guess, but at least for small values of EAX which are assigned to commands we would need better test coverage. Most anti- debugger tricks set EAX = 0 which is not tested at all so far.
win_context->Eip += 3;rec->ExceptionCode = EXCEPTION_BREAKPOINT;rec->ExceptionAddress = (char *)win_context->Eip;rec->NumberParameters = is_wow64 ? 1 : 3;rec->ExceptionInformation[0] = EAX_sig(context);rec->ExceptionInformation[1] = ECX_sig(context);rec->ExceptionInformation[2] = EDX_sig(context);break;} rec->ExceptionCode = EXCEPTION_ACCESS_VIOLATION; rec->NumberParameters = 2; rec->ExceptionInformation[0] = 0;diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index d6a610e..57b201a 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -219,6 +219,11 @@ static const struct exception
{ { 0xf1, 0x90, 0xc3 }, /* icebp; nop; ret */ 1, 1, FALSE, STATUS_SINGLE_STEP, 0 },
- { { 0xb8, 0xb8, 0xb8, 0xb8, 0xb8, /* mov $0xb8b8b8b8, %eax */
0xb9, 0xb9, 0xb9, 0xb9, 0xb9, /* mov $0xb9b9b9b9, %ecx */0xba, 0xba, 0xba, 0xba, 0xba, /* mov $0xbabababa, %edx */0xcd, 0x2d, 0xc3 }, /* int $0x2d; ret */17, 0, FALSE, STATUS_BREAKPOINT, 3, { 0xb8b8b8b8, 0xb9b9b9b9, 0xbabababa } },};
Could you maybe also provide a testcase when a debugger is attached? I would expect that it behaves different then. Actually it should not even produce an exception then because the exception is transparently translated into a debug event, like wine already does for debug strings for example.
I did of course test the behaviour with a debugger attached, although mostly to validate the %eip and exception address adjustments. The behaviour is consistent with what's in this patch. Different values of %eax (e.g. 0-5) don't seem to have any obvious effect outside of kernel debug mode.
On some systems it makes a difference, and causes a different ExceptionAddress value when a debugger is attached. However, the behaviour is not really consistent.
- ok( rec->ExceptionAddress == (char*)context->Eip ||
(rec->ExceptionCode == STATUS_BREAKPOINT && rec->ExceptionAddress == (char*)context->Eip + 1),I would prefer an exact test for ExceptionAddress here, off-by-one errors in exception handling would be kinda odd.
Well, yes, but as you can see from the test results, the behaviour is not consistent between different Windows versions.
On 16 February 2016 at 13:12, Sebastian Lackner sebastian@fds-team.de wrote:
Actually my tests confirms that it works in user mode. The only blocker is wow64, which is not really surprising.
I did test on a WoW64, yes, Windows 7 specifically. I think WoW64 is common enough these days that it would need a specific application that depends on the non-WoW64 behaviour to add code for handling that case.
-
Henri Verbeet -
Sebastian Lackner