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

[Sebastian Lackner]

On 16.02.2016 10:43, Henri Verbeet wrote:
> 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.

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.
>