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(a)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;
> else
> - {
> - 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;
>