Re: [PATCH] ole32: check for interface NULL which happens with e.g. Abiword

[Dmitry Timoshkov]

Marcus Meissner <marcus(a)jet.franken.de> wrote:
> +  if (!unk) {
> +      FIXME("hr was %d, but unk is NULL?\n", hr);
> +      return E_FAIL;
> +  }

IDropTarget_QueryInterface() should be fixed instead.

-- 
Dmitry.

[PATCH] ole32: check for interface NULL which happens with e.g. Abiword

[Huw Davies]

On Wed, Sep 15, 2010 at 08:04:51PM +0900, Dmitry Timoshkov wrote:
> Marcus Meissner <marcus(a)jet.franken.de> wrote:
> > +  if (!unk) {
> > +      FIXME("hr was %d, but unk is NULL?\n", hr);
> > +      return E_FAIL;
> > +  }
>
> IDropTarget_QueryInterface() should be fixed instead.

The implementation is in abiword, so that would be tricky.

Marcus, you're leaking a stream in this error path.  Better
to do something like:
   if(FAILED(hr) || !unk)

Huw.

[PATCH] ole32: check for interface NULL which happens with e.g. Abiword

[Marcus Meissner]

On Wed, Sep 15, 2010 at 12:22:57PM +0100, Huw Davies wrote:
> On Wed, Sep 15, 2010 at 08:04:51PM +0900, Dmitry Timoshkov wrote:
> > Marcus Meissner <marcus(a)jet.franken.de> wrote:
> > > +  if (!unk) {
> > > +      FIXME("hr was %d, but unk is NULL?\n", hr);
> > > +      return E_FAIL;
> > > +  }
> >
> > IDropTarget_QueryInterface() should be fixed instead.
>
> The implementation is in abiword, so that would be tricky.
>
> Marcus, you're leaking a stream in this error path.  Better
> to do something like:
>    if(FAILED(hr) || !unk)

yeah, but what error to return :/

Ciao, Marcus

[PATCH] ole32: check for interface NULL which happens with e.g. Abiword

[Huw Davies]

On Wed, Sep 15, 2010 at 01:34:06PM +0200, Marcus Meissner wrote:
> On Wed, Sep 15, 2010 at 12:22:57PM +0100, Huw Davies wrote:
> > On Wed, Sep 15, 2010 at 08:04:51PM +0900, Dmitry Timoshkov wrote:
> > > Marcus Meissner <marcus(a)jet.franken.de> wrote:
> > > > +  if (!unk) {
> > > > +      FIXME("hr was %d, but unk is NULL?\n", hr);
> > > > +      return E_FAIL;
> > > > +  }
> > >
> > > IDropTarget_QueryInterface() should be fixed instead.
> >
> > The implementation is in abiword, so that would be tricky.
> >
> > Marcus, you're leaking a stream in this error path.  Better
> > to do something like:
> >    if(FAILED(hr) || !unk)
>
> yeah, but what error to return :/

Yeah, sorry.  Something like:
    if(!unk) hr = E_NOINTERFACE;
right after the QI call.

Huw.

[PATCH] ole32: check for interface NULL which happens with e.g. Abiword

[Marcus Meissner]

On Wed, Sep 15, 2010 at 12:40:46PM +0100, Huw Davies wrote:
> On Wed, Sep 15, 2010 at 01:34:06PM +0200, Marcus Meissner wrote:
> > On Wed, Sep 15, 2010 at 12:22:57PM +0100, Huw Davies wrote:
> > > On Wed, Sep 15, 2010 at 08:04:51PM +0900, Dmitry Timoshkov wrote:
> > > > Marcus Meissner <marcus(a)jet.franken.de> wrote:
> > > > > +  if (!unk) {
> > > > > +      FIXME("hr was %d, but unk is NULL?\n", hr);
> > > > > +      return E_FAIL;
> > > > > +  }
> > > >
> > > > IDropTarget_QueryInterface() should be fixed instead.
> > >
> > > The implementation is in abiword, so that would be tricky.
> > >
> > > Marcus, you're leaking a stream in this error path.  Better
> > > to do something like:
> > >    if(FAILED(hr) || !unk)
> >
> > yeah, but what error to return :/
>
> Yeah, sorry.  Something like:
>     if(!unk) hr = E_NOINTERFACE;
> right after the QI call.

I ran testbot, https://testbot.winehq.org/JobDetails.pl?Key=5239
and it also has 0 as hr for the case.

So returning failure might not be right approach.

Abiword has this code btw:
	STDMETHODIMP XAP_Win32DropTarget::QueryInterface(REFIID /*riid*/, LPVOID FAR* /*ppvObj*/)
	{
		return S_OK;
	}
The fun...

Would erroring out be right?
If an interface directly inherits IUnknown, do we need a QueryInterface or could we just cast?

Ciao, Marcus

[PATCH] ole32: check for interface NULL which happens with e.g. Abiword

[Marcus Meissner]

On Wed, Sep 15, 2010 at 08:04:51PM +0900, Dmitry Timoshkov wrote:
> Marcus Meissner <marcus(a)jet.franken.de> wrote:
> > +  if (!unk) {
> > +      FIXME("hr was %d, but unk is NULL?\n", hr);
> > +      return E_FAIL;
> > +  }
>
> IDropTarget_QueryInterface() should be fixed instead.

I am not sure where where the DropTarget object comes from.

If it comes out of the marshaler, then its too much magic ;)

IRC identified in the meantime:

commit 6d1ef3a6a64f0fabf05ce1bba5f0ec4373684786
13:15 < igorko> Author: Huw Davies <huw(a)codeweavers.com>
13:15 < igorko> Date:   Thu Jul 22 13:37:19 2010 +0100
13:15 < igorko>     ole32: Implement cross-process drag and drop.
13:16 < igorko> :040000 040000 ab88cc6bf0936bc4adc7bff0673415282613d919
13:16 < igorko> 065c727204a46d01708bc01f10484cad8527e1a2 M    dlls

Ciao, Marcus