[ros-diffs] [ion] 60070: [NPFS-NEW]: Fix pool corruption and crashing bugs in NpPeek, which was using sizeof instead of FIELD_OFFSET. [NPFS-NEW]: Actually implement NpCancelWaiter instead of making it ASSERT. ...

Author: ion
Date: Fri Sep 13 07:49:42 2013
New Revision: 60070

URL: http://svn.reactos.org/svn/reactos?rev=60070&view=rev
Log:
[NPFS-NEW]: Fix pool corruption and crashing bugs in NpPeek, which was using sizeof
instead of FIELD_OFFSET.
[NPFS-NEW]: Actually implement NpCancelWaiter instead of making it ASSERT.
[NPFS-NEW]: Critical fixes to NpAddWaiter and NpWaitForNamedPipe to fix logic flaws.
NPFS-NEW now behaves without any visible regressions, and exhibits only 2 failures in the
kernel32 pipe winetest -- vs 120+ failures with the current NPFS driver. It has 0 ntdll
pipe failures, and 0 kmtest pipe failures.

Modified:
    trunk/reactos/drivers/filesystems/npfs_new/create.c
    trunk/reactos/drivers/filesystems/npfs_new/fsctrl.c
    trunk/reactos/drivers/filesystems/npfs_new/main.c
    trunk/reactos/drivers/filesystems/npfs_new/npfs.h
    trunk/reactos/drivers/filesystems/npfs_new/strucsup.c
    trunk/reactos/drivers/filesystems/npfs_new/waitsup.c

Modified: trunk/reactos/drivers/filesystems/npfs_new/create.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs_n…
==============================================================================
--- trunk/reactos/drivers/filesystems/npfs_new/create.c	[iso-8859-1] (original)
+++ trunk/reactos/drivers/filesystems/npfs_new/create.c	[iso-8859-1] Fri Sep 13 07:49:42
2013
@@ -551,7 +551,7 @@
                          Parameters->NamedPipeType & 0xFFFF,
                          &Fcb);
     if (!NT_SUCCESS(Status)) goto Quickie;
-    
+
     Status = NpCreateCcb(Fcb,
                          FileObject,
                          FILE_PIPE_LISTENING_STATE,

Modified: trunk/reactos/drivers/filesystems/npfs_new/fsctrl.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs_n…
==============================================================================
--- trunk/reactos/drivers/filesystems/npfs_new/fsctrl.c	[iso-8859-1] (original)
+++ trunk/reactos/drivers/filesystems/npfs_new/fsctrl.c	[iso-8859-1] Fri Sep 13 07:49:42
2013
@@ -206,7 +206,7 @@
 {
     PIO_STACK_LOCATION IoStack;
     NODE_TYPE_CODE Type;
-    ULONG InputLength;
+    ULONG OutputLength;
     ULONG NamedPipeEnd;
     PNP_CCB Ccb;
     PFILE_PIPE_PEEK_BUFFER PeekBuffer;
@@ -218,7 +218,7 @@
     PAGED_CODE();
 
     IoStack = IoGetCurrentIrpStackLocation(Irp);
-    InputLength = IoStack->Parameters.FileSystemControl.OutputBufferLength;
+    OutputLength = IoStack->Parameters.FileSystemControl.OutputBufferLength;
     Type = NpDecodeFileObject(IoStack->FileObject, NULL, &Ccb,
&NamedPipeEnd);
 
     if (!Type)
@@ -226,7 +226,8 @@
         return STATUS_PIPE_DISCONNECTED;
     }
 
-    if ((Type != NPFS_NTC_CCB) && (InputLength < sizeof(*PeekBuffer)))
+    if ((Type != NPFS_NTC_CCB) &&
+        (OutputLength < FIELD_OFFSET(FILE_PIPE_PEEK_BUFFER, Data)))
     {
         return STATUS_INVALID_PARAMETER;
     }
@@ -264,7 +265,7 @@
     PeekBuffer->NumberOfMessages = 0;
     PeekBuffer->MessageLength = 0;
     PeekBuffer->NamedPipeState = Ccb->NamedPipeState;
-    BytesPeeked = sizeof(*PeekBuffer);
+    BytesPeeked = FIELD_OFFSET(FILE_PIPE_PEEK_BUFFER, Data);
 
     if (DataQueue->QueueState == WriteEntries)
     {
@@ -279,7 +280,8 @@
             PeekBuffer->NumberOfMessages = DataQueue->EntriesInQueue;
             PeekBuffer->MessageLength = DataEntry->DataSize -
DataQueue->ByteOffset;
         }
-        if (InputLength == sizeof(*PeekBuffer))
+
+        if (OutputLength == FIELD_OFFSET(FILE_PIPE_PEEK_BUFFER, Data))
         {
             Status = PeekBuffer->ReadDataAvailable ? STATUS_BUFFER_OVERFLOW :
STATUS_SUCCESS;
         }
@@ -289,12 +291,12 @@
                                        TRUE,
                                        FALSE,
                                        PeekBuffer->Data,
-                                       InputLength - sizeof(*PeekBuffer),
+                                       OutputLength - FIELD_OFFSET(FILE_PIPE_PEEK_BUFFER,
Data),
                                        Ccb->Fcb->NamedPipeType ==
FILE_PIPE_MESSAGE_TYPE,
                                        Ccb,
                                        List);
             Status = IoStatus.Status;
-            BytesPeeked = IoStatus.Information + sizeof(*PeekBuffer);
+            BytesPeeked += IoStatus.Information;
         }
     }
     else
@@ -523,14 +525,18 @@
     NODE_TYPE_CODE NodeTypeCode;
     PLIST_ENTRY NextEntry;
     PNP_FCB Fcb;
+    PWCHAR OriginalBuffer;
     PAGED_CODE();
 
     IoStack = IoGetCurrentIrpStackLocation(Irp);
-    InLength = IoStack->Parameters.DeviceIoControl.InputBufferLength;
+    InLength = IoStack->Parameters.FileSystemControl.InputBufferLength;
 
     SourceString.Buffer = NULL;
 
-    if (NpDecodeFileObject(IoStack->FileObject, NULL, &Ccb, &NamedPipeEnd) !=
NPFS_NTC_ROOT_DCB)
+    if (NpDecodeFileObject(IoStack->FileObject,
+                           NULL,
+                           &Ccb,
+                           &NamedPipeEnd) != NPFS_NTC_ROOT_DCB)
     {
         Status = STATUS_ILLEGAL_FUNCTION;
         goto Quickie;
@@ -544,14 +550,17 @@
     }
 
     NameLength = Buffer->NameLength;
-    if ((NameLength > 0xFFFD) || ((NameLength + sizeof(*Buffer)) > InLength))
+    if ((NameLength > (0xFFFF - sizeof(UNICODE_NULL))) ||
+        ((NameLength + FIELD_OFFSET(FILE_PIPE_WAIT_FOR_BUFFER, Name)) > InLength))
     {
         Status = STATUS_INVALID_PARAMETER;
         goto Quickie;
     }
 
     SourceString.Length = (USHORT)NameLength + sizeof(OBJ_NAME_PATH_SEPARATOR);
-    SourceString.Buffer = ExAllocatePoolWithTag(PagedPool, SourceString.Length,
NPFS_WRITE_BLOCK_TAG);
+    SourceString.Buffer = ExAllocatePoolWithTag(PagedPool,
+                                                SourceString.Length,
+                                                NPFS_WRITE_BLOCK_TAG);
     if (!SourceString.Buffer)
     {
         Status = STATUS_INSUFFICIENT_RESOURCES;
@@ -562,6 +571,7 @@
     RtlCopyMemory(&SourceString.Buffer[1], Buffer->Name, Buffer->NameLength);
 
     Status = STATUS_SUCCESS;
+    OriginalBuffer = SourceString.Buffer;
     //Status = NpTranslateAlias(&SourceString);
     if (!NT_SUCCESS(Status)) goto Quickie;
 
@@ -583,7 +593,7 @@
         if (Ccb->NamedPipeState == FILE_PIPE_LISTENING_STATE) break;
     }
 
-    if (NextEntry == &Fcb->CcbList)
+    if (NextEntry != &Fcb->CcbList)
     {
         Status = STATUS_SUCCESS;
     }
@@ -592,7 +602,8 @@
         Status = NpAddWaiter(&NpVcb->WaitQueue,
                              Fcb->Timeout,
                              Irp,
-                             &SourceString);
+                             OriginalBuffer == SourceString.Buffer ?
+                             NULL : &SourceString);
     }
 
 Quickie:

Modified: trunk/reactos/drivers/filesystems/npfs_new/main.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs_n…
==============================================================================
--- trunk/reactos/drivers/filesystems/npfs_new/main.c	[iso-8859-1] (original)
+++ trunk/reactos/drivers/filesystems/npfs_new/main.c	[iso-8859-1] Fri Sep 13 07:49:42
2013
@@ -62,7 +62,7 @@
 
     DriverObject->DriverUnload = NULL;
 
-    RtlInitUnicodeString(&DeviceName, L"\\Device\\NamedPipe2");
+    RtlInitUnicodeString(&DeviceName, L"\\Device\\NamedPipe");
     Status = IoCreateDevice(DriverObject,
                             sizeof(NP_VCB),
                             &DeviceName,

Modified: trunk/reactos/drivers/filesystems/npfs_new/npfs.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs_n…
==============================================================================
--- trunk/reactos/drivers/filesystems/npfs_new/npfs.h	[iso-8859-1] (original)
+++ trunk/reactos/drivers/filesystems/npfs_new/npfs.h	[iso-8859-1] Fri Sep 13 07:49:42
2013
@@ -178,7 +178,7 @@
     KDPC Dpc;
     KTIMER Timer;
     PNP_WAIT_QUEUE WaitQueue;
-    UNICODE_STRING String;
+    UNICODE_STRING AliasName;
     PFILE_OBJECT FileObject;
 } NP_WAIT_QUEUE_ENTRY, *PNP_WAIT_QUEUE_ENTRY;
 
@@ -590,7 +590,7 @@
 NpAddWaiter(IN PNP_WAIT_QUEUE WaitQueue,
             IN LARGE_INTEGER WaitTime,
             IN PIRP Irp, 
-            IN PUNICODE_STRING Name);
+            IN PUNICODE_STRING AliasName);
 
 NTSTATUS
 NTAPI

Modified: trunk/reactos/drivers/filesystems/npfs_new/strucsup.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs_n…
==============================================================================
--- trunk/reactos/drivers/filesystems/npfs_new/strucsup.c	[iso-8859-1] (original)
+++ trunk/reactos/drivers/filesystems/npfs_new/strucsup.c	[iso-8859-1] Fri Sep 13 07:49:42
2013
@@ -66,7 +66,6 @@
             IN PLIST_ENTRY ListEntry)
 {
     PNP_DCB Dcb;
-
     PAGED_CODE();
 
     Dcb = Fcb->ParentDcb;
@@ -76,7 +75,7 @@
                    &Fcb->FullName,
                    STATUS_OBJECT_NAME_NOT_FOUND,
                    ListEntry);
-    
+
     RemoveEntryList(&Fcb->DcbEntry);
 
     if (Fcb->SecurityDescriptor)

Modified: trunk/reactos/drivers/filesystems/npfs_new/waitsup.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs_n…
==============================================================================
--- trunk/reactos/drivers/filesystems/npfs_new/waitsup.c	[iso-8859-1] (original)
+++ trunk/reactos/drivers/filesystems/npfs_new/waitsup.c	[iso-8859-1] Fri Sep 13 07:49:42
2013
@@ -45,7 +45,7 @@
 
     if (WaitEntry)
     {
-        ObfDereferenceObject(WaitEntry->FileObject);
+        ObDereferenceObject(WaitEntry->FileObject);
         ExFreePool(WaitEntry);
     }
 
@@ -66,23 +66,28 @@
     PNP_WAIT_QUEUE_ENTRY WaitEntry = Context;
 
     OldIrql = KfAcquireSpinLock(&WaitEntry->WaitQueue->WaitLock);
+
     Irp = WaitEntry->Irp;
     if (Irp)
     {
         RemoveEntryList(&Irp->Tail.Overlay.ListEntry);
+
         if (!IoSetCancelRoutine(Irp, NULL))
         {
             Irp->Tail.Overlay.DriverContext[1] = NULL;
             Irp = NULL;
         }
     }
+
     KfReleaseSpinLock(&WaitEntry->WaitQueue->WaitLock, OldIrql);
+
     if (Irp)
     {
         Irp->IoStatus.Status = STATUS_IO_TIMEOUT;
         IoCompleteRequest(Irp, IO_NAMED_PIPE_INCREMENT);
     }
-    ObfDereferenceObject(WaitEntry->FileObject);
+
+    ObDereferenceObject(WaitEntry->FileObject);
     ExFreePool(WaitEntry);
 }
 
@@ -99,13 +104,22 @@
 NpCancelWaiter(IN PNP_WAIT_QUEUE WaitQueue,
                IN PUNICODE_STRING PipeName,
                IN NTSTATUS Status,
-               IN PLIST_ENTRY ListEntry)
+               IN PLIST_ENTRY List)
 {
     UNICODE_STRING DestinationString;
     KIRQL OldIrql;
     PWCHAR Buffer;
-
-    Buffer = ExAllocatePoolWithTag(NonPagedPool, PipeName->Length,
NPFS_WAIT_BLOCK_TAG);
+    PLIST_ENTRY NextEntry;
+    PNP_WAIT_QUEUE_ENTRY WaitEntry, Linkage;
+    PIRP WaitIrp;
+    PFILE_PIPE_WAIT_FOR_BUFFER WaitBuffer;
+    ULONG i, NameLength;
+
+    Linkage = NULL;
+
+    Buffer = ExAllocatePoolWithTag(NonPagedPool,
+                                   PipeName->Length,
+                                   NPFS_WAIT_BLOCK_TAG);
     if (!Buffer) return STATUS_INSUFFICIENT_RESOURCES;
 
     RtlInitEmptyUnicodeString(&DestinationString, Buffer, PipeName->Length);
@@ -113,10 +127,78 @@
 
     OldIrql = KfAcquireSpinLock(&WaitQueue->WaitLock);
 
-    ASSERT(IsListEmpty(&WaitQueue->WaitList) == TRUE);
+    for (NextEntry = WaitQueue->WaitList.Flink;
+         NextEntry != &WaitQueue->WaitList;
+         NextEntry = NextEntry->Flink)
+    {
+        WaitIrp = CONTAINING_RECORD(NextEntry, IRP, Tail.Overlay.ListEntry);
+        WaitEntry = WaitIrp->Tail.Overlay.DriverContext[1];
+
+        if (WaitEntry->AliasName.Length)
+        {
+            ASSERT(FALSE);
+            if (DestinationString.Length == WaitEntry->AliasName.Length)
+            {
+                if (RtlCompareMemory(WaitEntry->AliasName.Buffer,
+                                     DestinationString.Buffer,
+                                     DestinationString.Length) ==
+                    DestinationString.Length)
+                {
+CancelWait:
+                    RemoveEntryList(&WaitIrp->Tail.Overlay.ListEntry);
+                    if (KeCancelTimer(&WaitEntry->Timer))
+                    {
+                        WaitEntry->WaitQueue = (PNP_WAIT_QUEUE)Linkage;
+                        Linkage = WaitEntry;
+                    }
+                    else
+                    {
+                        WaitEntry->Irp = NULL;
+                        WaitIrp->Tail.Overlay.DriverContext[1] = NULL;
+                    }
+
+                    if (IoSetCancelRoutine(WaitIrp, NULL))
+                    {
+                        WaitIrp->IoStatus.Information = 0;
+                        WaitIrp->IoStatus.Status = Status;
+                        InsertTailList(List, &WaitIrp->Tail.Overlay.ListEntry);
+                    }
+                    else
+                    {
+                        WaitIrp->Tail.Overlay.DriverContext[1] = NULL;
+                    }
+                }
+            }
+        }
+        else
+        {
+            WaitBuffer = WaitIrp->AssociatedIrp.SystemBuffer;
+
+            if (WaitBuffer->NameLength + sizeof(WCHAR) == DestinationString.Length)
+            {
+                NameLength = WaitBuffer->NameLength / sizeof(WCHAR);
+                for (i = 0; i < NameLength; i++)
+                {
+                    if (WaitBuffer->Name[i] != DestinationString.Buffer[i + 1])
break;
+                }
+
+                if (i >= NameLength) goto CancelWait;
+            }
+        }
+    }
 
     KfReleaseSpinLock(&WaitQueue->WaitLock, OldIrql);
+
     ExFreePool(DestinationString.Buffer);
+
+    while (Linkage)
+    {
+        WaitEntry = Linkage;
+        Linkage = (PNP_WAIT_QUEUE_ENTRY)Linkage->WaitQueue;
+        ObDereferenceObject(WaitEntry->FileObject);
+        ExFreePool(WaitEntry);
+    }
+
     return STATUS_SUCCESS;
 }
 
@@ -124,8 +206,8 @@
 NTAPI
 NpAddWaiter(IN PNP_WAIT_QUEUE WaitQueue,
             IN LARGE_INTEGER WaitTime,
-            IN PIRP Irp, 
-            IN PUNICODE_STRING Name)
+            IN PIRP Irp,
+            IN PUNICODE_STRING AliasName)
 {
     PIO_STACK_LOCATION IoStack;
     KIRQL OldIrql;
@@ -137,71 +219,74 @@
 
     IoStack = IoGetCurrentIrpStackLocation(Irp);
 
-    WaitEntry = ExAllocatePoolWithQuotaTag(NonPagedPool, sizeof(*WaitEntry),
NPFS_WRITE_BLOCK_TAG);
-    if (WaitEntry)
-    {
-        KeInitializeDpc(&WaitEntry->Dpc, NpTimerDispatch, WaitEntry);
-        KeInitializeTimer(&WaitEntry->Timer);
-
-        if (Name)
-        {
-            WaitEntry->String = *Name;
-        }
-        else
-        {
-            WaitEntry->String.Length = 0;
-            WaitEntry->String.Buffer = 0;
-        }
-
-        WaitEntry->WaitQueue = WaitQueue;
-        WaitEntry->Irp = Irp;
-
-        WaitBuffer = (PFILE_PIPE_WAIT_FOR_BUFFER)Irp->AssociatedIrp.SystemBuffer;
-        if (WaitBuffer->TimeoutSpecified)
-        {
-            DueTime = WaitBuffer->Timeout;
-        }
-        else
-        {
-            DueTime = WaitTime;
-        }
-
-        for (i = 0; i < WaitBuffer->NameLength / sizeof(WCHAR); i++)
-        {
-            WaitBuffer->Name[i] = RtlUpcaseUnicodeChar(WaitBuffer->Name[i]);
-        }
-
-        Irp->Tail.Overlay.DriverContext[0] = WaitQueue;
-        Irp->Tail.Overlay.DriverContext[1] = WaitEntry;
-        OldIrql = KfAcquireSpinLock(&WaitQueue->WaitLock);
-
-        IoSetCancelRoutine(Irp, NpCancelWaitQueueIrp);
-
-        if (Irp->Cancel && IoSetCancelRoutine(Irp, NULL))
-        {
-            Status = STATUS_CANCELLED;
-        }
-        else
-        {
-            InsertTailList(&WaitQueue->WaitList,
&Irp->Tail.Overlay.ListEntry);
-
-            IoMarkIrpPending(Irp);
-            Status = STATUS_PENDING;
-
-            WaitEntry->FileObject = IoStack->FileObject;
-            ObfReferenceObject(WaitEntry->FileObject);
-
-            KeSetTimer(&WaitEntry->Timer, DueTime, &WaitEntry->Dpc);
-            WaitEntry = NULL;
-
-        }
-        KfReleaseSpinLock(&WaitQueue->WaitLock, OldIrql);
-        if (WaitEntry) ExFreePool(WaitEntry);
+    WaitEntry = ExAllocatePoolWithQuotaTag(NonPagedPool,
+                                           sizeof(*WaitEntry),
+                                           NPFS_WRITE_BLOCK_TAG);
+    if (!WaitEntry)
+    {
+        return STATUS_INSUFFICIENT_RESOURCES;
+    }
+
+    KeInitializeDpc(&WaitEntry->Dpc, NpTimerDispatch, WaitEntry);
+    KeInitializeTimer(&WaitEntry->Timer);
+
+    if (AliasName)
+    {
+        WaitEntry->AliasName = *AliasName;
     }
     else
     {
-        Status = STATUS_INSUFFICIENT_RESOURCES;
-    }
+        WaitEntry->AliasName.Length = 0;
+        WaitEntry->AliasName.Buffer = NULL;
+    }
+
+    WaitEntry->WaitQueue = WaitQueue;
+    WaitEntry->Irp = Irp;
+
+    WaitBuffer = (PFILE_PIPE_WAIT_FOR_BUFFER)Irp->AssociatedIrp.SystemBuffer;
+    if (WaitBuffer->TimeoutSpecified)
+    {
+        DueTime = WaitBuffer->Timeout;
+    }
+    else
+    {
+        DueTime = WaitTime;
+    }
+
+    for (i = 0; i < WaitBuffer->NameLength / sizeof(WCHAR); i++)
+    {
+        WaitBuffer->Name[i] = RtlUpcaseUnicodeChar(WaitBuffer->Name[i]);
+    }
+
+    Irp->Tail.Overlay.DriverContext[0] = WaitQueue;
+    Irp->Tail.Overlay.DriverContext[1] = WaitEntry;
+
+    OldIrql = KfAcquireSpinLock(&WaitQueue->WaitLock);
+
+    IoSetCancelRoutine(Irp, NpCancelWaitQueueIrp);
+
+    if (Irp->Cancel && IoSetCancelRoutine(Irp, NULL))
+    {
+        Status = STATUS_CANCELLED;
+    }
+    else
+    {
+        InsertTailList(&WaitQueue->WaitList,
&Irp->Tail.Overlay.ListEntry);
+
+        IoMarkIrpPending(Irp);
+        Status = STATUS_PENDING;
+
+        WaitEntry->FileObject = IoStack->FileObject;
+        ObReferenceObject(WaitEntry->FileObject);
+
+        KeSetTimer(&WaitEntry->Timer, DueTime, &WaitEntry->Dpc);
+        WaitEntry = NULL;
+
+    }
+
+    KfReleaseSpinLock(&WaitQueue->WaitLock, OldIrql);
+    if (WaitEntry) ExFreePool(WaitEntry);
+
     return Status;
 }