[cgutman] 57175: [AFD] - Fix one of the worst bugs in AFD of all time. Our AFD code operated under the assumption that none of the input parameters would change once we called LockRequest. This ass...
Author: cgutman Date: Mon Aug 27 04:16:28 2012 New Revision: 57175
URL: http://svn.reactos.org/svn/reactos?rev=57175&view=rev Log: [AFD] - Fix one of the worst bugs in AFD of all time. Our AFD code operated under the assumption that none of the input parameters would change once we called LockRequest. This assumption is completely false. The only guarantee that made was that the pages never disappeared from us, not they they couldn't be modified. There are frequent cases where the user-mode buffer was modified from underneath us (WSPRecv allocates the struct on stack which makes it invalid during overlapped operations that complete later). When this happened, we would bugcheck when we tried to unlock the buffers since we accessed this in a member of the struct the caller passed us. - I've fixed this by adding a parameter to LockRequest which specifies whether the buffer should be copied back when it is unlocked. - This bug has been around for ages and I was never able to figure out why we just freed garbage sometimes. Now that the ws2_32_winetest exposed it reliably, I was finally able to fix it.
Modified: trunk/reactos/drivers/network/afd/afd/bind.c trunk/reactos/drivers/network/afd/afd/connect.c trunk/reactos/drivers/network/afd/afd/context.c trunk/reactos/drivers/network/afd/afd/info.c trunk/reactos/drivers/network/afd/afd/listen.c trunk/reactos/drivers/network/afd/afd/lock.c trunk/reactos/drivers/network/afd/afd/main.c trunk/reactos/drivers/network/afd/afd/read.c trunk/reactos/drivers/network/afd/afd/select.c trunk/reactos/drivers/network/afd/afd/write.c trunk/reactos/drivers/network/afd/include/afd.h
Modified: trunk/reactos/drivers/network/afd/afd/bind.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/bin... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/bind.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/bind.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -81,7 +81,7 @@ AFD_DbgPrint(MID_TRACE,("Called\n"));
if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp ); - if( !(BindReq = LockRequest( Irp, IrpSp )) ) + if( !(BindReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 );
Modified: trunk/reactos/drivers/network/afd/afd/connect.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/con... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/connect.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/connect.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -44,7 +44,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID ConnectOptions = LockRequest(Irp, IrpSp); + PVOID ConnectOptions = LockRequest(Irp, IrpSp, FALSE); UINT ConnectOptionsSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -80,7 +80,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT ConnectOptionsSize = LockRequest(Irp, IrpSp); + PUINT ConnectOptionsSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -144,7 +144,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID ConnectData = LockRequest(Irp, IrpSp); + PVOID ConnectData = LockRequest(Irp, IrpSp, FALSE); UINT ConnectDataSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -179,7 +179,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT ConnectDataSize = LockRequest(Irp, IrpSp); + PUINT ConnectDataSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -406,7 +406,7 @@ AFD_DbgPrint(MID_TRACE,("Called on %x\n", FCB));
if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp ); - if( !(ConnectReq = LockRequest( Irp, IrpSp )) ) + if( !(ConnectReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 );
Modified: trunk/reactos/drivers/network/afd/afd/context.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/con... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/context.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/context.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -60,7 +60,7 @@ PIO_STACK_LOCATION IrpSp ) { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID Context = LockRequest(Irp, IrpSp); + PVOID Context = LockRequest(Irp, IrpSp, FALSE);
if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp );
Modified: trunk/reactos/drivers/network/afd/afd/info.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/inf... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/info.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/info.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -13,7 +13,7 @@ AfdGetInfo( PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp ) { NTSTATUS Status = STATUS_SUCCESS; - PAFD_INFO InfoReq = LockRequest(Irp, IrpSp); + PAFD_INFO InfoReq = LockRequest(Irp, IrpSp, TRUE); PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; PLIST_ENTRY CurrentEntry; @@ -99,7 +99,7 @@ AfdSetInfo( PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp ) { NTSTATUS Status = STATUS_SUCCESS; - PAFD_INFO InfoReq = LockRequest(Irp, IrpSp); + PAFD_INFO InfoReq = LockRequest(Irp, IrpSp, FALSE); PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; PCHAR NewBuffer;
Modified: trunk/reactos/drivers/network/afd/afd/listen.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/lis... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/listen.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/listen.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -217,7 +217,7 @@
if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp );
- if( !(ListenReq = LockRequest( Irp, IrpSp )) ) + if( !(ListenReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 );
Modified: trunk/reactos/drivers/network/afd/afd/lock.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/loc... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/lock.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/lock.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -12,12 +12,13 @@ PVOID GetLockedData(PIRP Irp, PIO_STACK_LOCATION IrpSp) { ASSERT(Irp->MdlAddress); - - return MmGetSystemAddressForMdlSafe(Irp->MdlAddress, NormalPagePriority); + ASSERT(Irp->Tail.Overlay.DriverContext[0]); + + return Irp->Tail.Overlay.DriverContext[0]; }
/* Lock a method_neither request so it'll be available from DISPATCH_LEVEL */ -PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ) { +PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp, BOOLEAN Output ) { BOOLEAN LockFailed = FALSE;
ASSERT(!Irp->MdlAddress); @@ -84,12 +85,55 @@ return NULL; }
+ /* The mapped address goes in index 1 */ + Irp->Tail.Overlay.DriverContext[1] = MmGetSystemAddressForMdlSafe(Irp->MdlAddress, NormalPagePriority); + if (!Irp->Tail.Overlay.DriverContext[1]) + { + AFD_DbgPrint(MIN_TRACE,("Failed to get mapped address\n")); + MmUnlockPages(Irp->MdlAddress); + IoFreeMdl( Irp->MdlAddress ); + Irp->MdlAddress = NULL; + return NULL; + } + + /* The allocated address goes in index 0 */ + Irp->Tail.Overlay.DriverContext[0] = ExAllocatePool(NonPagedPool, MmGetMdlByteCount(Irp->MdlAddress)); + if (!Irp->Tail.Overlay.DriverContext[0]) + { + AFD_DbgPrint(MIN_TRACE,("Failed to allocate memory\n")); + MmUnlockPages(Irp->MdlAddress); + IoFreeMdl( Irp->MdlAddress ); + Irp->MdlAddress = NULL; + return NULL; + } + + RtlCopyMemory(Irp->Tail.Overlay.DriverContext[0], + Irp->Tail.Overlay.DriverContext[1], + MmGetMdlByteCount(Irp->MdlAddress)); + + /* If we don't want a copy back, we zero the mapped address pointer */ + if (!Output) + { + Irp->Tail.Overlay.DriverContext[1] = NULL; + } + return GetLockedData(Irp, IrpSp); }
VOID UnlockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ) { ASSERT(Irp->MdlAddress); + ASSERT(Irp->Tail.Overlay.DriverContext[0]); + + /* Check if we need to copy stuff back */ + if (Irp->Tail.Overlay.DriverContext[1] != NULL) + { + RtlCopyMemory(Irp->Tail.Overlay.DriverContext[1], + Irp->Tail.Overlay.DriverContext[0], + MmGetMdlByteCount(Irp->MdlAddress)); + } + + ExFreePool(Irp->Tail.Overlay.DriverContext[0]); MmUnlockPages( Irp->MdlAddress ); IoFreeMdl( Irp->MdlAddress ); Irp->MdlAddress = NULL;
Modified: trunk/reactos/drivers/network/afd/afd/main.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/mai... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/main.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/main.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -72,7 +72,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID DisconnectOptions = LockRequest(Irp, IrpSp); + PVOID DisconnectOptions = LockRequest(Irp, IrpSp, FALSE); UINT DisconnectOptionsSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -108,7 +108,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT DisconnectOptionsSize = LockRequest(Irp, IrpSp); + PUINT DisconnectOptionsSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -172,7 +172,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID DisconnectData = LockRequest(Irp, IrpSp); + PVOID DisconnectData = LockRequest(Irp, IrpSp, FALSE); UINT DisconnectDataSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -208,7 +208,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT DisconnectDataSize = LockRequest(Irp, IrpSp); + PUINT DisconnectDataSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -244,7 +244,7 @@ { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PULONG HandleFlags = LockRequest(Irp, IrpSp); + PULONG HandleFlags = LockRequest(Irp, IrpSp, TRUE); PAFD_TDI_HANDLE_DATA HandleData = Irp->UserBuffer;
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -680,7 +680,7 @@
if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp);
- if (!(DisReq = LockRequest(Irp, IrpSp))) + if (!(DisReq = LockRequest(Irp, IrpSp, FALSE))) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 );
Modified: trunk/reactos/drivers/network/afd/afd/read.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/rea... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/read.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/read.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -445,7 +445,7 @@ Irp, 0 ); }
- if( !(RecvReq = LockRequest( Irp, IrpSp )) ) + if( !(RecvReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 );
@@ -715,7 +715,7 @@ return UnlockAndMaybeComplete(FCB, STATUS_FILE_CLOSED, Irp, 0); }
- if( !(RecvReq = LockRequest( Irp, IrpSp )) ) + if( !(RecvReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete(FCB, STATUS_NO_MEMORY, Irp, 0);
AFD_DbgPrint(MID_TRACE,("Recv flags %x\n", RecvReq->AfdFlags));
Modified: trunk/reactos/drivers/network/afd/afd/select.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/sel... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/select.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/select.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -259,7 +259,7 @@ PFILE_OBJECT FileObject = IrpSp->FileObject; NTSTATUS Status = STATUS_NO_MEMORY; PAFD_EVENT_SELECT_INFO EventSelectInfo = - (PAFD_EVENT_SELECT_INFO)LockRequest( Irp, IrpSp ); + (PAFD_EVENT_SELECT_INFO)LockRequest( Irp, IrpSp, FALSE ); PAFD_FCB FCB = FileObject->FsContext;
if( !SocketAcquireStateLock( FCB ) ) { @@ -322,7 +322,7 @@ PIO_STACK_LOCATION IrpSp ) { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_ENUM_NETWORK_EVENTS_INFO EnumReq = - (PAFD_ENUM_NETWORK_EVENTS_INFO)LockRequest( Irp, IrpSp ); + (PAFD_ENUM_NETWORK_EVENTS_INFO)LockRequest( Irp, IrpSp, TRUE ); PAFD_FCB FCB = FileObject->FsContext;
AFD_DbgPrint(MID_TRACE,("Called (FCB %x)\n", FCB));
Modified: trunk/reactos/drivers/network/afd/afd/write.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/wri... ============================================================================== --- trunk/reactos/drivers/network/afd/afd/write.c [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/afd/write.c [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -299,7 +299,7 @@ 0 ); }
- if( !(SendReq = LockRequest( Irp, IrpSp )) ) + if( !(SendReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 );
/* Must lock buffers before handing off user data */ @@ -369,7 +369,7 @@ return UnlockAndMaybeComplete(FCB, STATUS_FILE_CLOSED, Irp, 0); }
- if( !(SendReq = LockRequest( Irp, IrpSp )) ) + if( !(SendReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete ( FCB, STATUS_NO_MEMORY, Irp, TotalBytesCopied );
@@ -516,7 +516,7 @@ return UnlockAndMaybeComplete(FCB, STATUS_FILE_CLOSED, Irp, 0); }
- if( !(SendReq = LockRequest( Irp, IrpSp )) ) + if( !(SendReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete(FCB, STATUS_NO_MEMORY, Irp, 0);
if (FCB->State == SOCKET_STATE_CREATED)
Modified: trunk/reactos/drivers/network/afd/include/afd.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/include... ============================================================================== --- trunk/reactos/drivers/network/afd/include/afd.h [iso-8859-1] (original) +++ trunk/reactos/drivers/network/afd/include/afd.h [iso-8859-1] Mon Aug 27 04:16:28 2012 @@ -308,7 +308,7 @@ NTSTATUS LostSocket( PIRP Irp ); PAFD_HANDLE LockHandles( PAFD_HANDLE HandleArray, UINT HandleCount ); VOID UnlockHandles( PAFD_HANDLE HandleArray, UINT HandleCount ); -PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ); +PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp, BOOLEAN Output ); VOID UnlockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ); PVOID GetLockedData( PIRP Irp, PIO_STACK_LOCATION IrpSp ); NTSTATUS LeaveIrpUntilLater( PAFD_FCB FCB, PIRP Irp, UINT Function );
-
cgutman＠svn.reactos.org