Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/2.0.0] Ensure HttpListener request buffer is aligned as required by the host processor #25763

Merged
merged 3 commits into from
Jan 3, 2018

Conversation

rmkerr
Copy link
Contributor

@rmkerr rmkerr commented Dec 6, 2017

On Windows, the HttpRecieveHttpRequest function requires a buffer with a memory alignment greater than or equal to the required alignment of the HTTP_REQUEST struct.

This fix ensures that alignment requirements are respected when allocating buffers in HttpListener RequestContextBase. Since HttpReceiveHttpRequest copies both the HTTP_REQUEST struct and the variable length request body into the buffer, we need to be able to allocate a buffer with variable size and with a set alignment. Since C# does not provide a method for specifying the alignment of byte arrays, I switched the underlying buffer to be unmanaged. This unmanaged buffer is allocated using Marshal.AllocHGlobal, which allocates memory at the maximum alignment required by the host processor. This fix differs from the 2.1.0 fix in how it zeros out allocated memory, as 2.0.0 disables support for Span.

Fixes #25289

rmkerr and others added 2 commits December 5, 2017 14:19
… processor (dotnet#25563)

On Windows, the HttpRecieveHttpRequest function requires a buffer with a memory alignment greater than or equal to the required alignment of the HTTP_REQUEST struct.

This fix ensures that alignment requirements are respected when allocating buffers in HttpListener RequestContextBase. Since HttpReceiveHttpRequest copies both the HTTP_REQUEST struct and the variable length request body into the buffer, we need to be able to allocate a buffer with variable size and with a set alignment. Since C# does not provide a method for specifying the alignment of byte arrays, I switched the underlying buffer to be unmanaged. This unmanaged buffer is allocated using Marshal.AllocHGlobal, which allocates memory at the maximum alignment required by the host processor.
@rmkerr rmkerr added this to the 2.0.x milestone Dec 6, 2017
@rmkerr rmkerr self-assigned this Dec 6, 2017
// Zero out the contents of the buffer.
for(int i = 0; i < size; ++i)
{
Marshal.WriteByte(_backingBuffer + i, 0);
Copy link
Member

@jkotas jkotas Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marshal.WriteByte is expensive API to call for something like this.

Instead, cast the _backingBuffer to (byte*), and cache it in a local:

byte *buffer = (byte*)_backingBuffer;
for (int i = 0; i < size; i++) buffer[i] = 0;

Copy link
Contributor Author

@rmkerr rmkerr Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. I'm glad to know there's a better way to do that.

Edit: This is done in the latest iteration.

@rmkerr
Copy link
Contributor Author

rmkerr commented Dec 6, 2017

@dotnet-bot test Outerloop Windows x64 Debug Build please
@dotnet-bot test Outerloop Windows x86 Debug Build please

@davidsh davidsh added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 8, 2017
@Petermarcu
Copy link
Member

@davidsh , We're open for checkins for the next release but I see the NO MERGE label on this one. Is there something left to do before we would want to merge?

@davidsh
Copy link
Contributor

davidsh commented Jan 3, 2018

@davidsh , We're open for checkins for the next release but I see the NO MERGE label on this one. Is there something left to do before we would want to merge?

It was marked as NO MERGE when the branch wasn't opened for checkin. It was marked that way to prevent accidental merges. But it is ready to go now. So, I will merge it now after CI finishes (again).

@davidsh davidsh removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 3, 2018
@davidsh davidsh merged commit 804c756 into dotnet:release/2.0.0 Jan 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants