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_ne... ============================================================================== --- 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_ne... ============================================================================== --- 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_ne... ============================================================================== --- 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_ne... ============================================================================== --- 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_ne... ============================================================================== --- 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_ne... ============================================================================== --- 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; }