[PATCH 5/5] ntdll: Add special handling for int $0x2d exceptions.

[Henri Verbeet]

On 12 February 2016 at 22:40, Sebastian Lackner <sebastian at 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.