fix: json string serializer improperly escaping characters (#6005)

* feat: escape json string according to RFC8259

* fix: do not append newline when json serde is in compact mode

* fix: json tests

1. Use the same locale settings as the apps
2. Added additional test case for a string that are known to be prone to locale issues
3. Removed test for escaping non-BMP characters to UTF-16 escape sequences

* chore: add more test cases to `JSONTest.testUtf8`

* chore: order cases in the same order as RFC8259

libtransmission/utils.cc

@@ -60,18 +60,32 @@ time_t libtransmission::detail::tr_time::current_time = {};
 
 // ---
 
-void tr_locale_set_global(char const* locale_name) noexcept
+std::optional<std::locale> tr_locale_set_global(char const* locale_name) noexcept
 {
     try
     {
-        std::locale::global(std::locale{ locale_name });
+        return tr_locale_set_global(std::locale{ locale_name });
+    }
+    catch (std::runtime_error const&)
+    {
+        return {};
+    }
+}
+
+std::optional<std::locale> tr_locale_set_global(std::locale const& locale) noexcept
+{
+    try
+    {
+        auto old_locale = std::locale::global(locale);
 
         std::cout.imbue(std::locale{});
         std::cerr.imbue(std::locale{});
+
+        return old_locale;
     }
     catch (std::exception const&)
     {
-        // Ignore.
+        return {};
     }
 }
 

libtransmission/utils.h

@@ -10,6 +10,7 @@
 #include <cstdint> // uint8_t, uint32_t, uint64_t
 #include <cstddef> // size_t
 #include <ctime> // time_t
+#include <locale>
 #include <memory>
 #include <optional>
 #include <string>
@@ -54,7 +55,9 @@ struct tr_error;
 #define tr_ngettext(singular, plural, count) ((count) == 1 ? (singular) : (plural))
 #endif
 
-void tr_locale_set_global(char const* locale_name) noexcept;
+std::optional<std::locale> tr_locale_set_global(char const* locale_name) noexcept;
+
+std::optional<std::locale> tr_locale_set_global(std::locale const& locale) noexcept;
 
 // ---
 

libtransmission/variant-json.cc

@@ -569,41 +569,36 @@ void jsonRealFunc(tr_variant const& /*var*/, double const val, void* vdata)
     jsonChildFunc(data);
 }
 
-[[nodiscard]] char* write_escaped_char(char* buf, char const* const end, std::string_view& sv)
-{
-    auto u16buf = std::array<std::uint16_t, 2>{};
-
-    auto const* const begin8 = std::data(sv);
-    auto const* const end8 = begin8 + std::size(sv);
-    auto const* walk8 = begin8;
-    utf8::next(walk8, end8);
-    auto const end16 = utf8::utf8to16(begin8, walk8, std::begin(u16buf));
-
-    for (auto it = std::cbegin(u16buf); it != end16; ++it)
-    {
-        buf = fmt::format_to_n(buf, end - buf - 1, FMT_COMPILE("\\u{:04x}"), *it).out;
-    }
-
-    sv.remove_prefix(walk8 - begin8 - 1);
-    return buf;
-}
-
+// https://datatracker.ietf.org/doc/html/rfc8259#section-7
 void jsonStringFunc(tr_variant const& /*var*/, std::string_view sv, void* vdata)
 {
     auto* const data = static_cast<struct JsonWalk*>(vdata);
 
+    auto const utf8_str = tr_strv_convert_utf8(sv);
+    auto utf8_sv = std::string_view{ utf8_str };
+
     auto& out = data->out;
-    auto const [buf, buflen] = out.reserve_space(std::size(sv) * 6 + 2);
+    auto const [buf, buflen] = out.reserve_space(std::size(utf8_sv) * 6 + 2);
     auto* walk = reinterpret_cast<char*>(buf);
     auto const* const begin = walk;
     auto const* const end = begin + buflen;
 
     *walk++ = '"';
 
-    for (; !std::empty(sv); sv.remove_prefix(1))
+    for (; !std::empty(utf8_sv); utf8_sv.remove_prefix(1))
     {
-        switch (sv.front())
+        switch (utf8_sv.front())
         {
+        case '"':
+            *walk++ = '\\';
+            *walk++ = '"';
+            break;
+
+        case '\\':
+            *walk++ = '\\';
+            *walk++ = '\\';
+            break;
+
         case '\b':
             *walk++ = '\\';
             *walk++ = 'b';
@@ -629,31 +624,14 @@ void jsonStringFunc(tr_variant const& /*var*/, std::string_view sv, void* vdata)
             *walk++ = 't';
             break;
 
-        case '"':
-            *walk++ = '\\';
-            *walk++ = '"';
-            break;
-
-        case '\\':
-            *walk++ = '\\';
-            *walk++ = '\\';
-            break;
-
         default:
-            if (isprint((unsigned char)sv.front()) != 0)
+            if (utf8_sv.front() >= '\u0000' && utf8_sv.front() <= '\u001f')
             {
-                *walk++ = sv.front();
+                walk = fmt::format_to_n(walk, end - walk - 1, "\\u{:04x}", utf8_sv.front()).out;
             }
             else
             {
-                try
-                {
-                    walk = write_escaped_char(walk, end, sv);
-                }
-                catch (utf8::exception const&)
-                {
-                    *walk++ = '?';
-                }
+                *walk++ = utf8_sv.front();
             }
             break;
         }
@@ -733,7 +711,7 @@ std::string tr_variant_serde::to_json_string(tr_variant const& var) const
     walk(var, Funcs, &data, true);
 
     auto& buf = data.out;
-    if (!std::empty(buf))
+    if (!compact_ && !std::empty(buf))
     {
         buf.push_back('\n');
     }

tests/libtransmission/json-test.cc

@@ -13,6 +13,7 @@
 #include <string_view>
 
 #include <libtransmission/quark.h>
+#include <libtransmission/utils.h>
 #include <libtransmission/variant.h>
 
 #include "gtest/gtest.h"
@@ -24,12 +25,11 @@ class JSONTest : public ::testing::TestWithParam<char const*>
 protected:
     void SetUp() override
     {
+        ::testing::TestWithParam<char const*>::SetUp();
+
         auto const* locale_str = GetParam();
-        try
-        {
-            old_locale_ = std::locale::global(std::locale{ {}, new std::numpunct_byname<char>{ locale_str } });
-        }
-        catch (std::runtime_error const&)
+        old_locale_ = tr_locale_set_global(locale_str);
+        if (!old_locale_)
         {
             GTEST_SKIP();
         }
@@ -39,8 +39,10 @@ protected:
     {
         if (old_locale_)
         {
-            std::locale::global(*old_locale_);
+            tr_locale_set_global(*old_locale_);
         }
+
+        ::testing::TestWithParam<char const*>::TearDown();
     }
 
 private:
@@ -95,8 +97,7 @@ TEST_P(JSONTest, testUtf8)
     auto sv = std::string_view{};
     tr_quark const key = tr_quark_new("key"sv);
 
-    auto serde = tr_variant_serde::json();
-    serde.inplace();
+    auto serde = tr_variant_serde::json().inplace().compact();
     auto var = serde.parse(in).value_or(tr_variant{});
     EXPECT_TRUE(var.holds_alternative<tr_variant::Map>());
     EXPECT_TRUE(tr_variantDictFindStrView(&var, key, &sv));
@@ -114,7 +115,7 @@ TEST_P(JSONTest, testUtf8)
      * 1. Feed it JSON-escaped nonascii to the JSON decoder.
      * 2. Confirm that the result is UTF-8.
      * 3. Feed the same UTF-8 back into the JSON encoder.
-     * 4. Confirm that the result is JSON-escaped.
+     * 4. Confirm that the result is UTF-8.
      * 5. Dogfood that result back into the parser.
      * 6. Confirm that the result is UTF-8.
      */
@@ -127,32 +128,34 @@ TEST_P(JSONTest, testUtf8)
     var.clear();
 
     EXPECT_FALSE(std::empty(json));
-    EXPECT_NE(std::string::npos, json.find("\\u00f6"));
-    EXPECT_NE(std::string::npos, json.find("\\u00e9"));
+    EXPECT_EQ(R"({"key":"Letöltések"})"sv, json);
     var = serde.parse(json).value_or(tr_variant{});
     EXPECT_TRUE(var.holds_alternative<tr_variant::Map>());
     EXPECT_TRUE(tr_variantDictFindStrView(&var, key, &sv));
     EXPECT_EQ("Letöltések"sv, sv);
-}
 
-TEST_P(JSONTest, testUtf16Surrogates)
-{
-    static auto constexpr ThinkingFaceEmojiUtf8 = "\xf0\x9f\xa4\x94"sv;
-    auto var = tr_variant{};
-    tr_variantInitDict(&var, 1);
-    auto const key = tr_quark_new("key"sv);
-    tr_variantDictAddStr(&var, key, ThinkingFaceEmojiUtf8);
+    // Test string known to be prone to locale issues
+    // https://github.com/transmission/transmission/issues/5967
+    var.clear();
+    tr_variantInitDict(&var, 1U);
+    tr_variantDictAddStr(&var, key, "Дыскаграфія"sv);
+    json = serde.to_string(var);
+    EXPECT_EQ(R"({"key":"Дыскаграфія"})"sv, json);
+    var = serde.parse(json).value_or(tr_variant{});
+    EXPECT_TRUE(var.holds_alternative<tr_variant::Map>());
+    EXPECT_TRUE(tr_variantDictFindStrView(&var, key, &sv));
+    EXPECT_EQ("Дыскаграфія"sv, sv);
 
-    auto serde = tr_variant_serde::json();
-    auto const json = serde.compact().to_string(var);
-    EXPECT_NE(std::string::npos, json.find("ud83e"));
-    EXPECT_NE(std::string::npos, json.find("udd14"));
-
-    auto parsed = serde.parse(json).value_or(tr_variant{});
-    EXPECT_TRUE(parsed.holds_alternative<tr_variant::Map>());
-    auto value = std::string_view{};
-    EXPECT_TRUE(tr_variantDictFindStrView(&parsed, key, &value));
-    EXPECT_EQ(ThinkingFaceEmojiUtf8, value);
+    // Thinking emoji 🤔
+    var.clear();
+    tr_variantInitDict(&var, 1U);
+    tr_variantDictAddStr(&var, key, "\xf0\x9f\xa4\x94"sv);
+    json = serde.to_string(var);
+    EXPECT_EQ("{\"key\":\"\xf0\x9f\xa4\x94\"}"sv, json);
+    var = serde.parse(json).value_or(tr_variant{});
+    EXPECT_TRUE(var.holds_alternative<tr_variant::Map>());
+    EXPECT_TRUE(tr_variantDictFindStrView(&var, key, &sv));
+    EXPECT_EQ("\xf0\x9f\xa4\x94"sv, sv);
 }
 
 TEST_P(JSONTest, test1)