[mjmartin] 38699: - Last implementation was failing to charge the QuotaAvailable for the message length proceeding the message in the buffer, causing overwriting of the pool. See bug 4018 for more info.

Author: mjmartin
Date: Sun Jan 11 08:28:45 2009
New Revision: 38699

URL: http://svn.reactos.org/svn/reactos?rev=38699&view=rev
Log:
- Last implementation was failing to charge the QuotaAvailable for the message length
proceeding the message in the buffer, causing overwriting of the pool. See bug 4018 for
more info.

Modified:
    trunk/reactos/drivers/filesystems/npfs/rw.c

Modified: trunk/reactos/drivers/filesystems/npfs/rw.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs/r…
==============================================================================
--- trunk/reactos/drivers/filesystems/npfs/rw.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/filesystems/npfs/rw.c [iso-8859-1] Sun Jan 11 08:28:45 2009
@@ -438,6 +438,7 @@
 					break;
 				}
 				ExReleaseFastMutex(&Ccb->DataListLock);
+
 				if (IoIsOperationSynchronous(Irp))
 				{
 					/* Wait for ReadEvent to become signaled */
@@ -448,8 +449,7 @@
 						Irp->RequestorMode,
 						FALSE,
 						NULL);
-					DPRINT("Finished waiting (%wZ)! Status: %x\n",
&Ccb->Fcb->PipeName, Status);
-					ExAcquireFastMutex(&Ccb->DataListLock);
+					DPRINT("Finished waiting (%wZ)! Status: %x\n",
&Ccb->Fcb->PipeName, Status);					
 
 					if ((Status == STATUS_USER_APC) || (Status == STATUS_KERNEL_APC))
 					{
@@ -460,6 +460,7 @@
 					{
 						ASSERT(FALSE);
 					}
+					ExAcquireFastMutex(&Ccb->DataListLock);
 				}
 				else
 				{
@@ -526,26 +527,25 @@
 				DPRINT("Message mode\n");
 
 				/* For Message mode, the Message length will be stored in the buffer preceeding the
Message. */
-
 				if (Ccb->ReadDataAvailable)
 				{
 					ULONG NextMessageLength=0;
 					//HexDump(Ccb->Data, (ULONG)Ccb->WritePtr - (ULONG)Ccb->Data);
-
 					/*First get the size of the message */
 					memcpy(&NextMessageLength, Ccb->Data, sizeof(NextMessageLength));
-
-					if (NextMessageLength == 0) 
+					if ((NextMessageLength == 0) || (NextMessageLength > Ccb->ReadDataAvailable))
 					{
 						DPRINT1("This should never happen! Possible memory corruption.\n");
 #ifndef NDEBUG
 						HexDump(Ccb->Data, (ULONG)Ccb->WritePtr - (ULONG)Ccb->Data);
 #endif
-						break;
+						ASSERT(FALSE);
 					}
 
 					/* Use the smaller value */
+
 					CopyLength = min(NextMessageLength, Length);
+
 					/* retrieve the message from the buffer */
 					memcpy(Buffer, (PVOID)((ULONG)Ccb->Data + sizeof(NextMessageLength)),
CopyLength);
 
@@ -558,6 +558,7 @@
 
 							/* Calculate the remaining message new size */
 							ULONG NewMessageSize = NextMessageLength-CopyLength;
+
 							/* Write a new Message size to buffer for the part of the message still there */
 							memcpy(Ccb->Data, &NewMessageSize, sizeof(NewMessageSize));
 
@@ -568,10 +569,15 @@
 
 							/* Update the write pointer */
 							Ccb->WritePtr = (PVOID)((ULONG)Ccb->WritePtr - CopyLength);
+
+							/* Add CopyLength only to WriteQuotaAvailable as the 
+							   Message Size was replaced in the buffer */
+							Ccb->WriteQuotaAvailable += CopyLength;
 						}
 						else
 						{
 							/* Client wanted the entire message */
+
 							/* Move the memory starting from the next Message just retrieved */
 							memmove(Ccb->Data, 
 								(PVOID)((ULONG_PTR) Ccb->Data + NextMessageLength +
sizeof(NextMessageLength)),
@@ -579,14 +585,24 @@
 
 							/* Update the write pointer */
 							Ccb->WritePtr = (PVOID)((ULONG)Ccb->WritePtr - NextMessageLength);
+
+							/* Add both the message length and the header to the WriteQuotaAvailable
+							   as they both were removed */
+							Ccb->WriteQuotaAvailable += (CopyLength + sizeof(NextMessageLength));
 						}
 					}
 					else
 					{
 						/* This was the last Message, so just zero this messages for safety sake */
 						memset(Ccb->Data,0,NextMessageLength + sizeof(NextMessageLength));
-						/* reset the write pointer */
+
+						/* reset the write pointer to beginning of buffer */
 						Ccb->WritePtr = Ccb->Data;
+
+						/* Add both the message length and the header to the WriteQuotaAvailable
+						   as they both were removed */
+						Ccb->WriteQuotaAvailable += (CopyLength + sizeof(ULONG));
+
 						KeResetEvent(&Ccb->ReadEvent);
 						if (Ccb->PipeState == FILE_PIPE_CONNECTED_STATE)
 						{
@@ -599,8 +615,10 @@
 #endif
 
 					Information += CopyLength;
-					Ccb->WriteQuotaAvailable +=CopyLength;
+
 					Ccb->ReadDataAvailable -= CopyLength;
+
+					if ((ULONG)Ccb->WriteQuotaAvailable > (ULONG)Ccb->MaxDataLength)
ASSERT(FALSE);
 				}
 
 				if (Information > 0)
@@ -700,7 +718,7 @@
 
 	if (Irp->MdlAddress == NULL)
 	{
-		DPRINT("Irp->MdlAddress == NULL\n");
+		DPRINT1("Irp->MdlAddress == NULL\n");
 		Status = STATUS_UNSUCCESSFUL;
 		Length = 0;
 		goto done;
@@ -708,7 +726,7 @@
 
 	if (ReaderCcb == NULL)
 	{
-		DPRINT("Pipe is NOT connected!\n");
+		DPRINT1("Pipe is NOT connected!\n");
 		if (Ccb->PipeState == FILE_PIPE_LISTENING_STATE)
 			Status = STATUS_PIPE_LISTENING;
 		else if (Ccb->PipeState == FILE_PIPE_DISCONNECTED_STATE)
@@ -721,7 +739,7 @@
 
 	if (ReaderCcb->Data == NULL)
 	{
-		DPRINT("Pipe is NOT writable!\n");
+		DPRINT1("Pipe is NOT writable!\n");
 		Status = STATUS_UNSUCCESSFUL;
 		Length = 0;
 		goto done;
@@ -738,7 +756,7 @@
 
 	while(1)
 	{
-		if (ReaderCcb->WriteQuotaAvailable == 0)
+		if ((ReaderCcb->WriteQuotaAvailable == 0))
 		{
 			KeSetEvent(&ReaderCcb->ReadEvent, IO_NO_INCREMENT, FALSE);
 			if (Ccb->PipeState != FILE_PIPE_CONNECTED_STATE)
@@ -749,14 +767,14 @@
 			}
 			ExReleaseFastMutex(&ReaderCcb->DataListLock);
 
-			DPRINT("Waiting for buffer space (%S)\n", Fcb->PipeName.Buffer);
+			DPRINT("Write Waiting for buffer space (%S)\n", Fcb->PipeName.Buffer);
 			Status = KeWaitForSingleObject(&Ccb->WriteEvent,
 				UserRequest,
 				Irp->RequestorMode,
 				FALSE,
 				NULL);
-			DPRINT("Finished waiting (%S)! Status: %x\n", Fcb->PipeName.Buffer,
Status);
-
+			DPRINT("Write Finished waiting (%S)! Status: %x\n", Fcb->PipeName.Buffer,
Status);
+			ExAcquireFastMutex(&ReaderCcb->DataListLock);
 			if ((Status == STATUS_USER_APC) || (Status == STATUS_KERNEL_APC))
 			{
 				Status = STATUS_CANCELLED;
@@ -777,8 +795,6 @@
 				// ExReleaseFastMutex(&ReaderCcb->DataListLock);
 				goto done;
 			}
-
-			ExAcquireFastMutex(&ReaderCcb->DataListLock);
 		}
 
 		if (Fcb->WriteMode == FILE_PIPE_BYTE_STREAM_MODE)
@@ -825,21 +841,41 @@
 		{
 			/* For Message Type Pipe, the Pipes memory will be used to store the size of each
message */
                         /* FIXME: Check and verify ReadMode ByteStream */
-			DPRINT("Message mode\n");
+
 			if (Length > 0)
 			{
-				CopyLength = min(Length, ReaderCcb->WriteQuotaAvailable);
+				/* Verify the WritePtr is still inside the buffer */
+				if (((ULONG)ReaderCcb->WritePtr > ((ULONG)ReaderCcb->Data +
(ULONG)ReaderCcb->MaxDataLength)) ||
+				((ULONG)ReaderCcb->WritePtr < (ULONG)ReaderCcb->Data))
+				{
+					DPRINT1("NPFS is writing out of its buffer. Report to developer!\n");
+					DPRINT1("ReaderCcb->WritePtr %x, ReaderCcb->Data %x,
ReaderCcb->MaxDataLength%d\n",
+						ReaderCcb->WritePtr,ReaderCcb->Data,ReaderCcb->MaxDataLength);
+					ASSERT(FALSE);
+				}
+
+				CopyLength = min(Length, ReaderCcb->WriteQuotaAvailable - sizeof(ULONG));
+
 				/* First Copy the Length of the message into the pipes buffer */
 				memcpy(ReaderCcb->WritePtr, &CopyLength, sizeof(CopyLength));
+
 				/* Now the user buffer itself */
 				memcpy((PVOID)((ULONG)ReaderCcb->WritePtr+ sizeof(CopyLength)), Buffer,
CopyLength);
+
 				/* Update the write pointer */
 				ReaderCcb->WritePtr = (PVOID)((ULONG)ReaderCcb->WritePtr + sizeof(CopyLength) +
CopyLength);
 
 				Information += CopyLength;
 
 				ReaderCcb->ReadDataAvailable += CopyLength;
-				ReaderCcb->WriteQuotaAvailable -= CopyLength;
+
+				ReaderCcb->WriteQuotaAvailable -= (CopyLength + sizeof(ULONG));
+
+				if ((ULONG)ReaderCcb->WriteQuotaAvailable >
(ULONG)ReaderCcb->MaxDataLength)
+				{
+					DPRINT1("QuotaAvailable is greater than buffer size!\n");
+					ASSERT(FALSE);
+				}
 			}
 
 			if (Information > 0)