Re: [PATCH] ole32: LeaveCriticalSection in one exit case (Coverity)

[Dmitry Timoshkov]

Marcus Meissner  wrote:
> --- a/dlls/ole32/ifs.c
> +++ b/dlls/ole32/ifs.c
> @@ -217,6 +217,8 @@ static LPVOID WINAPI IMalloc_fnRealloc(LPMALLOC iface,LPVOID pv,DWORD cb) {
>            IMallocSpy_Release(Malloc32.pSpy);
>    	Malloc32.SpyReleasePending = FALSE;
>    	Malloc32.pSpy = NULL;
> -
> /* cb == 0 case will release it some lines below. */
> -
> if (cb) LeaveCriticalSection(&IMalloc32_SpyCS);
>
> }
>
> if (0==cb) {

Why not unconditionally release the lock before the '0==cb' check?

[PATCH] ole32: LeaveCriticalSection in one exit case (Coverity)

[Marcus Meissner]

On Sun, Jun 23, 2013 at 07:15:43PM +0900, Dmitry Timoshkov wrote:
> Marcus Meissner  wrote:
> > --- a/dlls/ole32/ifs.c
> > +++ b/dlls/ole32/ifs.c
> > @@ -217,6 +217,8 @@ static LPVOID WINAPI IMalloc_fnRealloc(LPMALLOC iface,LPVOID pv,DWORD cb) {
> >            IMallocSpy_Release(Malloc32.pSpy);
> >    	Malloc32.SpyReleasePending = FALSE;
> >    	Malloc32.pSpy = NULL;
> > -
> > /* cb == 0 case will release it some lines below. */
> > -
> > if (cb) LeaveCriticalSection(&IMalloc32_SpyCS);
> >
> > }
> >
> > if (0==cb) {
>
> Why not unconditionally release the lock before the '0==cb' check?

That would cause LeaveCriticalSection be called twice with a race window inbetween.

CIao, Marcus

[PATCH] ole32: LeaveCriticalSection in one exit case (Coverity)

[Dmitry Timoshkov]

Marcus Meissner  wrote:
> > > --- a/dlls/ole32/ifs.c
> > > +++ b/dlls/ole32/ifs.c
> > > @@ -217,6 +217,8 @@ static LPVOID WINAPI IMalloc_fnRealloc(LPMALLOC iface,LPVOID pv,DWORD cb) {
> > >            IMallocSpy_Release(Malloc32.pSpy);
> > >    	Malloc32.SpyReleasePending = FALSE;
> > >    	Malloc32.pSpy = NULL;
> > > -
> > > /* cb == 0 case will release it some lines below. */
> > > -
> > > if (cb) LeaveCriticalSection(&IMalloc32_SpyCS);
> > >
> > > }
> > >
> > > if (0==cb) {
> >
> > Why not unconditionally release the lock before the '0==cb' check?
>
> That would cause LeaveCriticalSection be called twice with a race window inbetween.

Of course you'd need to remove a redundant LeaveCriticalSection under
'if (0==cb)' case, and I don't see a race there.

[PATCH] ole32: LeaveCriticalSection in one exit case (Coverity)

[Marcus Meissner]

On Sun, Jun 23, 2013 at 08:43:18PM +0900, Dmitry Timoshkov wrote:
> Marcus Meissner  wrote:
> > > > --- a/dlls/ole32/ifs.c
> > > > +++ b/dlls/ole32/ifs.c
> > > > @@ -217,6 +217,8 @@ static LPVOID WINAPI IMalloc_fnRealloc(LPMALLOC iface,LPVOID pv,DWORD cb) {
> > > >            IMallocSpy_Release(Malloc32.pSpy);
> > > >    	Malloc32.SpyReleasePending = FALSE;
> > > >    	Malloc32.pSpy = NULL;
> > > > -
> > > > /* cb == 0 case will release it some lines below. */
> > > > -
> > > > if (cb) LeaveCriticalSection(&IMalloc32_SpyCS);
> > > >
> > > > }
> > > >
> > > > if (0==cb) {
> > >
> > > Why not unconditionally release the lock before the '0==cb' check?
> >
> > That would cause LeaveCriticalSection be called twice with a race window inbetween.
>
> Of course you'd need to remove a redundant LeaveCriticalSection under
> 'if (0==cb)' case, and I don't see a race there.

I resubmitted a slightly improved fix, but I do not think it can be made as simple as you think it can.

Ciao, Marcus