diff --git a/.github/workflows/build-release.yml b/.github/workflows/build-release.yml index b73370cf9..d980617c8 100644 --- a/.github/workflows/build-release.yml +++ b/.github/workflows/build-release.yml @@ -40,9 +40,11 @@ jobs: ref: ${{ needs.update-packagejson.outputs.sha }} fetch-depth: 0 - uses: ./.github/actions/setup-dotnet - # pack nuget + # The source generator snapshot tests load a project reference rather than the version-stamped analyzer. + - run: dotnet test -c Release + - run: git clean -fdx + # Pack NuGet with the requested release version. - run: dotnet build -c Release -p:Version=${{ needs.update-packagejson.outputs.normalized_tag }} - - run: dotnet test -c Release --no-build - run: dotnet pack -c Release -p:Version=${{ needs.update-packagejson.outputs.normalized_tag }} -o ./publish - name: upload artifacts uses: actions/upload-artifact@v7 # must sync with actions/download-artifact@v8 in create-release diff --git a/.github/workflows/dotnet.yml b/.github/workflows/dotnet.yml index 578c78cd3..400846fb7 100644 --- a/.github/workflows/dotnet.yml +++ b/.github/workflows/dotnet.yml @@ -13,12 +13,16 @@ on: jobs: build-dotnet: - runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] + runs-on: ${{ matrix.os }} timeout-minutes: 15 steps: - uses: actions/checkout@v4 with: fetch-depth: 0 # avoid shallow clone so nbgv can do its work. - uses: ./.github/actions/setup-dotnet - - run: dotnet build -c Release -t:build,pack + - run: dotnet build -c Release -t:build,pack -warnaserror - run: dotnet test -c Release --no-build diff --git a/SECURITY.md b/SECURITY.md index e2612cff1..604e2c061 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -5,7 +5,7 @@ | Version | Supported | End-of-life date | | ------- | --------- | ---------------- | | 1.x | ❌ | -| 2.x | ✅ | 2025-12-31 | +| 2.x | ✅ | 2026-12-31 (at least) | | 3.x | ✅ | not yet determined | Each supported major version is only serviced for security issues at its tip. diff --git a/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/package.json b/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/package.json index f2b502787..ec902447d 100644 --- a/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/package.json +++ b/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/package.json @@ -1,7 +1,7 @@ { "name": "com.github.messagepack-csharp", "displayName": "MessagePack", - "version": "3.1.8", + "version": "3.1.9", "unity": "2021.3", "description": "Extremely Fast MessagePack Serializer for C#.", "keywords": [ diff --git a/src/MessagePack/MessagePackReader.cs b/src/MessagePack/MessagePackReader.cs index 351921abf..7e41b8979 100644 --- a/src/MessagePack/MessagePackReader.cs +++ b/src/MessagePack/MessagePackReader.cs @@ -31,6 +31,17 @@ ref partial struct MessagePackReader /// private SequenceReader reader; + /// + /// The minimum number of bytes that previously-read container headers have committed to their children + /// but that have not yet been consumed. + /// + private long minimumRemainingChildBytes; + + /// + /// The value of when was last updated. + /// + private long minimumRemainingChildBytesCheckpoint; + /// /// Initializes a new instance of the struct. /// @@ -353,7 +364,7 @@ public int ReadArrayHeader() // Protect against corrupted or mischievous data that may lead to allocating way too much memory. // We allow for each primitive to be the minimal 1 byte in size. // Formatters that know each element is larger can optionally add a stronger check. - ThrowInsufficientBufferUnless(this.reader.Remaining >= count); + this.ReserveMinimumChildBytes(count); return count; } @@ -430,7 +441,7 @@ public int ReadMapHeader() // Protect against corrupted or mischievous data that may lead to allocating way too much memory. // We allow for each primitive to be the minimal 1 byte in size, and we have a key=value map, so that's 2 bytes. // Formatters that know each element is larger can optionally add a stronger check. - ThrowInsufficientBufferUnless(this.reader.Remaining >= (long)count * 2); + this.ReserveMinimumChildBytes((long)count * 2); return count; } @@ -1002,6 +1013,20 @@ private static void ThrowInsufficientBufferUnless(bool condition) [DoesNotReturn] private static Exception ThrowUnreachable() => throw new Exception("Presumed unreachable point in code reached."); + private void ReserveMinimumChildBytes(long minimumChildBytes) + { + long consumed = this.reader.Consumed; + long bytesConsumedSinceCheckpoint = consumed - this.minimumRemainingChildBytesCheckpoint; + this.minimumRemainingChildBytes = Math.Max(0, this.minimumRemainingChildBytes - bytesConsumedSinceCheckpoint); + this.minimumRemainingChildBytesCheckpoint = consumed; + + ThrowInsufficientBufferUnless( + minimumChildBytes >= 0 + && this.minimumRemainingChildBytes <= this.reader.Remaining + && minimumChildBytes <= this.reader.Remaining - this.minimumRemainingChildBytes); + this.minimumRemainingChildBytes += minimumChildBytes; + } + private uint GetBytesLength() { ThrowInsufficientBufferUnless(this.TryGetBytesLength(out uint length)); diff --git a/src/MessagePack/MessagePackWriter.cs b/src/MessagePack/MessagePackWriter.cs index 36695da27..b6bd188fd 100644 --- a/src/MessagePack/MessagePackWriter.cs +++ b/src/MessagePack/MessagePackWriter.cs @@ -454,7 +454,7 @@ public void Write(byte[]? src) /// /// The span of bytes to write. /// - /// When is , the msgpack code used is , or instead. + /// When is , the msgpack code used is fixstr, or instead (never , which is not defined in the old spec). /// public void Write(scoped ReadOnlySpan src) { @@ -473,7 +473,7 @@ public void Write(scoped ReadOnlySpan src) /// /// The span of bytes to write. /// - /// When is , the msgpack code used is , or instead. + /// When is , the msgpack code used is fixstr, or instead (never , which is not defined in the old spec). /// public void Write(in ReadOnlySequence src) { @@ -498,7 +498,7 @@ public void Write(in ReadOnlySequence src) /// Alternatively a single call to or will take care of the header and content in one call. /// /// - /// When is , the msgpack code used is , or instead. + /// When is , the msgpack code used is fixstr, or instead (never , which is not defined in the old spec). /// /// public void WriteBinHeader(int length) @@ -559,15 +559,34 @@ public void WriteString(ReadOnlySpan utf8stringBytes) /// /// The number of bytes in the string that will follow this header. /// + /// /// The caller should use or /// after calling this method to actually write the content. /// Alternatively a single call to or will take care of the header and content in one call. + /// + /// + /// When is , is never used because it is not defined in the old spec; + /// lengths that would otherwise use str8 are encoded with instead. + /// /// public void WriteStringHeader(int byteCount) { // When we write the header, we'll ask for all the space we need for the payload as well // as that may help ensure we only allocate a buffer once. Span span = this.writer.GetSpan(byteCount + 5); + + // The old msgpack spec does not define str8 (0xd9). Use str16 for lengths 32-255 when OldSpec is set. + // This matches WriteString_PostEncoding and keeps binary-as-string (WriteBinHeader) legacy-compatible. + if (this.OldSpec && byteCount > MessagePackRange.MaxFixStringLength && byteCount <= byte.MaxValue) + { + span[0] = MessagePackCode.Str16; + span[1] = 0; + span[2] = unchecked((byte)byteCount); + + this.writer.Advance(3); + return; + } + AssumesTrue(MessagePackPrimitives.TryWriteStringHeader(span, (uint)byteCount, out int written)); this.writer.Advance(written); } diff --git a/tests/MessagePack.SourceGenerator.Tests/Verifiers/CSharpSourceGeneratorVerifier`1+Test.cs b/tests/MessagePack.SourceGenerator.Tests/Verifiers/CSharpSourceGeneratorVerifier`1+Test.cs index 74288a368..05ca9ef26 100644 --- a/tests/MessagePack.SourceGenerator.Tests/Verifiers/CSharpSourceGeneratorVerifier`1+Test.cs +++ b/tests/MessagePack.SourceGenerator.Tests/Verifiers/CSharpSourceGeneratorVerifier`1+Test.cs @@ -2,7 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. // Uncomment the following line to write expected files to disk -#define WRITE_EXPECTED +// #define WRITE_EXPECTED #if WRITE_EXPECTED #warning WRITE_EXPECTED is fine for local builds, but should not be merged to the main branch. @@ -117,13 +117,11 @@ static void AddGeneratedSources(ProjectState project, string testMethod, bool wi .Replace('(', '_') .Replace(')', '_'); - foreach (var resourceName in typeof(Test).Assembly.GetManifestResourceNames()) + foreach (var resourceName in typeof(Test).Assembly.GetManifestResourceNames() + .Where(name => name.StartsWith(expectedPrefix, StringComparison.Ordinal)) + .OrderByDescending(name => name.EndsWith(".MessagePack.GeneratedMessagePackResolver.g.cs", StringComparison.Ordinal)) + .ThenBy(name => name, StringComparer.Ordinal)) { - if (!resourceName.StartsWith(expectedPrefix)) - { - continue; - } - using var resourceStream = Assembly.GetExecutingAssembly().GetManifestResourceStream(resourceName); if (resourceStream is null) { diff --git a/tests/MessagePack.Tests/MessagePackReaderTests.cs b/tests/MessagePack.Tests/MessagePackReaderTests.cs index 3278386c2..c069d60fb 100644 --- a/tests/MessagePack.Tests/MessagePackReaderTests.cs +++ b/tests/MessagePack.Tests/MessagePackReaderTests.cs @@ -69,6 +69,142 @@ public void ReadArrayHeader_MitigatesLargeAllocations() }); } + [Fact] + [Trait("CWE", "789")] + public void ReadArrayHeader_MitigatesNestedAllocationAmplification() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteArrayHeader(4); + writer.WriteArrayHeader(4); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.Flush(); + + Assert.Throws(() => + { + var reader = new MessagePackReader(sequence); + Assert.Equal(4, reader.ReadArrayHeader()); + reader.ReadArrayHeader(); + }); + } + + [Fact] + public void ReadArrayHeader_AllowsValidNestedAndConsecutiveContainers() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteArrayHeader(2); + writer.Write("long child"); + writer.WriteArrayHeader(2); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteArrayHeader(1); + writer.WriteNil(); + writer.Flush(); + + var reader = new MessagePackReader(sequence); + Assert.Equal(2, reader.ReadArrayHeader()); + Assert.Equal("long child", reader.ReadString()); + Assert.Equal(2, reader.ReadArrayHeader()); + reader.ReadNil(); + reader.ReadNil(); + Assert.Equal(1, reader.ReadArrayHeader()); + reader.ReadNil(); + Assert.True(reader.End); + } + + [Fact] + public void TryReadArrayHeader_DoesNotReserveMinimumChildBytes() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteArrayHeader(4); + writer.WriteArrayHeader(4); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.Flush(); + + var reader = new MessagePackReader(sequence); + Assert.Equal(4, reader.ReadArrayHeader()); + Assert.True(reader.TryReadArrayHeader(out int count)); + Assert.Equal(4, count); + } + + [Fact] + [Trait("CWE", "190")] + public void ReadArrayHeader_MitigatesLargeAllocations_WhenElementCountExceedsInt32() + { + byte[] msgpack = { MessagePackCode.Array32, 0x80, 0, 0, 0 }; + + Assert.Throws(() => + { + var reader = new MessagePackReader(msgpack); + reader.ReadArrayHeader(); + }); + } + + [Fact] + public void CreatePeekReader_CopiesMinimumChildByteReservation() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteArrayHeader(4); + writer.WriteArrayHeader(4); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.Flush(); + + Assert.Throws(() => + { + var reader = new MessagePackReader(sequence); + Assert.Equal(4, reader.ReadArrayHeader()); + MessagePackReader peekReader = reader.CreatePeekReader(); + peekReader.ReadArrayHeader(); + }); + + Assert.Throws(() => + { + var reader = new MessagePackReader(sequence); + Assert.Equal(4, reader.ReadArrayHeader()); + reader.ReadArrayHeader(); + }); + } + + [Fact] + public void Clone_StartsMinimumChildByteReservationForReplacementBuffer() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteArrayHeader(4); + writer.WriteArrayHeader(4); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.Flush(); + + var reader = new MessagePackReader(sequence); + Assert.Equal(4, reader.ReadArrayHeader()); + + var replacementSequence = new Sequence(); + writer = new MessagePackWriter(replacementSequence); + writer.WriteArrayHeader(1); + writer.WriteNil(); + writer.Flush(); + + MessagePackReader clone = reader.Clone(replacementSequence); + Assert.Equal(1, clone.ReadArrayHeader()); + clone.ReadNil(); + Assert.True(clone.End); + } + [Fact] public void TryReadArrayHeader() { @@ -101,6 +237,81 @@ public void ReadMapHeader_MitigatesLargeAllocations() }); } + [Fact] + [Trait("CWE", "789")] + public void ReadMapHeader_MitigatesNestedAllocationAmplification() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteMapHeader(2); + writer.WriteMapHeader(2); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.Flush(); + + Assert.Throws(() => + { + var reader = new MessagePackReader(sequence); + Assert.Equal(2, reader.ReadMapHeader()); + reader.ReadMapHeader(); + }); + } + + [Fact] + [Trait("CWE", "789")] + public void ReadMapHeader_MitigatesMixedNestedAllocationAmplification() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteArrayHeader(4); + writer.WriteMapHeader(2); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.Flush(); + + Assert.Throws(() => + { + var reader = new MessagePackReader(sequence); + Assert.Equal(4, reader.ReadArrayHeader()); + reader.ReadMapHeader(); + }); + } + + [Fact] + public void ReadMapHeader_ReturnsReservationAsContentsAreConsumed() + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + writer.WriteMapHeader(2); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteMapHeader(2); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.WriteNil(); + writer.Flush(); + + var reader = new MessagePackReader(sequence); + Assert.Equal(2, reader.ReadMapHeader()); + reader.ReadNil(); + reader.ReadNil(); + reader.ReadNil(); + reader.ReadNil(); + Assert.Equal(2, reader.ReadMapHeader()); + reader.ReadNil(); + reader.ReadNil(); + reader.ReadNil(); + reader.ReadNil(); + Assert.True(reader.End); + } + [Fact] [Trait("CWE", "190")] public void ReadMapHeader_MitigatesLargeAllocations_WhenMinimumPayloadLengthOverflowsInt32() @@ -114,6 +325,19 @@ public void ReadMapHeader_MitigatesLargeAllocations_WhenMinimumPayloadLengthOver }); } + [Fact] + [Trait("CWE", "190")] + public void ReadMapHeader_MitigatesLargeAllocations_WhenElementCountExceedsInt32() + { + byte[] msgpack = { MessagePackCode.Map32, 0x80, 0, 0, 0 }; + + Assert.Throws(() => + { + var reader = new MessagePackReader(msgpack); + reader.ReadMapHeader(); + }); + } + [Fact] [Trait("CWE", "190")] public void SkipMap_MitigatesLargeAllocations_WhenMinimumPayloadLengthOverflowsInt32() diff --git a/tests/MessagePack.Tests/MessagePackWriterTests.cs b/tests/MessagePack.Tests/MessagePackWriterTests.cs index 85d293650..b3a02d907 100644 --- a/tests/MessagePack.Tests/MessagePackWriterTests.cs +++ b/tests/MessagePack.Tests/MessagePackWriterTests.cs @@ -143,6 +143,73 @@ public void WriteBinHeader() Assert.Equal(new byte[] { 1, 2, 3, 4, 5 }, reader.ReadBytes().Value.ToArray()); } + /// + /// Regression for https://github.com/MessagePack-CSharp/MessagePack-CSharp/issues/2286: + /// OldSpec must not emit str8 (0xD9) for lengths 32-255. + /// + [Theory] + [InlineData(31)] // fixstr + [InlineData(32)] // str16 under OldSpec (would be str8 otherwise) + [InlineData(255)] // str16 under OldSpec (would be str8 otherwise) + [InlineData(256)] // str16 + public void Write_ByteArray_OldSpec_AvoidsStr8(int length) + { + byte[] value = new byte[length]; + for (int i = 0; i < value.Length; i++) + { + value[i] = (byte)'A'; + } + + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence) { OldSpec = true }; + writer.Write(value); + writer.Flush(); + + ReadOnlySpan written = sequence.AsReadOnlySequence.ToArray(); + Assert.NotEqual(MessagePackCode.Str8, written[0]); + + if (length <= MessagePackRange.MaxFixStringLength) + { + Assert.Equal((byte)(MessagePackCode.MinFixStr | length), written[0]); + Assert.Equal(value, written.Slice(1).ToArray()); + } + else if (length <= ushort.MaxValue) + { + Assert.Equal(MessagePackCode.Str16, written[0]); + Assert.Equal((byte)(length >> 8), written[1]); + Assert.Equal((byte)length, written[2]); + Assert.Equal(value, written.Slice(3).ToArray()); + } + } + + [Theory] + [InlineData(32)] + [InlineData(255)] + public void WriteStringHeader_OldSpec_AvoidsStr8(int length) + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence) { OldSpec = true }; + writer.WriteStringHeader(length); + writer.Flush(); + + ReadOnlySpan written = sequence.AsReadOnlySequence.ToArray(); + Assert.Equal(new byte[] { MessagePackCode.Str16, (byte)(length >> 8), (byte)length }, written.ToArray()); + } + + [Theory] + [InlineData(32)] + [InlineData(255)] + public void WriteBinHeader_OldSpec_AvoidsStr8(int length) + { + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence) { OldSpec = true }; + writer.WriteBinHeader(length); + writer.Flush(); + + ReadOnlySpan written = sequence.AsReadOnlySequence.ToArray(); + Assert.Equal(new byte[] { MessagePackCode.Str16, (byte)(length >> 8), (byte)length }, written.ToArray()); + } + [Fact] public void WriteExtensionFormatHeader_NegativeExtension() { diff --git a/tests/MessagePack.Tests/OldSpecBinaryFormatterTest.cs b/tests/MessagePack.Tests/OldSpecBinaryFormatterTest.cs index 748451f4d..78d1473a7 100644 --- a/tests/MessagePack.Tests/OldSpecBinaryFormatterTest.cs +++ b/tests/MessagePack.Tests/OldSpecBinaryFormatterTest.cs @@ -23,6 +23,8 @@ public class OldSpecBinaryFormatterTest { [Theory] [InlineData(10)] // fixstr + [InlineData(32)] // str 16 (must not use str8 under OldSpec) + [InlineData(255)] // str 16 (must not use str8 under OldSpec) [InlineData(1000)] // str 16 [InlineData(100000)] // str 32 public void SerializeSimpleByteArray(int arrayLength) @@ -57,6 +59,8 @@ public void SerializeNil() [Theory] [InlineData(10)] // fixstr + [InlineData(32)] // str 16 (must not use str8 under OldSpec) + [InlineData(255)] // str 16 (must not use str8 under OldSpec) [InlineData(1000)] // str 16 [InlineData(100000)] // str 32 public void SerializeObject(int arrayLength) @@ -78,6 +82,8 @@ public void SerializeObject(int arrayLength) [Theory] [InlineData(10)] // fixstr + [InlineData(32)] // str 16 (must not use str8 under OldSpec) + [InlineData(255)] // str 16 (must not use str8 under OldSpec) [InlineData(1000)] // str 16 [InlineData(100000)] // str 32 public void DeserializeSimpleByteArray(int arrayLength) @@ -103,6 +109,8 @@ public void DeserializeNil() [Theory] [InlineData(10)] // fixstr + [InlineData(32)] // str 16 (must not use str8 under OldSpec) + [InlineData(255)] // str 16 (must not use str8 under OldSpec) [InlineData(1000)] // str 16 [InlineData(100000)] // str 32 public void DeserializeObject(int arrayLength) diff --git a/tests/MessagePack.Tests/PrimitiveObjectFormatterTests.cs b/tests/MessagePack.Tests/PrimitiveObjectFormatterTests.cs index 83761ba63..cc20b9b5a 100644 --- a/tests/MessagePack.Tests/PrimitiveObjectFormatterTests.cs +++ b/tests/MessagePack.Tests/PrimitiveObjectFormatterTests.cs @@ -3,11 +3,13 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Text; using System.Threading.Tasks; using MessagePack.Formatters; using MessagePack.Resolvers; +using Nerdbank.Streams; using Xunit; namespace MessagePack.Tests @@ -58,6 +60,33 @@ public void EnumRetainsUnderlyingType() Assert.Equal(SomeEnum.SomeValue, result); } + [Fact] + [Trait("CWE", "789")] + public void NestedArraysCannotReuseTrailingBytesToJustifyAllocations() + { + const int arrayLength = 1000; + const int nestingDepth = 10; + var sequence = new Sequence(); + var writer = new MessagePackWriter(sequence); + for (int i = 0; i < nestingDepth; i++) + { + writer.WriteArrayHeader(arrayLength); + } + + for (int i = 0; i < arrayLength; i++) + { + writer.WriteNil(); + } + + writer.Flush(); + + MessagePackSerializationException exception = Assert.Throws( + () => MessagePackSerializer.Deserialize( + sequence.AsReadOnlySequence, + ContractlessStandardResolver.Options.WithSecurity(MessagePackSecurity.UntrustedData))); + Assert.IsType(exception.InnerException); + } + public enum SomeEnum : ushort { None = 0,