-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Ensure HttpListener request buffer is aligned as required by the host processor #25563
Changes from 7 commits
4f18b04
03a3424
d0fd8b8
7305eba
6e0465e
fd0667f
3ae9b26
6c21edb
3f90e6f
8ad7f1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -786,75 +786,70 @@ internal static unsafe string GetVerb(HTTP_REQUEST* request) | |
return GetVerb(request, 0); | ||
} | ||
|
||
internal static unsafe string GetVerb(byte[] memoryBlob, IntPtr originalAddress) | ||
internal static unsafe string GetVerb(IntPtr memoryBlob, IntPtr originalAddress) | ||
{ | ||
fixed (byte* pMemoryBlob = memoryBlob) | ||
{ | ||
return GetVerb((HTTP_REQUEST*)pMemoryBlob, pMemoryBlob - (byte*)originalAddress); | ||
} | ||
return GetVerb((HTTP_REQUEST*)memoryBlob.ToPointer(), (byte*)memoryBlob - (byte*)originalAddress); | ||
} | ||
|
||
// Server API | ||
|
||
internal static unsafe WebHeaderCollection GetHeaders(byte[] memoryBlob, IntPtr originalAddress) | ||
internal static unsafe WebHeaderCollection GetHeaders(IntPtr memoryBlob, IntPtr originalAddress) | ||
{ | ||
NetEventSource.Enter(null); | ||
|
||
// Return value. | ||
WebHeaderCollection headerCollection = new WebHeaderCollection(); | ||
fixed (byte* pMemoryBlob = memoryBlob) | ||
{ | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
long fixup = pMemoryBlob - (byte*)originalAddress; | ||
int index; | ||
byte* pMemoryBlob = (byte*)memoryBlob; | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
long fixup = pMemoryBlob - (byte*)originalAddress; | ||
int index; | ||
|
||
// unknown headers | ||
if (request->Headers.UnknownHeaderCount != 0) | ||
// unknown headers | ||
if (request->Headers.UnknownHeaderCount != 0) | ||
{ | ||
HTTP_UNKNOWN_HEADER* pUnknownHeader = (HTTP_UNKNOWN_HEADER*)(fixup + (byte*)request->Headers.pUnknownHeaders); | ||
for (index = 0; index < request->Headers.UnknownHeaderCount; index++) | ||
{ | ||
HTTP_UNKNOWN_HEADER* pUnknownHeader = (HTTP_UNKNOWN_HEADER*)(fixup + (byte*)request->Headers.pUnknownHeaders); | ||
for (index = 0; index < request->Headers.UnknownHeaderCount; index++) | ||
// For unknown headers, when header value is empty, RawValueLength will be 0 and | ||
// pRawValue will be null. | ||
if (pUnknownHeader->pName != null && pUnknownHeader->NameLength > 0) | ||
{ | ||
// For unknown headers, when header value is empty, RawValueLength will be 0 and | ||
// pRawValue will be null. | ||
if (pUnknownHeader->pName != null && pUnknownHeader->NameLength > 0) | ||
string headerName = new string(pUnknownHeader->pName + fixup, 0, pUnknownHeader->NameLength); | ||
string headerValue; | ||
if (pUnknownHeader->pRawValue != null && pUnknownHeader->RawValueLength > 0) | ||
{ | ||
string headerName = new string(pUnknownHeader->pName + fixup, 0, pUnknownHeader->NameLength); | ||
string headerValue; | ||
if (pUnknownHeader->pRawValue != null && pUnknownHeader->RawValueLength > 0) | ||
{ | ||
headerValue = new string(pUnknownHeader->pRawValue + fixup, 0, pUnknownHeader->RawValueLength); | ||
} | ||
else | ||
{ | ||
headerValue = string.Empty; | ||
} | ||
headerCollection.Add(headerName, headerValue); | ||
headerValue = new string(pUnknownHeader->pRawValue + fixup, 0, pUnknownHeader->RawValueLength); | ||
} | ||
pUnknownHeader++; | ||
else | ||
{ | ||
headerValue = string.Empty; | ||
} | ||
headerCollection.Add(headerName, headerValue); | ||
} | ||
pUnknownHeader++; | ||
} | ||
} | ||
|
||
// known headers | ||
HTTP_KNOWN_HEADER* pKnownHeader = &request->Headers.KnownHeaders; | ||
for (index = 0; index < HttpHeaderRequestMaximum; index++) | ||
// known headers | ||
HTTP_KNOWN_HEADER* pKnownHeader = &request->Headers.KnownHeaders; | ||
for (index = 0; index < HttpHeaderRequestMaximum; index++) | ||
{ | ||
// For known headers, when header value is empty, RawValueLength will be 0 and | ||
// pRawValue will point to empty string ("\0") | ||
if (pKnownHeader->pRawValue != null) | ||
{ | ||
// For known headers, when header value is empty, RawValueLength will be 0 and | ||
// pRawValue will point to empty string ("\0") | ||
if (pKnownHeader->pRawValue != null) | ||
{ | ||
string headerValue = new string(pKnownHeader->pRawValue + fixup, 0, pKnownHeader->RawValueLength); | ||
headerCollection.Add(HTTP_REQUEST_HEADER_ID.ToString(index), headerValue); | ||
} | ||
pKnownHeader++; | ||
string headerValue = new string(pKnownHeader->pRawValue + fixup, 0, pKnownHeader->RawValueLength); | ||
headerCollection.Add(HTTP_REQUEST_HEADER_ID.ToString(index), headerValue); | ||
} | ||
pKnownHeader++; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove extra tab/spacing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still seeing this extra space even in your latest set of commits. |
||
NetEventSource.Exit(null); | ||
return headerCollection; | ||
} | ||
|
||
|
||
internal static unsafe uint GetChunks(byte[] memoryBlob, IntPtr originalAddress, ref int dataChunkIndex, ref uint dataChunkOffset, byte[] buffer, int offset, int size) | ||
internal static unsafe uint GetChunks(IntPtr memoryBlob, IntPtr originalAddress, ref int dataChunkIndex, ref uint dataChunkOffset, byte[] buffer, int offset, int size) | ||
{ | ||
if (NetEventSource.IsEnabled) | ||
{ | ||
|
@@ -863,92 +858,86 @@ internal static unsafe uint GetChunks(byte[] memoryBlob, IntPtr originalAddress, | |
|
||
// Return value. | ||
uint dataRead = 0; | ||
fixed (byte* pMemoryBlob = memoryBlob) | ||
byte* pMemoryBlob = (byte*)memoryBlob; | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
long fixup = pMemoryBlob - (byte*)originalAddress; | ||
|
||
if (request->EntityChunkCount > 0 && dataChunkIndex < request->EntityChunkCount && dataChunkIndex != -1) | ||
{ | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
long fixup = pMemoryBlob - (byte*)originalAddress; | ||
HTTP_DATA_CHUNK* pDataChunk = (HTTP_DATA_CHUNK*)(fixup + (byte*)&request->pEntityChunks[dataChunkIndex]); | ||
|
||
if (request->EntityChunkCount > 0 && dataChunkIndex < request->EntityChunkCount && dataChunkIndex != -1) | ||
fixed (byte* pReadBuffer = buffer) | ||
{ | ||
HTTP_DATA_CHUNK* pDataChunk = (HTTP_DATA_CHUNK*)(fixup + (byte*)&request->pEntityChunks[dataChunkIndex]); | ||
byte* pTo = &pReadBuffer[offset]; | ||
|
||
fixed (byte* pReadBuffer = buffer) | ||
while (dataChunkIndex < request->EntityChunkCount && dataRead < size) | ||
{ | ||
byte* pTo = &pReadBuffer[offset]; | ||
|
||
while (dataChunkIndex < request->EntityChunkCount && dataRead < size) | ||
if (dataChunkOffset >= pDataChunk->BufferLength) | ||
{ | ||
dataChunkOffset = 0; | ||
dataChunkIndex++; | ||
pDataChunk++; | ||
} | ||
else | ||
{ | ||
if (dataChunkOffset >= pDataChunk->BufferLength) | ||
byte* pFrom = pDataChunk->pBuffer + dataChunkOffset + fixup; | ||
|
||
uint bytesToRead = pDataChunk->BufferLength - (uint)dataChunkOffset; | ||
if (bytesToRead > (uint)size) | ||
{ | ||
dataChunkOffset = 0; | ||
dataChunkIndex++; | ||
pDataChunk++; | ||
bytesToRead = (uint)size; | ||
} | ||
else | ||
for (uint i = 0; i < bytesToRead; i++) | ||
{ | ||
byte* pFrom = pDataChunk->pBuffer + dataChunkOffset + fixup; | ||
|
||
uint bytesToRead = pDataChunk->BufferLength - (uint)dataChunkOffset; | ||
if (bytesToRead > (uint)size) | ||
{ | ||
bytesToRead = (uint)size; | ||
} | ||
for (uint i = 0; i < bytesToRead; i++) | ||
{ | ||
*(pTo++) = *(pFrom++); | ||
} | ||
dataRead += bytesToRead; | ||
dataChunkOffset += bytesToRead; | ||
*(pTo++) = *(pFrom++); | ||
} | ||
dataRead += bytesToRead; | ||
dataChunkOffset += bytesToRead; | ||
} | ||
} | ||
} | ||
//we're finished. | ||
if (dataChunkIndex == request->EntityChunkCount) | ||
{ | ||
dataChunkIndex = -1; | ||
} | ||
} | ||
|
||
//we're finished. | ||
if (dataChunkIndex == request->EntityChunkCount) | ||
{ | ||
dataChunkIndex = -1; | ||
} | ||
|
||
if (NetEventSource.IsEnabled) | ||
{ | ||
NetEventSource.Exit(null); | ||
} | ||
return dataRead; | ||
} | ||
|
||
internal static unsafe HTTP_VERB GetKnownVerb(byte[] memoryBlob, IntPtr originalAddress) | ||
internal static unsafe HTTP_VERB GetKnownVerb(IntPtr memoryBlob, IntPtr originalAddress) | ||
{ | ||
NetEventSource.Enter(null); | ||
|
||
// Return value. | ||
HTTP_VERB verb = HTTP_VERB.HttpVerbUnknown; | ||
fixed (byte* pMemoryBlob = memoryBlob) | ||
|
||
HTTP_REQUEST* request = (HTTP_REQUEST*)memoryBlob.ToPointer(); | ||
if ((int)request->Verb > (int)HTTP_VERB.HttpVerbUnparsed && (int)request->Verb < (int)HTTP_VERB.HttpVerbMaximum) | ||
{ | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
if ((int)request->Verb > (int)HTTP_VERB.HttpVerbUnparsed && (int)request->Verb < (int)HTTP_VERB.HttpVerbMaximum) | ||
{ | ||
verb = request->Verb; | ||
} | ||
verb = request->Verb; | ||
} | ||
|
||
NetEventSource.Exit(null); | ||
return verb; | ||
} | ||
|
||
internal static unsafe IPEndPoint GetRemoteEndPoint(byte[] memoryBlob, IntPtr originalAddress) | ||
internal static unsafe IPEndPoint GetRemoteEndPoint(IntPtr memoryBlob, IntPtr originalAddress) | ||
{ | ||
if (NetEventSource.IsEnabled) NetEventSource.Enter(null); | ||
|
||
SocketAddress v4address = new SocketAddress(AddressFamily.InterNetwork, IPv4AddressSize); | ||
SocketAddress v6address = new SocketAddress(AddressFamily.InterNetworkV6, IPv6AddressSize); | ||
|
||
fixed (byte* pMemoryBlob = memoryBlob) | ||
{ | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
IntPtr address = request->Address.pRemoteAddress != null ? (IntPtr)(pMemoryBlob - (byte*)originalAddress + (byte*)request->Address.pRemoteAddress) : IntPtr.Zero; | ||
CopyOutAddress(address, ref v4address, ref v6address); | ||
} | ||
byte* pMemoryBlob = (byte*)memoryBlob; | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
IntPtr address = request->Address.pRemoteAddress != null ? (IntPtr)(pMemoryBlob - (byte*)originalAddress + (byte*)request->Address.pRemoteAddress) : IntPtr.Zero; | ||
CopyOutAddress(address, ref v4address, ref v6address); | ||
|
||
IPEndPoint endpoint = null; | ||
if (v4address != null) | ||
|
@@ -964,19 +953,17 @@ internal static unsafe IPEndPoint GetRemoteEndPoint(byte[] memoryBlob, IntPtr or | |
return endpoint; | ||
} | ||
|
||
internal static unsafe IPEndPoint GetLocalEndPoint(byte[] memoryBlob, IntPtr originalAddress) | ||
internal static unsafe IPEndPoint GetLocalEndPoint(IntPtr memoryBlob, IntPtr originalAddress) | ||
{ | ||
if (NetEventSource.IsEnabled) NetEventSource.Enter(null); | ||
|
||
SocketAddress v4address = new SocketAddress(AddressFamily.InterNetwork, IPv4AddressSize); | ||
SocketAddress v6address = new SocketAddress(AddressFamily.InterNetworkV6, IPv6AddressSize); | ||
|
||
fixed (byte* pMemoryBlob = memoryBlob) | ||
{ | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
IntPtr address = request->Address.pLocalAddress != null ? (IntPtr)(pMemoryBlob - (byte*)originalAddress + (byte*)request->Address.pLocalAddress) : IntPtr.Zero; | ||
CopyOutAddress(address, ref v4address, ref v6address); | ||
} | ||
byte* pMemoryBlob = (byte*)memoryBlob; | ||
HTTP_REQUEST* request = (HTTP_REQUEST*)pMemoryBlob; | ||
IntPtr address = request->Address.pLocalAddress != null ? (IntPtr)(pMemoryBlob - (byte*)originalAddress + (byte*)request->Address.pLocalAddress) : IntPtr.Zero; | ||
CopyOutAddress(address, ref v4address, ref v6address); | ||
|
||
IPEndPoint endpoint = null; | ||
if (v4address != null) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,16 @@ | |
// See the LICENSE file in the project root for more information. | ||
|
||
using System.Diagnostics; | ||
using System.Runtime.InteropServices; | ||
|
||
namespace System.Net | ||
{ | ||
internal abstract unsafe class RequestContextBase : IDisposable | ||
{ | ||
private Interop.HttpApi.HTTP_REQUEST* _memoryBlob; | ||
private Interop.HttpApi.HTTP_REQUEST* _originalBlobAddress; | ||
private byte[] _backingBuffer; | ||
private IntPtr _backingBuffer = IntPtr.Zero; | ||
private int _backingBufferLength = 0; | ||
|
||
// Must call this from derived class' constructors. | ||
protected void BaseConstruction(Interop.HttpApi.HTTP_REQUEST* requestBlob) | ||
|
@@ -29,7 +31,7 @@ protected void BaseConstruction(Interop.HttpApi.HTTP_REQUEST* requestBlob) | |
// before an object (HttpListenerRequest) which closes the RequestContext on demand is returned to the application. | ||
internal void ReleasePins() | ||
{ | ||
Debug.Assert(_memoryBlob != null || _backingBuffer == null, "RequestContextBase::ReleasePins()|ReleasePins() called twice."); | ||
Debug.Assert(_memoryBlob != null || _backingBuffer == IntPtr.Zero, "RequestContextBase::ReleasePins()|ReleasePins() called twice."); | ||
_originalBlobAddress = _memoryBlob; | ||
UnsetBlob(); | ||
OnReleasePins(); | ||
|
@@ -48,7 +50,14 @@ public void Dispose() | |
Dispose(true); | ||
} | ||
|
||
protected virtual void Dispose(bool disposing) { } | ||
protected virtual void Dispose(bool disposing) | ||
{ | ||
if (_backingBuffer != IntPtr.Zero) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please review IDisposable pattern. Not sure if you should add an 'if' check for "disposing == true" here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the current behavior is correct, based on this page. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. What about a 'finalizer'? Does this have one already and do we need one? Reading the pattern description from your link above says this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RequestContextBase does implement a finalizer that calls Dispose(bool), freeing the unmanaged resources it has acquired. I'm not very familiar with the pattern though, so I'll read through the rest of that dispose/finalize article and make sure I'm doing everything correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've checked it out and am confident the dispose pattern is correctly implemented here. |
||
{ | ||
Marshal.FreeHGlobal(_backingBuffer); | ||
_backingBuffer = IntPtr.Zero; | ||
} | ||
} | ||
|
||
~RequestContextBase() | ||
{ | ||
|
@@ -59,12 +68,12 @@ internal Interop.HttpApi.HTTP_REQUEST* RequestBlob | |
{ | ||
get | ||
{ | ||
Debug.Assert(_memoryBlob != null || _backingBuffer == null, "RequestContextBase::Dispose()|RequestBlob requested after ReleasePins()."); | ||
Debug.Assert(_memoryBlob != null || _backingBuffer == IntPtr.Zero, "RequestContextBase::Dispose()|RequestBlob requested after ReleasePins()."); | ||
return _memoryBlob; | ||
} | ||
} | ||
|
||
internal byte[] RequestBuffer | ||
internal IntPtr RequestBuffer | ||
{ | ||
get | ||
{ | ||
|
@@ -76,7 +85,7 @@ internal uint Size | |
{ | ||
get | ||
{ | ||
return (uint)_backingBuffer.Length; | ||
return (uint)_backingBufferLength; | ||
} | ||
} | ||
|
||
|
@@ -91,7 +100,7 @@ internal IntPtr OriginalBlobAddress | |
|
||
protected void SetBlob(Interop.HttpApi.HTTP_REQUEST* requestBlob) | ||
{ | ||
Debug.Assert(_memoryBlob != null || _backingBuffer == null, "RequestContextBase::Dispose()|SetBlob() called after ReleasePins()."); | ||
Debug.Assert(_memoryBlob != null || _backingBuffer == IntPtr.Zero, "RequestContextBase::Dispose()|SetBlob() called after ReleasePins()."); | ||
if (requestBlob == null) | ||
{ | ||
UnsetBlob(); | ||
|
@@ -116,7 +125,19 @@ protected void UnsetBlob() | |
|
||
protected void SetBuffer(int size) | ||
{ | ||
_backingBuffer = size == 0 ? null : new byte[size]; | ||
if(_backingBuffer != IntPtr.Zero) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add space character after 'if' |
||
{ | ||
Marshal.FreeHGlobal(_backingBuffer); | ||
} | ||
|
||
_backingBuffer = size == 0 ? IntPtr.Zero : Marshal.AllocHGlobal(size); | ||
_backingBufferLength = size; | ||
|
||
// Zero out the contents of the buffer. | ||
for(int i = 0; i < size; ++i) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add space character after 'for' |
||
{ | ||
Marshal.WriteByte(_backingBuffer + i, 0); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How big are these usually? Calling Marshal.WriteByte per byte for a large buffer isn't cheap. Alternatively you could use span: new Span<byte>((byte*)_backingBuffer, 0, size).Fill(0); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default size is 4096, but they can be larger than that depending on the size of the request. I really don't like having that loop there, so I'll look into doing it that way instead. Thanks! |
||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vast majority of the line changes in this file are the result of removing these fixed statements, and thus changing the indentation for large blocks of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, if you append ?w=1 to the GitHub url, it'll ignore whitespace differences:
https://github.com/dotnet/corefx/pull/25563/files?w=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know!