Re: [PATCH] ddraw: Avoid break in __TRY blocks.

[Sebastian Lackner]

On 14.02.2017 22:56, Stefan Dösinger wrote:
> This fixes compilation on Visual Studio with native compiler exceptions.
>
> Signed-off-by: Stefan Dösinger

While you are just at it, please note that this code also contains a leak
when the exception is triggered. Actually, it would probably be easier to
test if the pointer is writeable in advance, and exit early without creating
the surface at all.
> dlls/ddraw/ddraw.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/ddraw/ddraw.c b/dlls/ddraw/ddraw.c
> index 254dae7..54f3817 100644
> --- a/dlls/ddraw/ddraw.c
> +++ b/dlls/ddraw/ddraw.c
> @@ -2836,11 +2836,13 @@ static HRESULT WINAPI ddraw7_CreateSurface(IDirectDraw7 *iface, DDSURFACEDESC2 *
>          if (FAILED(hr))
>          {
>              *surface = NULL;
> -
>        break;
>    }
> -
>    *surface = &impl->IDirectDrawSurface7_iface;
> -
>    IDirectDraw7_AddRef(iface);
> -
>    impl->ifaceToRelease = (IUnknown *)iface;
> -
>    else
> -
>    {
> -
>        *surface = &impl->IDirectDrawSurface7_iface;
> -
>        IDirectDraw7_AddRef(iface);
> -
>        impl->ifaceToRelease = (IUnknown *)iface;
> -
>    }
> }
>    __EXCEPT_PAGE_FAULT
>    {
>
> @@ -2898,11 +2900,13 @@ static HRESULT WINAPI ddraw4_CreateSurface(IDirectDraw4 *iface,
>          if (FAILED(hr))
>          {
>              *surface = NULL;
> -
>        break;
>    }
> -
>    *surface = &impl->IDirectDrawSurface4_iface;
> -
>    IDirectDraw4_AddRef(iface);
> -
>    impl->ifaceToRelease = (IUnknown *)iface;
> -
>    else
> -
>    {
> -
>        *surface = &impl->IDirectDrawSurface4_iface;
> -
>        IDirectDraw4_AddRef(iface);
> -
>        impl->ifaceToRelease = (IUnknown *)iface;
> -
>    }
> }
>    __EXCEPT_PAGE_FAULT
>    {
>
> @@ -2962,10 +2966,12 @@ static HRESULT WINAPI ddraw2_CreateSurface(IDirectDraw2 *iface,
>          if (FAILED(hr))
>          {
>              *surface = NULL;
> -
>        break;
>    }
> -
>    *surface = &impl->IDirectDrawSurface_iface;
> -
>    impl->ifaceToRelease = NULL;
> -
>    else
> -
>    {
> -
>        *surface = &impl->IDirectDrawSurface_iface;
> -
>        impl->ifaceToRelease = NULL;
> -
>    }
> }
>    __EXCEPT_PAGE_FAULT
>    {
>
> @@ -3022,10 +3028,12 @@ static HRESULT WINAPI ddraw1_CreateSurface(IDirectDraw *iface,
>          if (FAILED(hr))
>          {
>              *surface = NULL;
> -
>        break;
>    }
> -
>    *surface = &impl->IDirectDrawSurface_iface;
> -
>    impl->ifaceToRelease = NULL;
> -
>    else
> -
>    {
> -
>        *surface = &impl->IDirectDrawSurface_iface;
> -
>        impl->ifaceToRelease = NULL;
> -
>    }
> }
>    __EXCEPT_PAGE_FAULT
>    {

[PATCH] ddraw: Avoid break in __TRY blocks.

[Stefan Dösinger]

> Am 14.02.2017 um 22:01 schrieb Sebastian Lackner :
>
> While you are just at it, please note that this code also contains a leak
> when the exception is triggered. Actually, it would probably be easier to
> test if the pointer is writeable in advance, and exit early without creating
> the surface at all.

I wouldn’t be the least bit surprised if Windows leaked in this situation as well. But I guess for the test it is relevant for Valgrind runs.

A pre-emptive

__TRY
{
    *surface = NULL;
}
__EXCEPT_PAGE_FAULT
{
    return E_INVALIDARG;
}

before doing any work would be nicer, if it doesn’t have any unexpected side effects (e.g. Windows might keep *surface alone in some error conditions, which wouldn’t surprise me either).

Alex, can you look into fixing the leak?

[PATCH] ddraw: Avoid break in __TRY blocks.

[Alex Henrie]

2017-02-14 15:15 GMT-07:00 Stefan Dösinger :
> > Am 14.02.2017 um 22:01 schrieb Sebastian Lackner :
> >
> > While you are just at it, please note that this code also contains a leak
> > when the exception is triggered. Actually, it would probably be easier to
> > test if the pointer is writeable in advance, and exit early without creating
> > the surface at all.
>
> I wouldn’t be the least bit surprised if Windows leaked in this situation as well. But I guess for the test it is relevant for Valgrind runs.
>
> A pre-emptive
>
> __TRY
> {
>     *surface = NULL;
> }
> __EXCEPT_PAGE_FAULT
> {
>     return E_INVALIDARG;
> }
>
> before doing any work would be nicer, if it doesn’t have any unexpected side effects (e.g. Windows might keep *surface alone in some error conditions, which wouldn’t surprise me either).
>
> Alex, can you look into fixing the leak?

Yes, I'll look into it. Sorry for not noticing the leak before.

-Alex

[PATCH] ddraw: Avoid break in __TRY blocks.

[Stefan Dösinger]

> Am 14.02.2017 um 22:29 schrieb Alex Henrie :
>
> Yes, I'll look into it. Sorry for not noticing the leak before.

Please port the test to ddraw2-7 as well, and in ddraw4 and ddraw7 test the IDirectDraw4/7 refcount after the create call that fails because of the invalid pointer. Windows might leak a reference. (If it does, I don’t think we want to upstream the test, but document this behaviour in a comment)

What happens if you have a create failure like DDERR_NOCOOPERATIVELEVELSET and an invalid pointer?

[PATCH] ddraw: Avoid break in __TRY blocks.

[Alex Henrie]

I've now run numerous tests on DirectDraw.

The following conditions do not result in an attempted write to the
surface pointer:
- Null surface descriptor (returns E_INVALIDARG)
- Wrong surface descriptor size (returns E_INVALIDARG)
- Unset cooperative level (returns DDERR_NOCOOPERATIVELEVELSET)

The following conditions do result in an attempted write to the surface pointer:
- Tried to create explicit frontbuffer (returns DDERR_INVALIDCAPS if
NULL was written to the surface pointer and E_INVALIDARG if the write
failed)
- Requested surface with zero width or zero height (returns E_INVALIDARG)

None of the above conditions increments the IDirectDraw7 refcount.

So, we can try to write NULL to the surface pointer before the
explicit frontbuffer/backbuffer and zero width/height checks, and that
should give us behavior that is indistinguishable from Windows.

-Alex