Unset reserved bit when setting 31 bit uint values (#7339)

* Unset reserved bit when setting 31 bit uint values

* Add FrameWriter tests

* Add overload

src/Servers/Kestrel/Core/src/Internal/Http2/Bitshifter.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
@@ -36,11 +36,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
         // Does not overwrite the highest order bit
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static void WriteUInt31BigEndian(Span<byte> destination, uint value)
+            => WriteUInt31BigEndian(destination, value, true);
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static void WriteUInt31BigEndian(Span<byte> destination, uint value, bool preserveHighestBit)
         {
             Debug.Assert(value <= 0x7F_FF_FF_FF, value.ToString());
-            // Keep the highest bit
+
-            var reserved = (destination[0] & 0x80u) << 24;
+            if (preserveHighestBit)
-            BinaryPrimitives.WriteUInt32BigEndian(destination, value | reserved);
+            {
+                // Keep the highest bit
+                value |= (destination[0] & 0x80u) << 24;
+            }
+
+            BinaryPrimitives.WriteUInt32BigEndian(destination, value);
         }
     }
 }

src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs

@@ -402,7 +402,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
                 _outgoingFrame.PrepareWindowUpdate(streamId, sizeIncrement);
                 WriteHeaderUnsynchronized();
                 var buffer = _outputWriter.GetSpan(4);
-                Bitshifter.WriteUInt31BigEndian(buffer, (uint)sizeIncrement);
+                Bitshifter.WriteUInt31BigEndian(buffer, (uint)sizeIncrement, preserveHighestBit: false);
                 _outputWriter.Advance(4);
                 return TimeFlushUnsynchronizedAsync();
             }
@@ -538,7 +538,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
                 WriteHeaderUnsynchronized();
 
                 var buffer = _outputWriter.GetSpan(8);
-                Bitshifter.WriteUInt31BigEndian(buffer, (uint)lastStreamId);
+                Bitshifter.WriteUInt31BigEndian(buffer, (uint)lastStreamId, preserveHighestBit: false);
                 buffer = buffer.Slice(4);
                 BinaryPrimitives.WriteUInt32BigEndian(buffer, (uint)errorCode);
                 _outputWriter.Advance(8);
@@ -578,7 +578,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
             buffer[1] = frame.Flags;
             buffer = buffer.Slice(2);
 
-            Bitshifter.WriteUInt31BigEndian(buffer, (uint)frame.StreamId);
+            Bitshifter.WriteUInt31BigEndian(buffer, (uint)frame.StreamId, preserveHighestBit: false);
 
             output.Advance(Http2FrameReader.HeaderLength);
         }

src/Servers/Kestrel/Core/test/BitShifterTests.cs

@@ -0,0 +1,48 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
+{
+    public class BitShifterTests
+    {
+        [Fact]
+        public void WriteUInt31BigEndian_PreservesHighestBit()
+        {
+            // Arrange
+            Span<byte> dirtySpan = new byte[] { 0xff, 0xff, 0xff, 0xff };
+
+            // Act
+            Bitshifter.WriteUInt31BigEndian(dirtySpan, 1);
+
+            Assert.Equal(new byte[] { 0x80, 0x00, 0x00, 0x01 }, dirtySpan.ToArray());
+        }
+
+        [Fact]
+        public void WriteUInt31BigEndian_True_PreservesHighestBit()
+        {
+            // Arrange
+            Span<byte> dirtySpan = new byte[] { 0xff, 0xff, 0xff, 0xff };
+
+            // Act
+            Bitshifter.WriteUInt31BigEndian(dirtySpan, 1, true);
+
+            Assert.Equal(new byte[] { 0x80, 0x00, 0x00, 0x01 }, dirtySpan.ToArray());
+        }
+
+        [Fact]
+        public void WriteUInt31BigEndian_False_OverwritesHighestBit()
+        {
+            // Arrange
+            Span<byte> dirtySpan = new byte[] { 0xff, 0xff, 0xff, 0xff };
+
+            // Act
+            Bitshifter.WriteUInt31BigEndian(dirtySpan, 1, false);
+
+            Assert.Equal(new byte[] { 0x00, 0x00, 0x00, 0x01 }, dirtySpan.ToArray());
+        }
+    }
+}

src/Servers/Kestrel/Core/test/Http2FrameWriterTests.cs

@@ -0,0 +1,108 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Buffers;
+using System.IO.Pipelines;
+using System.Linq;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
+using Moq;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
+{
+    public class Http2FrameWriterTests
+    {
+        private MemoryPool<byte> _dirtyMemoryPool;
+
+        public Http2FrameWriterTests()
+        {
+            var memoryBlock = new Mock<IMemoryOwner<byte>>();
+            memoryBlock.Setup(block => block.Memory).Returns(() =>
+            {
+                var blockArray = new byte[4096];
+                for (int i = 0; i < 4096; i++)
+                {
+                    blockArray[i] = 0xff;
+                }
+                return new Memory<byte>(blockArray);
+            });
+
+            var dirtyMemoryPool = new Mock<MemoryPool<byte>>();
+            dirtyMemoryPool.Setup(pool => pool.Rent(It.IsAny<int>())).Returns(memoryBlock.Object);
+            _dirtyMemoryPool = dirtyMemoryPool.Object;
+        }
+
+        [Fact]
+        public async Task WriteWindowUpdate_UnsetsReservedBit()
+        {
+            // Arrange
+            var pipe = new Pipe(new PipeOptions(_dirtyMemoryPool, PipeScheduler.Inline, PipeScheduler.Inline));
+            var frameWriter = new Http2FrameWriter(pipe.Writer, null, null, null, null, null, null, new Mock<IKestrelTrace>().Object);
+
+            // Act
+            await frameWriter.WriteWindowUpdateAsync(1, 1);
+
+            // Assert
+            var payload = await pipe.Reader.ReadForLengthAsync(Http2FrameReader.HeaderLength + 4);
+
+            Assert.Equal(new byte[] { 0x00, 0x00, 0x00, 0x01 }, payload.Skip(Http2FrameReader.HeaderLength).Take(4).ToArray());
+        }
+
+        [Fact]
+        public async Task WriteGoAway_UnsetsReservedBit()
+        {
+            // Arrange
+            var pipe = new Pipe(new PipeOptions(_dirtyMemoryPool, PipeScheduler.Inline, PipeScheduler.Inline));
+            var frameWriter = new Http2FrameWriter(pipe.Writer, null, null, null, null, null, null, new Mock<IKestrelTrace>().Object);
+
+            // Act
+            await frameWriter.WriteGoAwayAsync(1, Http2ErrorCode.NO_ERROR);
+
+            // Assert
+            var payload = await pipe.Reader.ReadForLengthAsync(Http2FrameReader.HeaderLength + 4);
+
+            Assert.Equal(new byte[] { 0x00, 0x00, 0x00, 0x01 }, payload.Skip(Http2FrameReader.HeaderLength).Take(4).ToArray());
+        }
+
+        [Fact]
+        public async Task WriteHeader_UnsetsReservedBit()
+        {
+            // Arrange
+            var pipe = new Pipe(new PipeOptions(_dirtyMemoryPool, PipeScheduler.Inline, PipeScheduler.Inline));
+            var frame = new Http2Frame();
+            frame.PreparePing(Http2PingFrameFlags.NONE);
+
+            // Act
+            Http2FrameWriter.WriteHeader(frame, pipe.Writer);
+            await pipe.Writer.FlushAsync();
+
+            // Assert
+            var payload = await pipe.Reader.ReadForLengthAsync(Http2FrameReader.HeaderLength);
+
+            Assert.Equal(new byte[] { 0x00, 0x00, 0x00, 0x00 }, payload.Skip(5).Take(4).ToArray());
+        }
+    }
+
+    public static class PipeReaderExtensions
+    {
+        public static async Task<byte[]> ReadForLengthAsync(this PipeReader pipeReader, int length)
+        {
+            while (true)
+            {
+                var result = await pipeReader.ReadAsync();
+                var buffer = result.Buffer;
+
+                if (!buffer.IsEmpty && buffer.Length >= length)
+                {
+                    return buffer.Slice(0, length).ToArray();
+                }
+
+                pipeReader.AdvanceTo(buffer.Start, buffer.End);
+            }
+        }
+    }
+}

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs

@@ -1055,7 +1055,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
             frame.PrepareWindowUpdate(streamId, sizeIncrement);
             Http2FrameWriter.WriteHeader(frame, outputWriter);
             var buffer = outputWriter.GetSpan(4);
-            Bitshifter.WriteUInt31BigEndian(buffer, (uint)sizeIncrement);
+            BinaryPrimitives.WriteUInt32BigEndian(buffer, (uint)sizeIncrement);
             outputWriter.Advance(4);
             return FlushAsync(outputWriter);
         }