From 4c39374dc063487886f93cc2460a03e26a213a2f Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 4 May 2016 16:45:59 -0700 Subject: [PATCH 1/2] Always check if block is last in linked list before consuming it - This prevents a race where we could read into the "Next" Block without completely consuming the previous one. --- .../Infrastructure/MemoryPoolIterator.cs | 74 ++++++++++++------- 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs index 286862ce4b..8d65c5c377 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs @@ -68,6 +68,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure } var index = _index; + var wasLastBlock = block.Next == null; if (index < block.End) { @@ -77,7 +78,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure do { - if (block.Next == null) + if (wasLastBlock) { return -1; } @@ -87,6 +88,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure index = block.Start; } + wasLastBlock = block.Next == null; + if (index < block.End) { _block = block; @@ -102,7 +105,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { return; } + + var wasLastBlock = _block.Next == null; var following = _block.End - _index; + if (following >= bytesToSkip) { _index += bytesToSkip; @@ -113,7 +119,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure var index = _index; while (true) { - if (block.Next == null) + if (wasLastBlock) { return; } @@ -123,7 +129,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure block = block.Next; index = block.Start; } + + wasLastBlock = block.Next == null; following = block.End - index; + if (following >= bytesToSkip) { _block = block; @@ -141,6 +150,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure return -1; } + var wasLastBlock = _block.Next == null; var index = _index; if (index < block.End) @@ -150,7 +160,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure do { - if (block.Next == null) + if (wasLastBlock) { return -1; } @@ -160,6 +170,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure index = block.Start; } + wasLastBlock = block.Next == null; + if (index < block.End) { return block.Array[index]; @@ -173,14 +185,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { return -1; } - else if (_block.End - _index >= sizeof(long)) + + var wasLastBlock = _block.Next == null; + + if (_block.End - _index >= sizeof(long)) { fixed (byte* ptr = &_block.Array[_index]) { return *(long*)(ptr); } } - else if (_block.Next == null) + else if (wasLastBlock) { return -1; } @@ -219,6 +234,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure var block = _block; var index = _index; + var wasLastBlock = block.Next == null; var following = block.End - index; byte[] array; var byte0 = byte0Vector[0]; @@ -227,16 +243,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { while (following == 0) { - var newBlock = block.Next; - if (newBlock == null) + if (wasLastBlock) { _block = block; _index = index; return -1; } - index = newBlock.Start; - following = newBlock.End - index; - block = newBlock; + block = block.Next; + index = block.Start; + wasLastBlock = block.Next == null; + following = block.End - index; } array = block.Array; while (following > 0) @@ -298,6 +314,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure var block = _block; var index = _index; + var wasLastBlock = block.Next == null; var following = block.End - index; byte[] array; int byte0Index = int.MaxValue; @@ -309,16 +326,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { while (following == 0) { - var newBlock = block.Next; - if (newBlock == null) + if (wasLastBlock) { _block = block; _index = index; return -1; } - index = newBlock.Start; - following = newBlock.End - index; - block = newBlock; + block = block.Next; + index = block.Start; + wasLastBlock = block.Next == null; + following = block.End - index; } array = block.Array; while (following > 0) @@ -405,6 +422,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure var block = _block; var index = _index; + var wasLastBlock = block.Next == null; var following = block.End - index; byte[] array; int byte0Index = int.MaxValue; @@ -418,16 +436,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { while (following == 0) { - var newBlock = block.Next; - if (newBlock == null) + if (wasLastBlock) { _block = block; _index = index; return -1; } - index = newBlock.Start; - following = newBlock.End - index; - block = newBlock; + block = block.Next; + index = block.Start; + wasLastBlock = block.Next == null; + following = block.End - index; } array = block.Array; while (following > 0) @@ -605,16 +623,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { return false; } - else if (_index < _block.End) - { - _block.Array[_index++] = data; - return true; - } var block = _block; var index = _index; while (true) { + var wasLastBlock = block.Next == null; + if (index < block.End) { _block = block; @@ -622,7 +637,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure block.Array[index] = data; return true; } - else if (block.Next == null) + else if (wasLastBlock) { return false; } @@ -679,6 +694,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure var remaining = count; while (true) { + // Determine if we might attempt to copy data from block.Next before + // calculating "following" so we don't risk skipping data that could + // be added after block.End when we decide to copy from block.Next. + // block.End will always be advanced before block.Next is set. + var wasLastBlock = block.Next == null; var following = block.End - index; if (remaining <= following) { @@ -689,7 +709,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure } return new MemoryPoolIterator(block, index + remaining); } - else if (block.Next == null) + else if (wasLastBlock) { actual = count - remaining + following; if (array != null) From 12a3816c122f0d86968922480bd2e53f42bde365 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 5 May 2016 15:00:58 -0700 Subject: [PATCH 2/2] Make some MemoryPoolBlock operations volatile - Making MemoryPoolBlock.End volatile and using Volatile.Write for MemoryPoolBlock.Next prevents a race that could cause threads consuming blocks to skip produced bytes. --- src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs | 2 +- .../Infrastructure/MemoryPoolBlock.cs | 6 +++--- .../Infrastructure/MemoryPoolIterator.cs | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs index 1588e83011..40532e17b4 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs @@ -105,7 +105,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } else { - _tail.Next = _pinned; + Volatile.Write(ref _tail.Next, _pinned); _tail = _pinned; } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolBlock.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolBlock.cs index 25cab8c4f4..1bcd60ecaa 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolBlock.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolBlock.cs @@ -57,14 +57,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure /// the Start is guaranteed to be equal to Array.Offset. The value of Start may be assigned anywhere between Data.Offset and /// Data.Offset + Data.Count, and must be equal to or less than End. /// - public int Start { get; set; } + public int Start; /// /// The End represents the offset into Array where the range of "active" bytes ends. At the point when the block is leased /// the End is guaranteed to be equal to Array.Offset. The value of Start may be assigned anywhere between Data.Offset and /// Data.Offset + Data.Count, and must be equal to or less than End. /// - public int End { get; set; } + public volatile int End; /// /// Reference to the next block of data when the overall "active" bytes spans multiple blocks. At the point when the block is @@ -72,7 +72,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure /// working memory. The "active" memory is grown when bytes are copied in, End is increased, and Next is assigned. The "active" /// memory is shrunk when bytes are consumed, Start is increased, and blocks are returned to the pool. /// - public MemoryPoolBlock Next { get; set; } + public MemoryPoolBlock Next; ~MemoryPoolBlock() { diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs index 8d65c5c377..202d15191f 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/MemoryPoolIterator.cs @@ -4,6 +4,7 @@ using System; using System.Diagnostics; using System.Numerics; +using System.Threading; namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { @@ -767,7 +768,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { var nextBlock = pool.Lease(); block.End = blockIndex; - block.Next = nextBlock; + Volatile.Write(ref block.Next, nextBlock); block = nextBlock; blockIndex = block.Data.Offset; @@ -820,7 +821,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Infrastructure { var nextBlock = pool.Lease(); block.End = blockIndex; - block.Next = nextBlock; + Volatile.Write(ref block.Next, nextBlock); block = nextBlock; blockIndex = block.Data.Offset;