Re: riched20: Avoid releasing a non-existent interface.

[Nikolay Sivov]

> -
>    This->txtSel->reOle = NULL;
> -
>    ITextSelection_Release(&This->txtSel->ITextSelection_iface);
> -
>    IOleClientSite_Release(&This->clientSite->IOleClientSite_iface);
> -
>    if(This->txtSel)
> -
>      {
> -
>        This->txtSel->reOle = NULL;
> -
>        ITextSelection_Release(&This->txtSel->ITextSelection_iface);
> -
>      }
> -
>    if(This->clientSite)
> -
>      {
> -
>        This->clientSite->reOle = NULL;
> -
>        IOleClientSite_Release(&This->clientSite->IOleClientSite_iface);
> -
>      }
>     heap_fr

This can't happen.
>   IOleClientSiteImpl *This = impl_from_IOleClientSite(iface);
>   ULONG ref = InterlockedDecrement(&This->ref);
>   if (ref == 0)
> -
>  {
> -
>    if(This->reOle)
> -
>      This->reOle->clientSite = NULL;
>     heap_free(This);
> -
>  }
> return ref;

Why do you need this?

riched20: Avoid releasing a non-existent interface.

[Jactry Zeng]

Hi Nikolay,

Thanks for your review!

The crash can be reproduced follow this:
- first release the ITextSelection or IOleClientSite interfaces completely;
- release ITextDocument interface;
- try to release the IRichEditOle (crash happen)

And this patch try to fix it.

(tests in attachment can reproduce the crash.)

2014-04-16 18:59 GMT+08:00 Nikolay Sivov :
> -
>    This->txtSel->reOle = NULL;
> > -
> >    ITextSelection_Release(&This->txtSel->ITextSelection_iface);
> > -
> >    IOleClientSite_Release(&This->clientSite->IOleClientSite_iface);
> > -
> >    if(This->txtSel)
> > -
> >      {
> > -
> >        This->txtSel->reOle = NULL;
> > -
> >        ITextSelection_Release(&This->txtSel->ITextSelection_iface);
> > -
> >      }
> > -
> >    if(This->clientSite)
> > -
> >      {
> > -
> >        This->clientSite->reOle = NULL;
> > -
> >        IOleClientSite_Release(&This->clientSite->IOleClientSite_
> >
> > iface);
> > -
> >      }
> >     heap_fr
>
> This can't happen.
>    IOleClientSiteImpl *This = impl_from_IOleClientSite(iface);
> >   ULONG ref = InterlockedDecrement(&This->ref);
> >   if (ref == 0)
> > -
> >  {
> > -
> >    if(This->reOle)
> > -
> >      This->reOle->clientSite = NULL;
> >     heap_free(This);
> > -
> >  }
> > return ref;
>
> Why do you need this?

riched20: Avoid releasing a non-existent interface.

[Nikolay Sivov]

On 4/16/2014 17:37, Jactry Zeng wrote:
> Hi Nikolay,
>
> Thanks for your review!
>
> The crash can be reproduced follow this:
> - first release the ITextSelection or IOleClientSite
>
> interfaces completely;
> - release ITextDocument interface;
> - try to release the IRichEditOle (crash happen)
>
> And this patch try to fix it.
> - /* hres = ITextDocument_GetSelection(txtDoc, &txtSel); */
> - /* ok(hres == S_OK, "ITextDocument_GetSelection\n"); */
> - /* while(ITextSelection_Release(txtSel)); */

What you're doing is a violation of refcount handling. The rule is to 
release what you got, without relying on internals like that. In this 
case GetSelection() returns interface pointer and you're responsible in 
exactly one Release() on it.

Interesting thing to test would be to check if GetSelection() returns 
new instance every time it's called. If this is a case it will justify 
some code changes to support this, right now patch is wrong.

If it actually returns same interface pointer you can't protect from 
use-after-free because I can grab multiple references with several 
GetSelection() calls, and when I'll try to release them it will be freed 
already by a loop like that.

riched20: Avoid releasing a non-existent interface.

[Jactry Zeng]

2014-04-17 11:54 GMT+08:00 Nikolay Sivov :
> What you're doing is a violation of refcount handling. The rule is to

release what you got, without relying on internals like that. In this case
GetSelection() returns interface pointer and you're responsible in exactly
one Release() on it.
> Interesting thing to test would be to check if GetSelection() returns new

instance every time it's called. If this is a case it will justify some
code changes to support this, right now patch is wrong.
> If it actually returns same interface pointer you can't protect from

use-after-free because I can grab multiple references with several
GetSelection() calls, and when I'll try to release them it will be freed
already by a loop like that.

Yes, you are right. GetSelection() will not return a new pointer every time
it's called, I have checked it by some tests on Windows.
I will try some other ways to fix this bug.

Thanks again!

riched20: Avoid releasing a non-existent interface.

[Nikolay Sivov]

On 4/17/2014 11:59, Jactry Zeng wrote:
> 2014-04-17 11:54 GMT+08:00 Nikolay Sivov <[e-mail] 
> >:
> > What you're doing is a violation of refcount handling. The rule is
>
> to release what you got, without relying on internals like that. In 
> this case GetSelection() returns interface pointer and you're 
> responsible in exactly one Release() on it.
> > Interesting thing to test would be to check if GetSelection()
>
> returns new instance every time it's called. If this is a case it will 
> justify some code changes to support this, right now patch is wrong.
> > If it actually returns same interface pointer you can't protect from
>
> use-after-free because I can grab multiple references with several 
> GetSelection() calls, and when I'll try to release them it will be 
> freed already by a loop like that.
>
> Yes, you are right. GetSelection() will not return a new pointer every 
> time it's called, I have checked it by some tests on Windows.

Did you actually change selection range in between of these calls? It's 
possible that new instance will be returned after selection changed. 
This way you'll end up with two alive instances.

Another thing to test if ITextDocument responds to IID_ITextSelection query.
> I will try some other ways to fix this bug.

First thing to do is to figure out what relations are between these 
interfaces.
> Thanks again!
>
> -- 
> Regards,
> Jactry Zeng

riched20: Avoid releasing a non-existent interface.

[Jactry Zeng]

2014-04-17 16:20 GMT+08:00 Nikolay Sivov :
> It's possible that new instance will be returned after selection changed.

This way you'll end up with two alive instances.
>

No. According these test:
https://github.com/Jactry/wine/commit/4e0c5e8f2f169560ef8d8d7ae0d0620b8ddcd1...
GetSelection will not return a new instance after selection changed.
> Another thing to test if ITextDocument responds to IID_ITextSelection

query.
> > I will try some other ways to fix this bug.
>
> First thing to do is to figure out what relations are between these

interfaces.

You are right, I need having more tests.