RE: Correction to crash inside RtlCaptureStackBackTrace() + test case

[Janne Hakonen]

> It doesn't need a try-except block, just better wrap-around handling.

Ah, you mean to enhance the if-check?

Well..., to me it seems that there is a problem with the 2nd check: 
"(void *)(frame + 1) >= NtCurrentTeb()->Tib.StackBase"

If frame is 0xffffffff or close to it (e.g. 0xfffffffc), adding 1 to its address makes it overflow and the left side becomes 0x3, or 0x0 in 0xfffffffc's case. And that would make the check fail.

I would propose that this would be corrected with an additional overflow check.
 
There are two 4 byte values that needs to be read: next frame address and return address. 
So, it would be checked that address of last byte of return address (frame + 7 bytes) is not smaller than current frame address, right?

The if-check would then be as follows:

if (((void *)frame < NtCurrentTeb()->Tib.StackLimit) ||
            ((void *)(frame + 1) >= NtCurrentTeb()->Tib.StackBase) ||
            ((ULONG_PTR)frame & 3) ||
            ((ULONG)((char *)frame + 7) < (ULONG)frame))
    ...

I'm not quite sure if casting pointers to ULONG for comparison is wise move, but can't think anything else for that.
 
Do you think there is any problems with this approach?


Other alternative could be to just simply check if the frame pointer equals to 0xfffffffc, that, combined with "(ULONG_PTR)frame & 3" should handle all cases near 0xffffffff.

Best Regards,
Janne

Correction to crash inside RtlCaptureStackBackTrace() + test case

[Janne Hakonen]

Ah, I see you had already corrected the stack overflowing, and found better way even. :) 

I tested the correction with the test cases and they passed. 
I also tried AwesomiumGL and I couldn't get it to crash, so looks to me that this bug is fixed.

The test cases at http://source.winehq.org/patches/ are still in pending state. Should I improve them somehow, or leave them as they are?

Thanks for help!
Janne

Correction to crash inside RtlCaptureStackBackTrace() + test case

[Alexandre Julliard]

Janne Hakonen <joyer83(a)live.fi> writes:
> The test cases at http://source.winehq.org/patches/ are still in
> pending state. Should I improve them somehow, or leave them as they
> are?

I'm not sure that there's a good way of testing this. You can't use
exception handlers in tests, and you can't assume that ebp is pointing
to the current frame either, unless you do the whole function in
assembly.

-- 
Alexandre Julliard
julliard(a)winehq.org

Correction to crash inside RtlCaptureStackBackTrace() + test case

[Janne Hakonen]

> You can't use exception handlers in tests
> I'm not sure why not, but I'll take you word of this.

Without exception handling the test cases for RtlCaptureStackBackTrace would also always crash on Win 2000/NT so that makes all of the tests useless, I think.

Well, at least I learned how to (/ not to) write test cases. :)

Thanks,
Janne