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

[Piotr Caban]

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?
> -
>    {
> -
>        rec = (void*)rec->ExceptionInformation[4];
> -
> -
>        for (cur = data->frame_info_head; cur; cur = cur->next)
> -
>        {
> -
>            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:
> - 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?

Thanks,
Piotr

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

[Piotr Caban]

On 05/29/17 21:56, Piotr Caban 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?
> > -
> >    {
> > -
> >        rec = (void*)rec->ExceptionInformation[4];
> > -
> > -
> >        for (cur = data->frame_info_head; cur; cur = cur->next)
> > -
> >        {
> > -
> >            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:
> > - 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?

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(...) {}

Cheers,
Piotr