RE: [PATCH 2/7] msvcrt: Clean up registered C++ object in handler.

[Daniel Lehman]

On 05/24/17 02:55, Daniel Lehman wrote:
> +static DWORD cxx_catch_cleanup(EXCEPTION_RECORD *rec, EXCEPTION_REGISTRATION_RECORD *frame,
> -
>                           CONTEXT *context, 
>
> +EXCEPTION_REGISTRATION_RECORD **pdispatcher) {
> - if (rec->ExceptionFlags & (EH_UNWINDING | EH_EXIT_UNWIND))
> - {
> -
>    thread_data_t *data = msvcrt_get_thread_data();
> -
>    frame_info *cur;
> -
> -
>    if (cxx_is_consolidate(rec))
>
> Is this condition really needed? Shouldn't we clean the object no matter what's the reason of unwind?

Yeah.  That's covered by patch 7/7.  The original code only cleaned up if consolidating.

Since this 2/7 patch was somewhat of a refactoring for the later patches, I kept the consolidate-only cleanup logic.  I can merge them if you want
> > -
> >            if ((ULONG64)cur <= (ULONG64)frame)
>
> This condition is not working. It's making assumption about order of catch_frame and frame_info variables on stack while they are declared this way:

I see what you mean.  If I forcefully reverse them, my tests crash;  my version of gcc was always putting them in the same place on the stack, regardless of where they were declared
> > - EXCEPTION_REGISTRATION_RECORD catch_frame;
> >  cxx_frame_info frame_info;
>
> Shouldn't the cxx_catch_cleanup just unregister the object that was registered in call_catch_block?

I didn't find a way to be call __CxxUnregisterObject on that one specifically (I'll try suggestions if you got em)

but the loop in cxx_catch_cleanup effective does this.  when unwinding a nested exception, cxx_catch_cleanup will be called when unwinding call_catch_block, so all objects registered in call_catch_block and below will be cleaned up at that point.  Unless the catch block itself explicitly calls __CxxRegisterObject, the only object on the list up to that frame will be the one from call_catch_block
> Here's a test case that demonstrate the problem with cur <= frame
> comparison:
>
> try {
>    	try { int *p = NULL; *p = 0x42; }
>    	catch (klass x) { throw 1; }
>    } catch (int i) { }
>
> try { throw 1; }
>    catch(...) {}

This crashes for me even on Windows because the SEGV is uncaught.  It 'works' if I set an seh translator that throws int, but I get identical results on Wine with my series applied.  Do I need to add something?

Thanks
daniel

[PATCH 2/7] msvcrt: Clean up registered C++ object in handler.

[Piotr Caban]

On 05/31/17 00:59, Daniel Lehman wrote:
> On 05/24/17 02:55, Daniel Lehman wrote:
> > +static DWORD cxx_catch_cleanup(EXCEPTION_RECORD *rec, EXCEPTION_REGISTRATION_RECORD *frame,
> > -
> >                           CONTEXT *context,
> >
> > +EXCEPTION_REGISTRATION_RECORD **pdispatcher) {
> > - if (rec->ExceptionFlags & (EH_UNWINDING | EH_EXIT_UNWIND))
> > - {
> > -
> >    thread_data_t *data = msvcrt_get_thread_data();
> > -
> >    frame_info *cur;
> > -
> > -
> >    if (cxx_is_consolidate(rec))
> >
> > Is this condition really needed? Shouldn't we clean the object no matter what's the reason of unwind?
>
> Yeah.  That's covered by patch 7/7.  The original code only cleaned up if consolidating.
>
> Since this 2/7 patch was somewhat of a refactoring for the later patches, I kept the consolidate-only cleanup logic.  I can merge them if you want

Yes, it would be preferable to merge the patches.
> > > -
> > >            if ((ULONG64)cur <= (ULONG64)frame)
> >
> > This condition is not working. It's making assumption about order of catch_frame and frame_info variables on stack while they are declared this way:
>
> I see what you mean.  If I forcefully reverse them, my tests crash;  my version of gcc was always putting them in the same place on the stack, regardless of where they were declared
> > > - EXCEPTION_REGISTRATION_RECORD catch_frame;
> > >   cxx_frame_info frame_info;
> >
> > Shouldn't the cxx_catch_cleanup just unregister the object that was registered in call_catch_block >
>
> I didn't find a way to be call __CxxUnregisterObject on that one specifically (I'll try suggestions if you got em)

I think that the easiest solution would be to define something like:
struct catch_cleanup_frame {
     EXCEPTION_REGISTRATION_RECORD frame;
     cxx_frame_info frame_info;
}
and just call __CxxUnregisterObject with passed 
catch_cleanup_frame->frame_info in handler.
> > Here's a test case that demonstrate the problem with cur <= frame
> > comparison:
> >
> > try {
> >    	try { int *p = NULL; *p = 0x42; }
> >    	catch (klass x) { throw 1; }
> >    } catch (int i) { }
> >
> > try { throw 1; }
> >    catch(...) {}
>
> This crashes for me even on Windows because the SEGV is uncaught.  It 'works' if I set an seh translator that throws int, but I get identical results on Wine with my series applied.  Do I need to add something?

It was meant to be run with seh translator set. It might be 
compiler/optimization level specific. On my computer it leads to 
incorrect stack usage caused by "cur <= frame" condition. The 
__CxxUnregisterObject is not called in my case and the registered 
objects list points to invalid stack space.

Thanks,
Piotr