[arty] 35581: Catch failure to release the cancel spinlock. Would've spotted my error in tcpip.sys.

Author: arty
Date: Sat Aug 23 18:04:15 2008
New Revision: 35581

URL: http://svn.reactos.org/svn/reactos?rev=35581&view=rev
Log:
Catch failure to release the cancel spinlock.  Would've spotted my error in
tcpip.sys.

Modified:
    trunk/reactos/ntoskrnl/io/iomgr/irp.c

Modified: trunk/reactos/ntoskrnl/io/iomgr/irp.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/irp.c?rev...
==============================================================================
--- trunk/reactos/ntoskrnl/io/iomgr/irp.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/io/iomgr/irp.c [iso-8859-1] Sat Aug 23 18:04:15 2008
@@ -970,12 +970,14 @@
 IoCancelIrp(IN PIRP Irp)
 {
     KIRQL OldIrql;
+    KIRQL IrqlAtEntry;
     PDRIVER_CANCEL CancelRoutine;
     IOTRACE(IO_IRP_DEBUG,
             "%s - Canceling IRP %p\n",
             __FUNCTION__,
             Irp);
     ASSERT(Irp->Type == IO_TYPE_IRP);
+    IrqlAtEntry = KeGetCurrentIrql();

/* Acquire the cancel lock and cancel the IRP */
     IoAcquireCancelSpinLock(&OldIrql);
@@ -999,6 +1001,7 @@
         /* Set the cancel IRQL And call the routine */
         Irp->CancelIrql = OldIrql;
         CancelRoutine(IoGetCurrentIrpStackLocation(Irp)->DeviceObject, Irp);
+	ASSERT(IrqlAtEntry == KeGetCurrentIrql());
         return TRUE;
     }

[Alex Ionescu]

--- Please, please, don't think I'm trying to say "revert this!" ---

There are dozens (if not more) places in the kernel where such checks  
could/should be made. I believe the more correct approach instead of  
adding them to every function, is to use a similar framework to  
Microsoft's Driver Verifier, where such functions are hooked and  
wrapped through similar verifications.

I'm not saying this particular assert should be removed, I'm just  
letting people know that if there's going to be major work on this --  
there's a better way.

Just my 2c.

On 23-Aug-08, at 4:04 PM, [e-mail] wrote:
> Author: arty
> Date: Sat Aug 23 18:04:15 2008
> New Revision: 35581
>
> URL: http://svn.reactos.org/svn/reactos?rev=35581&view=rev
> Log:
> Catch failure to release the cancel spinlock.  Would've spotted my  
> error in
> tcpip.sys.
>
> Modified:
>    trunk/reactos/ntoskrnl/io/iomgr/irp.c
>
> Modified: trunk/reactos/ntoskrnl/io/iomgr/irp.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/irp.c?rev...
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- trunk/reactos/ntoskrnl/io/iomgr/irp.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/io/iomgr/irp.c [iso-8859-1] Sat Aug 23  
> 18:04:15 2008
> @@ -970,12 +970,14 @@
> IoCancelIrp(IN PIRP Irp)
> {
>     KIRQL OldIrql;
>
> KIRQL IrqlAtEntry;
> PDRIVER_CANCEL CancelRoutine;
> IOTRACE(IO_IRP_DEBUG,
>        "%s - Canceling IRP %p\n",
>        __FUNCTION__,
>        Irp);
> ASSERT(Irp->Type == IO_TYPE_IRP);
>
> IrqlAtEntry = KeGetCurrentIrql();
>
> /* Acquire the cancel lock and cancel the IRP */
> IoAcquireCancelSpinLock(&OldIrql);
>
> @@ -999,6 +1001,7 @@
>         /* Set the cancel IRQL And call the routine */
>         Irp->CancelIrql = OldIrql;
>         CancelRoutine(IoGetCurrentIrpStackLocation(Irp)-
> > DeviceObject, Irp);
> ASSERT(IrqlAtEntry == KeGetCurrentIrql());
>    return TRUE;
> }

Best regards,
Alex Ionescu