From 917f9e507312e68c656c72e856458b61f5161e60 Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Mon, 19 Feb 2024 22:05:55 +0100 Subject: [PATCH] Add support for ISpanFormattable in StringBuilder.Append(Object) --- .../src/System/Text/StringBuilder.cs | 22 +++++- .../System/Text/StringBuilderTests.cs | 78 +++++++++++++++++-- 2 files changed, 91 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index c1f86a1ca2d1e..98994b6b40e4c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -1052,12 +1052,19 @@ private void AppendWithExpansion(char value) [CLSCompliant(false)] public StringBuilder Append(ulong value) => AppendSpanFormattable(value); - private StringBuilder AppendSpanFormattable(T value) where T : ISpanFormattable + private StringBuilder AppendSpanFormattable(T value, bool untrusted = false) where T : ISpanFormattable { - Debug.Assert(typeof(T).Assembly.Equals(typeof(object).Assembly), "Implementation trusts the results of TryFormat because T is expected to be something known"); + Debug.Assert(untrusted || typeof(T).Assembly.Equals(typeof(object).Assembly), "Implementation trusts the results of TryFormat because T is expected to be something known"); if (value.TryFormat(RemainingCurrentChunk, out int charsWritten, format: default, provider: null)) { + if (untrusted && ((uint)charsWritten > (uint)RemainingCurrentChunkLength)) + { + // Protect against faulty ISpanFormattable implementations returning invalid charsWritten values. + // Other code in _stringBuilder uses Unsafe manipulation, and we want to ensure m_ChunkLength remains safe. + ThrowHelper.ThrowFormatInvalidString(); + } + m_ChunkLength += charsWritten; return this; } @@ -1078,7 +1085,12 @@ internal StringBuilder AppendSpanFormattable(T value, string? format, IFormat return Append(value.ToString(format, provider)); } - public StringBuilder Append(object? value) => (value == null) ? this : Append(value.ToString()); + public StringBuilder Append(object? value) => value switch + { + null => this, + ISpanFormattable sf => AppendSpanFormattable(sf, untrusted: true), + object o => Append(o.ToString()) + }; public StringBuilder Append(char[]? value) { @@ -1620,7 +1632,7 @@ internal StringBuilder AppendFormatHelper(IFormatProvider? provider, string form arg is ISpanFormattable spanFormattableArg && spanFormattableArg.TryFormat(RemainingCurrentChunk, out int charsWritten, itemFormatSpan, provider)) { - if ((uint)charsWritten > (uint)RemainingCurrentChunk.Length) + if ((uint)charsWritten > (uint)RemainingCurrentChunkLength) { // Untrusted ISpanFormattable implementations might return an erroneous charsWritten value, // and m_ChunkLength might end up being used in Unsafe code, so fail if we get back an @@ -2423,6 +2435,8 @@ private Span RemainingCurrentChunk get => new Span(m_ChunkChars, m_ChunkLength, m_ChunkChars.Length - m_ChunkLength); } + private int RemainingCurrentChunkLength => m_ChunkChars.Length - m_ChunkLength; + /// /// Finds the chunk that logically succeeds the specified chunk. /// diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Text/StringBuilderTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Text/StringBuilderTests.cs index 2d1f569dd103b..3dc5ba76fb5b7 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Text/StringBuilderTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Text/StringBuilderTests.cs @@ -378,18 +378,86 @@ public static void Append_Long_NoSpareCapacity_ThrowsArgumentOutOfRangeException AssertExtensions.Throws(s_noCapacityParamName, () => builder.Append((long)1)); } + public static IEnumerable Append_Object_TestData() + { + yield return new object[] { "Hello", "abc", "Helloabc" }; + yield return new object[] { "", "g", "g" }; + yield return new object[] { "Hello", "", "Hello" }; + yield return new object[] { "Hello", null, "Hello" }; + yield return new object[] { "Hello", $"abc", "Helloabc" }; + // ISpanFormattable inputs: simple validation of known types that implement the interface + yield return new object[] { "", (byte)42, "42" }; + yield return new object[] { "", 'A', "A" }; + yield return new object[] { "", DateTime.ParseExact("2021-03-15T14:52:51.5058563Z", "o", null, DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal), "3/15/2021 2:52:51 PM" }; + yield return new object[] { "", DateTimeOffset.ParseExact("2021-03-15T14:52:51.5058563Z", "o", null, DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal), "3/15/2021 2:52:51 PM +00:00" }; + yield return new object[] { "", (decimal)42, "42" }; + yield return new object[] { "", (double)42, "42" }; + yield return new object[] { "", Guid.Parse("68d9cfaf-feab-4d5b-96d8-a3fd889ae89f"), "68d9cfaf-feab-4d5b-96d8-a3fd889ae89f" }; + yield return new object[] { "", (Half)42, "42" }; + yield return new object[] { "", (short)42, "42" }; + yield return new object[] { "", (int)42, "42" }; + yield return new object[] { "", (long)42, "42" }; + yield return new object[] { "", (IntPtr)42, "42" }; + yield return new object[] { "", new Rune('A'), "A" }; + yield return new object[] { "", (sbyte)42, "42" }; + yield return new object[] { "", (float)42, "42" }; + yield return new object[] { "", TimeSpan.FromSeconds(42), "00:00:42" }; + yield return new object[] { "", (ushort)42, "42" }; + yield return new object[] { "", (uint)42, "42" }; + yield return new object[] { "", (ulong)42, "42" }; + yield return new object[] { "", (UIntPtr)42, "42" }; + yield return new object[] { "", new Version(1, 2, 3, 4), "1.2.3.4" }; + // ISpanFormattable inputs: advanced validation to ensure that ToString() is not called by default. + yield return new object[] { "Hello", new FormattableStringWithSpanOnly("abc"), "Helloabc" }; + yield return new object[] { "Hello", $"{new FormattableStringWithSpanOnly("abc")}", "Helloabc" }; + } + + private struct FormattableStringWithSpanOnly : ISpanFormattable + { + private string _value; + + public FormattableStringWithSpanOnly(string value) => _value = value; + + public override readonly string ToString() => throw new NotImplementedException(); + public readonly string ToString(string? format, IFormatProvider? formatProvider) => ToString(); + + public bool TryFormat(Span destination, out int charsWritten, ReadOnlySpan format, IFormatProvider? provider) => + destination.TryWrite($"{_value}", out charsWritten); + } + [Theory] - [InlineData("Hello", "abc", "Helloabc")] - [InlineData("Hello", "def", "Hellodef")] - [InlineData("", "g", "g")] - [InlineData("Hello", "", "Hello")] - [InlineData("Hello", null, "Hello")] + [MemberData(nameof(Append_Object_TestData))] public static void Append_Object(string original, object value, string expected) { var builder = new StringBuilder(original); builder.Append(value); Assert.Equal(expected, builder.ToString()); } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Append_Object_InvalidTryFormatCharsWritten_Throws(bool tooBig) // vs tooSmall + { + var builder = new StringBuilder(); + Assert.Throws(() => builder.Append(new InvalidCharsWritten(tooBig))); + } + + private sealed class InvalidCharsWritten : ISpanFormattable + { + private bool _tooBig; + + public InvalidCharsWritten(bool tooBig) => _tooBig = tooBig; + + public bool TryFormat(Span destination, out int charsWritten, ReadOnlySpan format, IFormatProvider provider) + { + charsWritten = _tooBig ? destination.Length + 1 : -1; + return true; + } + + public string ToString(string format, IFormatProvider formatProvider) => + throw new NotImplementedException(); + } [Fact] public static void Append_Object_NoSpareCapacity_ThrowsArgumentOutOfRangeException()