From f24c6a08dd386c8833874e8622e2ee49a39b864d Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Mon, 13 Feb 2023 23:09:50 -0800 Subject: [PATCH] fix: Fixes string interpolation in value string builder (#1339) --- .../Tests/Buffers/ValueStringBuilderTests.cs | 62 ++++--- Projects/Server/Buffers/ValueStringBuilder.cs | 172 ++---------------- .../Buffers/ValueStringBuilderExtensions.cs | 32 ---- Projects/Server/Client/ClientVersion.cs | 2 +- .../Server/Localization/LocalizationEntry.cs | 15 +- Projects/UOContent/Compression/TarArchive.cs | 15 +- Projects/UOContent/Items/Misc/Teleporter.cs | 2 +- Projects/UOContent/Misc/ClientVerification.cs | 26 +-- Projects/UOContent/Skills/ForensicEval.cs | 3 +- 9 files changed, 87 insertions(+), 242 deletions(-) delete mode 100644 Projects/Server/Buffers/ValueStringBuilderExtensions.cs diff --git a/Projects/Server.Tests/Tests/Buffers/ValueStringBuilderTests.cs b/Projects/Server.Tests/Tests/Buffers/ValueStringBuilderTests.cs index 462b318e1..7e5c17795 100644 --- a/Projects/Server.Tests/Tests/Buffers/ValueStringBuilderTests.cs +++ b/Projects/Server.Tests/Tests/Buffers/ValueStringBuilderTests.cs @@ -1,34 +1,48 @@ using Server.Buffers; using Xunit; -namespace Server.Tests.Buffers +namespace Server.Tests.Buffers; + +public class ValueStringBuilderTests { - public class ValueStringBuilderTests + [Theory] + [InlineData("Admin Kamron", "Kamron", 0, 6)] + [InlineData("Admin Kamron", "Admin ron", 6, 3)] + [InlineData("Admin Kamron", "Admin", 5, 7)] + public void TestRemove(string original, string removed, int startIndex, int length) { - [Theory] - [InlineData("Admin Kamron", "Kamron", 0, 6)] - [InlineData("Admin Kamron", "Admin ron", 6, 3)] - [InlineData("Admin Kamron", "Admin", 5, 7)] - public void TestRemove(string original, string removed, int startIndex, int length) - { - using var sb = new ValueStringBuilder(stackalloc char[64]); - sb.Append(original); - sb.Remove(startIndex, length); + using var sb = new ValueStringBuilder(stackalloc char[64]); + sb.Append(original); + sb.Remove(startIndex, length); - Assert.Equal(removed, sb.ToString()); - } + Assert.Equal(removed, sb.ToString()); + } - [Theory] - [InlineData(8)] - [InlineData(9876)] - [InlineData(-5)] - [InlineData(-130984209)] - public void TestAppendInt32(int value) - { - using var sb = new ValueStringBuilder(stackalloc char[64]); - sb.Append(value); + [Theory] + [InlineData(8)] + [InlineData(9876)] + [InlineData(-5)] + [InlineData(-130984209)] + public void TestAppendInt32(int value) + { + using var sb = new ValueStringBuilder(stackalloc char[64]); + sb.Append(value); - Assert.Equal(value.ToString(), sb.ToString()); - } + Assert.Equal(value.ToString(), sb.ToString()); + } + + [Theory] + [InlineData("Kamron")] + [InlineData("")] + [InlineData(5)] + [InlineData(-30.6)] + public void TestAppendInterpolation(object value) + { + var sb = ValueStringBuilder.Create(); + sb.Append( $"Hi, this is {value}."); + sb.Append(" I am a string."); + + Assert.Equal($"Hi, this is {value}. I am a string.", sb.ToString()); + sb.Dispose(); } } diff --git a/Projects/Server/Buffers/ValueStringBuilder.cs b/Projects/Server/Buffers/ValueStringBuilder.cs index b5a0e8c24..6b7746b24 100644 --- a/Projects/Server/Buffers/ValueStringBuilder.cs +++ b/Projects/Server/Buffers/ValueStringBuilder.cs @@ -208,6 +208,17 @@ public ref struct ValueStringBuilder } } + // Compiler generated + public void Append(ref RawInterpolatedStringHandler handler) => Append(handler.Text); + + // Compiler generated + public void Append( + IFormatProvider? formatProvider, + [InterpolatedStringHandlerArgument("formatProvider")] + ref RawInterpolatedStringHandler handler + ) => Append(handler.Text); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] public void Append(string? s) { @@ -450,165 +461,4 @@ public ref struct ValueStringBuilder _length -= length; } - - /// Provides a handler used by the language compiler to append interpolated strings into instances. - [InterpolatedStringHandler] - public ref struct AppendInterpolatedStringHandler - { - // Implementation note: - // As this type is only intended to be targeted by the compiler, public APIs eschew argument validation logic - // in a variety of places, e.g. allowing a null input when one isn't expected to produce a NullReferenceException rather - // than an ArgumentNullException. - - /// The associated StringBuilder to which to append. - internal ValueStringBuilder _stringBuilder; - - /// Creates a handler used to append an interpolated string into a . - /// The number of constant characters outside of interpolation expressions in the interpolated string. - /// The number of interpolation expressions in the interpolated string. - /// The associated StringBuilder to which to append. - /// This is intended to be called only by compiler-generated code. Arguments are not validated as they'd otherwise be for members intended to be used directly. - public AppendInterpolatedStringHandler(int literalLength, int formattedCount, ValueStringBuilder stringBuilder) - { - _stringBuilder = stringBuilder; - } - - /// Writes the specified string to the handler. - /// The string to write. - public void AppendLiteral(string value) => _stringBuilder.Append(value); - - // Design note: - // This provides the same set of overloads and semantics as DefaultInterpolatedStringHandler. - - /// Writes the specified value to the handler. - /// The value to write. - public void AppendFormatted(T value) => _stringBuilder.Append(value); - - /// Writes the specified value to the handler. - /// The value to write. - /// The format string. - public void AppendFormatted(T value, string? format) => _stringBuilder.Append(value, format); - - /// Writes the specified value to the handler. - /// The value to write. - /// Minimum number of characters that should be written for this value. If the value is negative, it indicates left-aligned and the required minimum is the absolute value. - public void AppendFormatted(T value, int alignment) => - AppendFormatted(value, alignment, format: null); - - /// Writes the specified value to the handler. - /// The value to write. - /// The format string. - /// Minimum number of characters that should be written for this value. If the value is negative, it indicates left-aligned and the required minimum is the absolute value. - public void AppendFormatted(T value, int alignment, string? format) - { - if (alignment == 0) - { - // This overload is used as a fallback from several disambiguation overloads, so special-case 0. - AppendFormatted(value, format); - } - else if (alignment < 0) - { - // Left aligned: format into the handler, then append any additional padding required. - int start = _stringBuilder.Length; - AppendFormatted(value, format); - int paddingRequired = -alignment - (_stringBuilder.Length - start); - if (paddingRequired > 0) - { - _stringBuilder.Append(' ', paddingRequired); - } - } - else - { - var startingPos = _stringBuilder._length; - AppendFormatted(value, format); - - InsertAlignment(startingPos, alignment); - } - } - - /// Writes the specified character span to the handler. - /// The span to write. - public void AppendFormatted(ReadOnlySpan value) => _stringBuilder.Append(value); - - /// Writes the specified string of chars to the handler. - /// The span to write. - /// Minimum number of characters that should be written for this value. If the value is negative, it indicates left-aligned and the required minimum is the absolute value. - /// The format string. - public void AppendFormatted(ReadOnlySpan value, int alignment = 0, string? format = null) - { - if (alignment == 0) - { - _stringBuilder.Append(value); - } - else - { - bool leftAlign = false; - if (alignment < 0) - { - leftAlign = true; - alignment = -alignment; - } - - int paddingRequired = alignment - value.Length; - if (paddingRequired <= 0) - { - _stringBuilder.Append(value); - } - else if (leftAlign) - { - _stringBuilder.Append(value); - _stringBuilder.Append(' ', paddingRequired); - } - else - { - _stringBuilder.Append(' ', paddingRequired); - _stringBuilder.Append(value); - } - } - } - - /// Writes the specified value to the handler. - /// The value to write. - public void AppendFormatted(string? value) => _stringBuilder.Append(value); - - /// Writes the specified value to the handler. - /// The value to write. - /// Minimum number of characters that should be written for this value. If the value is negative, it indicates left-aligned and the required minimum is the absolute value. - /// The format string. - public void AppendFormatted(string? value, int alignment = 0, string? format = null) => - // Format is meaningless for strings and doesn't make sense for someone to specify. We have the overload - // simply to disambiguate between ROS and object, just in case someone does specify a format, as - // string is implicitly convertible to both. Just delegate to the T-based implementation. - AppendFormatted(value, alignment, format); - - /// Writes the specified value to the handler. - /// The value to write. - /// Minimum number of characters that should be written for this value. If the value is negative, it indicates left-aligned and the required minimum is the absolute value. - /// The format string. - public void AppendFormatted(object? value, int alignment = 0, string? format = null) => - // This overload is expected to be used rarely, only if either a) something strongly typed as object is - // formatted with both an alignment and a format, or b) the compiler is unable to target type to T. It - // exists purely to help make cases from (b) compile. Just delegate to the T-based implementation. - AppendFormatted(value, alignment, format); - - private void InsertAlignment(int startingPos, int alignment) - { - var charsWritten = _stringBuilder._length - startingPos; - - var paddingNeeded = alignment - charsWritten; - if (paddingNeeded > 0) - { - var chars = _stringBuilder._chars; - if (chars.Length - _stringBuilder._length < paddingNeeded) - { - _stringBuilder.Grow(paddingNeeded); - } - - chars.Slice(startingPos, charsWritten).CopyTo(chars[(startingPos + paddingNeeded)..]); - chars.Slice(startingPos, paddingNeeded).Fill(' '); - - _stringBuilder._length += paddingNeeded; - } - } - } } diff --git a/Projects/Server/Buffers/ValueStringBuilderExtensions.cs b/Projects/Server/Buffers/ValueStringBuilderExtensions.cs deleted file mode 100644 index 71d515d8b..000000000 --- a/Projects/Server/Buffers/ValueStringBuilderExtensions.cs +++ /dev/null @@ -1,32 +0,0 @@ -/************************************************************************* - * ModernUO * - * Copyright 2019-2022 - ModernUO Development Team * - * Email: hi@modernuo.com * - * File: ValueStringBuilderExtensions.cs * - * * - * This program is free software: you can redistribute it and/or modify * - * it under the terms of the GNU General Public License as published by * - * the Free Software Foundation, either version 3 of the License, or * - * (at your option) any later version. * - * * - * You should have received a copy of the GNU General Public License * - * along with this program. If not, see . * - *************************************************************************/ - -using System.Runtime.CompilerServices; - -namespace Server.Buffers; - -public static class ValueStringBuilderExtensions -{ - // Compiler generated - public static void Append( - // ReSharper disable once RedundantAssignment - this ref ValueStringBuilder stringBuilder, - [InterpolatedStringHandlerArgument("stringBuilder")] - ref ValueStringBuilder.AppendInterpolatedStringHandler handler) - { - // Reassign since the string builder stored on the interpolated handler is by-value - stringBuilder = handler._stringBuilder; - } -} diff --git a/Projects/Server/Client/ClientVersion.cs b/Projects/Server/Client/ClientVersion.cs index f85cfc1b4..0ac0e4a07 100644 --- a/Projects/Server/Client/ClientVersion.cs +++ b/Projects/Server/Client/ClientVersion.cs @@ -213,7 +213,7 @@ public class ClientVersion : IComparable, IComparer 5 || Minor > 0 || Revision > 6) { diff --git a/Projects/Server/Localization/LocalizationEntry.cs b/Projects/Server/Localization/LocalizationEntry.cs index ae4747569..d611222dc 100644 --- a/Projects/Server/Localization/LocalizationEntry.cs +++ b/Projects/Server/Localization/LocalizationEntry.cs @@ -50,37 +50,38 @@ public class LocalizationEntry private static void ParseText(string text, out string[] textSlices, out string stringFormatter) { + var sb = ValueStringBuilder.Create(256); + using var queue = PooledRefQueue.Create(); + bool hasMatch = false; var prevIndex = 0; - var builder = new ValueStringBuilder(stackalloc char[256]); - using var queue = PooledRefQueue.Create(); foreach (Match match in _textRegex.Matches(text)) { if (prevIndex < match.Index) { var substr = text[prevIndex..match.Index]; - builder.Append(substr); + sb.Append(substr); queue.Enqueue(substr); } queue.Enqueue(null); hasMatch = true; - builder.Append($"{{{int.Parse(match.Groups[1].Value) - 1}}}"); + sb.Append($"{{{int.Parse(match.Groups[1].Value) - 1}}}"); prevIndex = match.Index + match.Length; } if (prevIndex < text.Length - 1) { var substr = prevIndex == 0 ? text : text[prevIndex..]; - builder.Append(substr); + sb.Append(substr); queue.Enqueue(substr); } textSlices = queue.ToArray(); - stringFormatter = hasMatch ? builder.ToString() : null; + stringFormatter = hasMatch ? sb.ToString() : null; - builder.Dispose(); + sb.Dispose(); } /// diff --git a/Projects/UOContent/Compression/TarArchive.cs b/Projects/UOContent/Compression/TarArchive.cs index 702866122..babb5e1bc 100755 --- a/Projects/UOContent/Compression/TarArchive.cs +++ b/Projects/UOContent/Compression/TarArchive.cs @@ -96,13 +96,22 @@ namespace Server.Compression new FileInfo(destinationArchiveFileName).EnsureDirectory(); - using var builder = ValueStringBuilder.Create(); + var sb = ValueStringBuilder.Create(); var i = 0; foreach (var path in paths) { - builder.Append($"{(i++ > 0 ? " " : "")}\"{Path.GetRelativePath(relativeTo, path)}\""); + var relativePath = Path.GetRelativePath(relativeTo, path); + if (i++ > 0) + { + sb.Append($" \"{relativePath}\""); + } + else + { + sb.Append($"\"{relativePath}\""); + } } - var pathsToCompress = builder.ToString(); + var pathsToCompress = sb.ToString(); + sb.Dispose(); var tarFlags = compressCommand == null ? "-acf" : "-cf"; var useExternalCompression = compressCommand != null ? $"--use-compress-program \"{compressCommand}\" " : ""; diff --git a/Projects/UOContent/Items/Misc/Teleporter.cs b/Projects/UOContent/Items/Misc/Teleporter.cs index 2789afca9..047390984 100644 --- a/Projects/UOContent/Items/Misc/Teleporter.cs +++ b/Projects/UOContent/Items/Misc/Teleporter.cs @@ -939,7 +939,7 @@ public partial class ConditionTeleporter : Teleporter { base.GetProperties(list); - using var props = new ValueStringBuilder(stackalloc char[128]); + using var props = ValueStringBuilder.Create(128); if (GetFlag(ConditionFlag.DenyMounted)) { diff --git a/Projects/UOContent/Misc/ClientVerification.cs b/Projects/UOContent/Misc/ClientVerification.cs index 992c0fb59..80b8d9a81 100644 --- a/Projects/UOContent/Misc/ClientVerification.cs +++ b/Projects/UOContent/Misc/ClientVerification.cs @@ -83,7 +83,7 @@ namespace Server.Misc private static void EventSink_ClientVersionReceived(NetState state, ClientVersion version) { - using var message = ValueStringBuilder.Create(); + var sb = ValueStringBuilder.Create(); if (!_enable || state.Mobile?.AccessLevel != AccessLevel.Player) { @@ -100,47 +100,47 @@ namespace Server.Misc if (MinRequired != null && version < MinRequired) { - message.Append($"This server doesn't support clients older than {MinRequired}."); + sb.Append($"This server doesn't support clients older than {MinRequired}."); shouldKick = strictRequirement; } else if (MaxRequired != null && version > MaxRequired) { - message.Append($"This server doesn't support clients newer than {MaxRequired}."); + sb.Append($"This server doesn't support clients newer than {MaxRequired}."); shouldKick = strictRequirement; } else if (!AllowRegular || !AllowUOTD) { if (!AllowRegular && version.Type == ClientType.Regular) { - message.Append("This server does not allow regular clients to connect."); + sb.Append("This server does not allow regular clients to connect."); shouldKick = true; } else if (!AllowUOTD && state.IsUOTDClient) { - message.Append("This server does not allow UO:TD clients to connect."); + sb.Append("This server does not allow UO:TD clients to connect."); shouldKick = true; } - if (message.Length > 0) + if (sb.Length > 0) { if (AllowRegular && AllowUOTD) { - message.Append(" You can use regular or UO:TD clients."); + sb.Append(" You can use regular or UO:TD clients."); } else if (AllowRegular) { - message.Append(" You can use regular clients."); + sb.Append(" You can use regular clients."); } else if (AllowUOTD) { - message.Append(" You can use UO:TD clients."); + sb.Append(" You can use UO:TD clients."); } } } - if (message.Length > 0) + if (sb.Length > 0) { - state.Mobile.SendMessage(0x22, message.ToString()); + state.Mobile.SendMessage(0x22, sb.ToString()); } if (shouldKick) @@ -150,7 +150,7 @@ namespace Server.Misc return; } - if (message.Length > 0) + if (sb.Length > 0) { switch (_invalidClientResponse) { @@ -170,6 +170,8 @@ namespace Server.Misc } } } + + sb.Dispose(); } private static void OnKick(NetState ns) diff --git a/Projects/UOContent/Skills/ForensicEval.cs b/Projects/UOContent/Skills/ForensicEval.cs index 1b82311fa..de462162e 100644 --- a/Projects/UOContent/Skills/ForensicEval.cs +++ b/Projects/UOContent/Skills/ForensicEval.cs @@ -71,7 +71,7 @@ namespace Server.SkillHandlers if (c.Looters.Count > 0) { - using var sb = new ValueStringBuilder(stackalloc char[128]); + var sb = ValueStringBuilder.Create(128); int i = 0; foreach (var looter in c.Looters) { @@ -93,6 +93,7 @@ namespace Server.SkillHandlers // This body has been disturbed by ~1_PLAYER_NAMES~ from.SendLocalizedMessage(1042752, sb.ToString()); + sb.Dispose(); } else {