Re: [ros-dev] [ros-diffs] [mjmartin] 47393: [win32k] - The timer is created usingUserCreateObject. It may be a good idea to save the handle in the timer object so that it can be deleted later. - Dereference the object before attempting to delete it.

[Ged Murphy]

There's no lock on the list access.

On 29 May 2010 07:51, [e-mail] wrote:
> Author: mjmartin
> Date: Sat May 29 08:51:03 2010
> New Revision: 47393
>
> URL: http://svn.reactos.org/svn/reactos?rev=47393&view=rev
> Log:
> [win32k]
> The timer is created usingUserCreateObject. It may be a good idea to save
>
> the handle in the timer object so that it can be deleted later.
> Dereference the object before attempting to delete it.
>
> Modified:
>    trunk/reactos/subsystems/win32/win32k/ntuser/timer.c
>
> Modified: trunk/reactos/subsystems/win32/win32k/ntuser/timer.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ntu...
>
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/ntuser/timer.c [iso-8859-1]
> (original)
> +++ trunk/reactos/subsystems/win32/win32k/ntuser/timer.c [iso-8859-1] Sat
> May 29 08:51:03 2010
> @@ -50,13 +50,21 @@
>   if (!FirstpTmr)
>   {
>       FirstpTmr = UserCreateObject(gHandleTable, NULL, &Handle, otTimer,
> sizeof(TIMER));
>  if (FirstpTmr) InitializeListHead(&FirstpTmr->ptmrList);
>  if (FirstpTmr)
>  {
>     FirstpTmr->head.h = Handle;
>     InitializeListHead(&FirstpTmr->ptmrList);
>  }
> Ret = FirstpTmr;
> }
> else
> {
>     Ret = UserCreateObject(gHandleTable, NULL, &Handle, otTimer,
>
> sizeof(TIMER));
>  if (Ret) InsertTailList(&FirstpTmr->ptmrList, &Ret->ptmrList);
>  if (Ret)
>  {
>     Ret->head.h = Handle;
>     InsertTailList(&FirstpTmr->ptmrList, &Ret->ptmrList);
>  }
> }
> return Ret;
>
> }
> @@ -66,14 +74,17 @@
>  FASTCALL
>  RemoveTimer(PTIMER pTmr)
>  {
> BOOL Ret = FALSE;
> if (pTmr)
> {
>   /* Set the flag, it will be removed when ready */
>   RemoveEntryList(&pTmr->ptmrList);
> UserDeleteObject( UserHMGetHandle(pTmr), otTimer);
> return TRUE;
> }
> return FALSE;
> UserDereferenceObject(pTmr);
> Ret = UserDeleteObject( UserHMGetHandle(pTmr), otTimer);
> }
> if (!Ret) DPRINT1("Warning unable to delete timer\n");
> return Ret;
>
> }
>
> PTIMER
> @@ -528,9 +539,7 @@
>    {
>       if ((pTmr) && (pTmr->pti == pti) && (pTmr->pWnd == Window))
>       {
>     RemoveEntryList(&pTmr->ptmrList);
>     UserDeleteObject( UserHMGetHandle(pTmr), otTimer);
>     TimersRemoved = TRUE;
>     TimersRemoved = RemoveTimer(pTmr);
> }
> pLE = pTmr->ptmrList.Flink;
> pTmr = CONTAINING_RECORD(pLE, TIMER, ptmrList);
>
> @@ -557,9 +566,7 @@
>    {
>       if ((pTmr) && (pTmr->pti == pti))
>       {
>     RemoveEntryList(&pTmr->ptmrList);
>     UserDeleteObject( UserHMGetHandle(pTmr), otTimer);
>     TimersRemoved = TRUE;
>     TimersRemoved = RemoveTimer(pTmr);
> }
> pLE = pTmr->ptmrList.Flink;
> pTmr = CONTAINING_RECORD(pLE, TIMER, ptmrList);

[James Tabor]

Locks?

On Sun, May 30, 2010 at 6:41 AM, Ged Murphy [e-mail] wrote:
> There's no lock on the list access.
>
> On 29 May 2010 07:51, [e-mail] wrote:
> > Author: mjmartin
> > Date: Sat May 29 08:51:03 2010
> > New Revision: 47393
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=47393&view=rev
> > Log:
> > [win32k]
> > The timer is created usingUserCreateObject. It may be a good idea to
> >
> > save the handle in the timer object so that it can be deleted later.
> > Dereference the object before attempting to delete it.
> >
> > Modified:
> >    trunk/reactos/subsystems/win32/win32k/ntuser/timer.c
> >   FirstpTmr = UserCreateObject(gHandleTable, NULL, &Handle, otTimer,
> >
> > sizeof(TIMER));
> >  if (FirstpTmr) InitializeListHead(&FirstpTmr->ptmrList);
> >  if (FirstpTmr)
> >  {
> >     FirstpTmr->head.h = Handle;
> >     InitializeListHead(&FirstpTmr->ptmrList);
> >  }
> > Ret = FirstpTmr;
> > }

UserCreateObject already sets the header handle and cLockObj!

from object.c, UserCreateObject:
{
<snip ,,....>
   /* Now set default headers. */
   ((PHEAD)Object)->h = hi;
   ((PHEAD)Object)->cLockObj = 2; // we need this, because we create 2
refs: handle and pointer!

if (h)
      *h = hi;
   return Object;
}

[Ged Murphy]

I've spoken to Mike about this, apparently the lock is held much higher in
the call chain (EnterUserExclusive), which isn't obvious in the patch.
Sounds like a pretty inefficient lock though.

On 30 May 2010 23:37, James Tabor [e-mail] wrote:
> Locks?

[Michael Martin]

Yes EnterUserExclusive is used alot elsewhere in win32k and we dont really need to use it with timers, so its inefficient.
I'm going to change this so that timers use thier own lock.

mike
Date: Sun, 30 May 2010 23:48:11 +0100
From: [e-mail]
To: [e-mail]
Subject: Re: [ros-dev] [ros-diffs] [mjmartin] 47393: [win32k] - The timer is	created usingUserCreateObject. It may be a good idea to save	the handle in the timer object so that it can be deleted	later. - Dereference the object before attempting to delete it.

I've spoken to Mike about this, apparently the lock is held much higher in the call chain (EnterUserExclusive), which isn't obvious in the patch.
Sounds like a pretty inefficient lock though.

On 30 May 2010 23:37, James Tabor [e-mail] wrote:

Locks?

_________________________________________________________________
The New Busy think 9 to 5 is a cute idea. Combine multiple calendars with Hotmail. 
http://www.windowslive.com/campaign/thenewbusy?tile=multicalendar&ocid=P...

[Timo Kreuzer]

Ged Murphy wrote:
> I've spoken to Mike about this, apparently the lock is held much
> higher in the call chain (EnterUserExclusive), which isn't obvious in
> the patch.
> Sounds like a pretty inefficient lock though.

Yes and exactly the way it's done in Windows. All USER code is guarded
by one huge lock.
For now it's fine, maybe once we get SMP ready, we can work out a better
solution.