From d81e8d48fe53ccfbbda406da9097e349caa99a8f Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 10 Sep 2019 14:26:35 -0700 Subject: [PATCH 1/5] Avoid MemoryMarshal.Cast when transcoding from UTF-16 to UTF-8 while escaping in Utf8JsonWriter. --- .../Json/Writer/JsonWriterHelper.Escaping.cs | 7 ++- .../tests/Serialization/Value.WriteTests.cs | 12 +++++ .../tests/Utf8JsonWriterTests.cs | 48 +++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs b/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs index aa74e144612b..adf895a233de 100644 --- a/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs +++ b/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs @@ -79,13 +79,16 @@ public static int NeedsEscaping(ReadOnlySpan value, JavaScriptEncoder enco return idx; } - public static int NeedsEscaping(ReadOnlySpan value, JavaScriptEncoder encoder) + public static unsafe int NeedsEscaping(ReadOnlySpan value, JavaScriptEncoder encoder) { int idx; if (encoder != null) { - idx = encoder.FindFirstCharacterToEncodeUtf8(MemoryMarshal.Cast(value)); + fixed (char* ptr = value) + { + idx = encoder.FindFirstCharacterToEncode(ptr, value.Length); + } goto Return; } diff --git a/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs b/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs index 8cd539338b0e..b024e7526718 100644 --- a/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs +++ b/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs @@ -22,6 +22,18 @@ public static void WriteStringWithRelaxedEscaper() Assert.NotEqual(expected, JsonSerializer.Serialize(inputString)); } + // https://github.com/dotnet/corefx/issues/40979 + [Fact] + public static void EscapingShouldntStackOverflow_40979() + { + var test = new { Name = "\u6D4B\u8A6611" }; + + var options = new JsonSerializerOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping, PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + string result = JsonSerializer.Serialize(test, options); + + Assert.Equal("{\"name\": \"\u6D4B\u8A6611\"}", result); + } + [Fact] public static void WritePrimitives() { diff --git a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs index 772c89f75a94..a5098d531bb1 100644 --- a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs +++ b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs @@ -56,6 +56,54 @@ public void CantWriteToNonWritableStream(bool formatted, bool skipValidation) Assert.Throws(() => new Utf8JsonWriter(stream, options)); } + [Fact] + public static void WritingStringsWithCustomEscaping() + { + var output = new ArrayBufferWriter(); + var writerOptions = new JsonWriterOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; + + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue("\u6D4B\u8A6611"); + } + JsonTestHelper.AssertContents("\"\\u6D4B\\u8A6611\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue("\u6D4B\u8A6611"); + } + JsonTestHelper.AssertContents("\"\u6D4B\u8A6611\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue("\u00E9\""); + } + JsonTestHelper.AssertContents("\"\\u00E9\\u0022\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue("\u00E9\""); + } + JsonTestHelper.AssertContents("\"\u00E9\\\"\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue("\u2020\""); + } + JsonTestHelper.AssertContents("\"\\u2020\\u0022\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue("\u2020\""); + } + JsonTestHelper.AssertContents("\"\u2020\\\"\"", output); + } + [Fact] public void WriteJsonWritesToIBWOnDemand_Dispose() { From 6f05ffa8249b9c6cdfa0f4cdfd40e9ecdcbc2b10 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 10 Sep 2019 14:35:36 -0700 Subject: [PATCH 2/5] Fix a typo in spacing within the test. --- src/System.Text.Json/tests/Serialization/Value.WriteTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs b/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs index b024e7526718..e7193f57b677 100644 --- a/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs +++ b/src/System.Text.Json/tests/Serialization/Value.WriteTests.cs @@ -31,7 +31,7 @@ public static void EscapingShouldntStackOverflow_40979() var options = new JsonSerializerOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping, PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; string result = JsonSerializer.Serialize(test, options); - Assert.Equal("{\"name\": \"\u6D4B\u8A6611\"}", result); + Assert.Equal("{\"name\":\"\u6D4B\u8A6611\"}", result); } [Fact] From 11e0f499ee3d68003429974a77ca4e700fc879b6 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 10 Sep 2019 16:22:23 -0700 Subject: [PATCH 3/5] Guard against empty spans where an implementation of JavascriptEncoder might not handle null ptrs correctly. --- .../Json/Writer/JsonWriterHelper.Escaping.cs | 4 +- .../tests/Utf8JsonWriterTests.cs | 151 ++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs b/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs index adf895a233de..a41035bb51c6 100644 --- a/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs +++ b/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs @@ -83,7 +83,9 @@ public static unsafe int NeedsEscaping(ReadOnlySpan value, JavaScriptEncod { int idx; - if (encoder != null) + // Some implementations of JavascriptEncoder.FindFirstCharacterToEncode may not accept + // null pointers and gaurd against that. Hence, check up-front and fall down to return -1. + if (encoder != null && !value.IsEmpty) { fixed (char* ptr = value) { diff --git a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs index a5098d531bb1..9b9dd53bc618 100644 --- a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs +++ b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs @@ -56,6 +56,157 @@ public void CantWriteToNonWritableStream(bool formatted, bool skipValidation) Assert.Throws(() => new Utf8JsonWriter(stream, options)); } + [Fact] + public static void WritingNullStringsWithCustomEscaping() + { + var output = new ArrayBufferWriter(); + var writerOptions = new JsonWriterOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; + + string str = null; + + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(str); + } + JsonTestHelper.AssertContents("null", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(str); + } + JsonTestHelper.AssertContents("null", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + byte[] utf8Str = null; + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(utf8Str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(utf8Str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + JsonEncodedText jsonText = JsonEncodedText.Encode(utf8Str.AsSpan()); + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(jsonText); + } + JsonTestHelper.AssertContents("\"\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(jsonText); + } + JsonTestHelper.AssertContents("\"\"", output); + } + + [Fact] + public static void WritingNullStringsWithBuggyJavascriptEncoder() + { + var output = new ArrayBufferWriter(); + var writerOptions = new JsonWriterOptions { Encoder = new BuggyJavaScriptEncoder() }; + + string str = null; + + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(str); + } + JsonTestHelper.AssertContents("null", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(str); + } + JsonTestHelper.AssertContents("null", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + byte[] utf8Str = null; + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(utf8Str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(utf8Str.AsSpan()); + } + JsonTestHelper.AssertContents("\"\"", output); + + JsonEncodedText jsonText = JsonEncodedText.Encode(utf8Str.AsSpan()); + output.Clear(); + using (var writer = new Utf8JsonWriter(output)) + { + writer.WriteStringValue(jsonText); + } + JsonTestHelper.AssertContents("\"\"", output); + + output.Clear(); + using (var writer = new Utf8JsonWriter(output, writerOptions)) + { + writer.WriteStringValue(jsonText); + } + JsonTestHelper.AssertContents("\"\"", output); + } + + public class BuggyJavaScriptEncoder : JavaScriptEncoder + { + public override int MaxOutputCharactersPerInputCharacter => throw new NotImplementedException(); + + public override unsafe int FindFirstCharacterToEncode(char* text, int textLength) + { + // Access the text pointer even though it might be null and text length is 0. + return *text; + } + + public override unsafe bool TryEncodeUnicodeScalar(int unicodeScalar, char* buffer, int bufferLength, out int numberOfCharactersWritten) + { + numberOfCharactersWritten = 0; + return false; + } + + public override bool WillEncode(int unicodeScalar) => false; + } + [Fact] public static void WritingStringsWithCustomEscaping() { From 8a39f3167f81baee0b8dfee957a905e8752e459a Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 10 Sep 2019 16:35:50 -0700 Subject: [PATCH 4/5] Cleanup tests to avoid some duplication. --- .../tests/Utf8JsonWriterTests.cs | 67 ++----------------- 1 file changed, 6 insertions(+), 61 deletions(-) diff --git a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs index 9b9dd53bc618..341fcef6cd0e 100644 --- a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs +++ b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs @@ -59,75 +59,20 @@ public void CantWriteToNonWritableStream(bool formatted, bool skipValidation) [Fact] public static void WritingNullStringsWithCustomEscaping() { - var output = new ArrayBufferWriter(); var writerOptions = new JsonWriterOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; - - string str = null; - - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(str); - } - JsonTestHelper.AssertContents("null", output); - - output.Clear(); - using (var writer = new Utf8JsonWriter(output, writerOptions)) - { - writer.WriteStringValue(str); - } - JsonTestHelper.AssertContents("null", output); - - output.Clear(); - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(str.AsSpan()); - } - JsonTestHelper.AssertContents("\"\"", output); - - output.Clear(); - using (var writer = new Utf8JsonWriter(output, writerOptions)) - { - writer.WriteStringValue(str.AsSpan()); - } - JsonTestHelper.AssertContents("\"\"", output); - - byte[] utf8Str = null; - output.Clear(); - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(utf8Str.AsSpan()); - } - JsonTestHelper.AssertContents("\"\"", output); - - output.Clear(); - using (var writer = new Utf8JsonWriter(output, writerOptions)) - { - writer.WriteStringValue(utf8Str.AsSpan()); - } - JsonTestHelper.AssertContents("\"\"", output); - - JsonEncodedText jsonText = JsonEncodedText.Encode(utf8Str.AsSpan()); - output.Clear(); - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(jsonText); - } - JsonTestHelper.AssertContents("\"\"", output); - - output.Clear(); - using (var writer = new Utf8JsonWriter(output, writerOptions)) - { - writer.WriteStringValue(jsonText); - } - JsonTestHelper.AssertContents("\"\"", output); + WriteNullStringsHelper(writerOptions); } [Fact] public static void WritingNullStringsWithBuggyJavascriptEncoder() { - var output = new ArrayBufferWriter(); var writerOptions = new JsonWriterOptions { Encoder = new BuggyJavaScriptEncoder() }; + WriteNullStringsHelper(writerOptions); + } + private static void WriteNullStringsHelper(JsonWriterOptions writerOptions) + { + var output = new ArrayBufferWriter(); string str = null; using (var writer = new Utf8JsonWriter(output)) From f7e74f107230d0ef5053d6cbe01660d01f8c8fc4 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 10 Sep 2019 16:41:16 -0700 Subject: [PATCH 5/5] Some more test clean up. --- .../tests/Utf8JsonWriterTests.cs | 36 ++++--------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs index 341fcef6cd0e..1c56b3fe5b0d 100644 --- a/src/System.Text.Json/tests/Utf8JsonWriterTests.cs +++ b/src/System.Text.Json/tests/Utf8JsonWriterTests.cs @@ -59,7 +59,13 @@ public void CantWriteToNonWritableStream(bool formatted, bool skipValidation) [Fact] public static void WritingNullStringsWithCustomEscaping() { - var writerOptions = new JsonWriterOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; + var writerOptions = new JsonWriterOptions(); + WriteNullStringsHelper(writerOptions); + + writerOptions = new JsonWriterOptions { Encoder = JavaScriptEncoder.Default }; + WriteNullStringsHelper(writerOptions); + + writerOptions = new JsonWriterOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; WriteNullStringsHelper(writerOptions); } @@ -75,26 +81,12 @@ private static void WriteNullStringsHelper(JsonWriterOptions writerOptions) var output = new ArrayBufferWriter(); string str = null; - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(str); - } - JsonTestHelper.AssertContents("null", output); - - output.Clear(); using (var writer = new Utf8JsonWriter(output, writerOptions)) { writer.WriteStringValue(str); } JsonTestHelper.AssertContents("null", output); - output.Clear(); - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(str.AsSpan()); - } - JsonTestHelper.AssertContents("\"\"", output); - output.Clear(); using (var writer = new Utf8JsonWriter(output, writerOptions)) { @@ -103,13 +95,6 @@ private static void WriteNullStringsHelper(JsonWriterOptions writerOptions) JsonTestHelper.AssertContents("\"\"", output); byte[] utf8Str = null; - output.Clear(); - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(utf8Str.AsSpan()); - } - JsonTestHelper.AssertContents("\"\"", output); - output.Clear(); using (var writer = new Utf8JsonWriter(output, writerOptions)) { @@ -118,13 +103,6 @@ private static void WriteNullStringsHelper(JsonWriterOptions writerOptions) JsonTestHelper.AssertContents("\"\"", output); JsonEncodedText jsonText = JsonEncodedText.Encode(utf8Str.AsSpan()); - output.Clear(); - using (var writer = new Utf8JsonWriter(output)) - { - writer.WriteStringValue(jsonText); - } - JsonTestHelper.AssertContents("\"\"", output); - output.Clear(); using (var writer = new Utf8JsonWriter(output, writerOptions)) {