[PATCH 6/6] d3dx9: Secure against unsafe iface to COM object transitions

[Michael Stefaniuc]

Signed-off-by: Michael Stefaniuc 
---
 dlls/d3dx9_36/effect.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
index 8152362170..2443c20a63 100644
--- a/dlls/d3dx9_36/effect.c
+++ b/dlls/d3dx9_36/effect.c
@@ -1763,6 +1763,8 @@ static inline struct d3dx_effect_pool *impl_from_ID3DXEffectPool(ID3DXEffectPool
     return CONTAINING_RECORD(iface, struct d3dx_effect_pool, ID3DXEffectPool_iface);
 }

+static inline struct d3dx_effect_pool *unsafe_impl_from_ID3DXEffectPool(ID3DXEffectPool *iface);
+
 static inline struct d3dx_effect *impl_from_ID3DXEffect(ID3DXEffect *iface)
 {
     return CONTAINING_RECORD(iface, struct d3dx_effect, ID3DXEffect_iface);
@@ -6162,7 +6164,7 @@ static HRESULT d3dx9_effect_init(struct d3dx_effect *effect, struct IDirect3DDev
     if (pool)
     {
         pool->lpVtbl->AddRef(pool);
-        effect->pool = impl_from_ID3DXEffectPool(pool);
+        effect->pool = unsafe_impl_from_ID3DXEffectPool(pool);
     }

IDirect3DDevice9_AddRef(device);
@@ -6474,6 +6476,14 @@ static const struct ID3DXEffectPoolVtbl ID3DXEffectPool_Vtbl =
     d3dx_effect_pool_Release
 };

+static inline struct d3dx_effect_pool *unsafe_impl_from_ID3DXEffectPool(ID3DXEffectPool *iface)
+{
+    if (!iface || iface->lpVtbl != &ID3DXEffectPool_Vtbl)
+        return NULL;
+
+    return impl_from_ID3DXEffectPool(iface);
+}
+
 HRESULT WINAPI D3DXCreateEffectPool(ID3DXEffectPool **pool)
 {
     struct d3dx_effect_pool *object;

[Chip Davis]

March 22, 2019 2:49 AM, "Michael Stefaniuc"  wrote:
> @@ -6162,7 +6164,7 @@ static HRESULT d3dx9_effect_init(struct d3dx_effect *effect, struct IDirect3DDev
>      if (pool)
>      {
>          pool->lpVtbl->AddRef(pool);
> -
>    effect->pool = impl_from_ID3DXEffectPool(pool);
> -
>    effect->pool = unsafe_impl_from_ID3DXEffectPool(pool);
> }

Now you are leaking 'pool' in case it's not one of ours.

Chip

[Paul Gofman]

On 3/22/19 16:37, Chip Davis wrote:
> March 22, 2019 2:49 AM, "Michael Stefaniuc"  wrote:
> > @@ -6162,7 +6164,7 @@ static HRESULT d3dx9_effect_init(struct d3dx_effect *effect, struct IDirect3DDev
> >       if (pool)
> >       {
> >           pool->lpVtbl->AddRef(pool);
> > -
> >    effect->pool = impl_from_ID3DXEffectPool(pool);
> > -
> >    effect->pool = unsafe_impl_from_ID3DXEffectPool(pool);
> > }
>
> Now you are leaking 'pool' in case it's not one of ours.
>
> Chip

This can't really work if the pool is not one of ours. Pool does not 
have a public interface to work with from the effect's side. We don't 
have any tests showing what native d3dx9 will do in such a case: crash 
(I would put my bet on this before testing), return an error or act if 
no pool is provided, or even use the pool pointer just like a hash value 
and work fine with any object claimed to be a pool??. IMO unless we want 
to add some tests for this case having NULL from 
unsafe_impl_from_ID3DXEffectPool() deserves a FIXME at the first place. 
Is silently acting as if there is no pool better than crashing or 
leaking something pretending to be a pool object, unless we are sure 
this is a right thing to do?

[Paul Gofman]

On 3/22/19 17:25, Paul Gofman wrote:
> On 3/22/19 16:37, Chip Davis wrote:
> > March 22, 2019 2:49 AM, "Michael Stefaniuc"  wrote:
> > > @@ -6162,7 +6164,7 @@ static HRESULT d3dx9_effect_init(struct 
> > > d3dx_effect *effect, struct IDirect3DDev
> > >       if (pool)
> > >       {
> > >           pool->lpVtbl->AddRef(pool);
> > > -        effect->pool = impl_from_ID3DXEffectPool(pool);
> > > +        effect->pool = unsafe_impl_from_ID3DXEffectPool(pool);
> > >       }
> >
> > Now you are leaking 'pool' in case it's not one of ours.
> >
> > Chip
>
> This can't really work if the pool is not one of ours. Pool does not 
> have a public interface to work with from the effect's side. We don't 
> have any tests showing what native d3dx9 will do in such a case: crash 
> (I would put my bet on this before testing), return an error or act if 
> no pool is provided, or even use the pool pointer just like a hash 
> value and work fine with any object claimed to be a pool??. IMO unless 
> we want to add some tests for this case having NULL from 
> unsafe_impl_from_ID3DXEffectPool() deserves a FIXME at the first 
> place. Is silently acting as if there is no pool better than crashing 
> or leaking something pretending to be a pool object, unless we are 
> sure this is a right thing to do?

FWIW I just made a quick test setting pool->lpVtbl to NULL in pool 
before using this pool in D3DXCreateEffect, and native d3dx9_36.dll 
crashes on read access to address 0x4, just the same way as builtin 
currently does.