Fix BLI_str_escape with control characters, add unit tests

3 files changed, 95 insertions, 38 deletions

diff --git a/source/blender/blenlib/BLI_string.h b/source/blender/blenlib/BLI_string.h
index 94d907b2765..d88a28a2fb8 100644
--- a/source/blender/blenlib/BLI_string.h
+++ b/source/blender/blenlib/BLI_string.h
@@ -85,7 +85,7 @@ size_t BLI_vsnprintf_rlen(char *__restrict buffer,
 char *BLI_sprintfN(const char *__restrict format, ...) ATTR_WARN_UNUSED_RESULT
     ATTR_NONNULL(1) ATTR_MALLOC ATTR_PRINTF_FORMAT(1, 2);
 
-size_t BLI_str_escape(char *__restrict dst, const char *__restrict src, const size_t maxncpy)
+size_t BLI_str_escape(char *__restrict dst, const char *__restrict src, const size_t dst_maxncpy)
     ATTR_NONNULL();
 
 size_t BLI_str_format_int_grouped(char dst[16], int num) ATTR_NONNULL();
diff --git a/source/blender/blenlib/intern/string.c b/source/blender/blenlib/intern/string.c
index dd120a531fa..4734753d304 100644
--- a/source/blender/blenlib/intern/string.c
+++ b/source/blender/blenlib/intern/string.c
@@ -317,49 +317,40 @@ char *BLI_sprintfN(const char *__restrict format, ...)
   return n;
 }
 
-/* match pythons string escaping, assume double quotes - (")
- * TODO: should be used to create RNA animation paths.
- * TODO: support more fancy string escaping. current code is primitive
- *    this basically is an ascii version of PyUnicode_EncodeUnicodeEscape()
- *    which is a useful reference. */
-size_t BLI_str_escape(char *__restrict dst, const char *__restrict src, const size_t maxncpy)
+/**
+ * This roughly matches C and Python's string escaping with double quotes - `"`.
+ *
+ * Since every character may need escaping,
+ * it's common to create a buffer twice as large as the input.
+ *
+ * \param dst: The destination string, at least \a dst_maxncpy, typically `(strlen(src) * 2) + 1`.
+ * \param src: The un-escaped source string.
+ * \param dst_maxncpy: The maximum number of bytes allowable to copy.
+ *
+ * \note This is used for creating animation paths in blend files.
+ */
+size_t BLI_str_escape(char *__restrict dst, const char *__restrict src, const size_t dst_maxncpy)
 {
-  size_t len = 0;
 
-  BLI_assert(maxncpy != 0);
+  BLI_assert(dst_maxncpy != 0);
 
-  while (len < maxncpy) {
-    switch (*src) {
-      case '\0':
-        goto escape_finish;
-      case '\\':
-      case '"':
-        ATTR_FALLTHROUGH;
-
-      /* less common but should also be support */
-      case '\t':
-      case '\n':
-      case '\r':
-        if (len + 1 < maxncpy) {
-          *dst++ = '\\';
-          len++;
-        }
-        else {
-          /* not enough space to escape */
-          break;
-        }
-        ATTR_FALLTHROUGH;
-      default:
-        *dst = *src;
+  size_t len = 0;
+  for (; (len < dst_maxncpy) && (*src != '\0'); dst++, src++, len++) {
+    char c = *src;
+    if (ELEM(c, '\\', '"') ||                       /* Use as-is. */
+        ((c == '\t') && ((void)(c = 't'), true)) || /* Tab. */
+        ((c == '\n') && ((void)(c = 'n'), true)) || /* Newline. */
+        ((c == '\r') && ((void)(c = 'r'), true)))   /* Carriage return. */
+    {
+      if (UNLIKELY(len + 1 >= dst_maxncpy)) {
+        /* Not enough space to escape. */
         break;
+      }
+      *dst++ = '\\';
+      len++;
     }
-    dst++;
-    src++;
-    len++;
+    *dst = c;
   }
-
-escape_finish:
-
   *dst = '\0';
 
   return len;
diff --git a/source/blender/blenlib/tests/BLI_string_test.cc b/source/blender/blenlib/tests/BLI_string_test.cc
index 58135adbcca..7ef2f5a888e 100644
--- a/source/blender/blenlib/tests/BLI_string_test.cc
+++ b/source/blender/blenlib/tests/BLI_string_test.cc
@@ -802,3 +802,69 @@ TEST_F(StringCasecmpNatural, TextAndNumbers)
   testReturnsLessThanZeroForAll(negative);
   testReturnsMoreThanZeroForAll(positive);
 }
+
+/* BLI_str_escape */
+
+class StringEscape : public testing::Test {
+ protected:
+  StringEscape()
+  {
+  }
+
+  using CompareWordsArray = vector<std::array<const char *, 2>>;
+
+  void testEscapeWords(const CompareWordsArray &items)
+  {
+    size_t dst_test_len;
+    char dst_test[64];
+    for (const auto &item : items) {
+      /* Escape the string. */
+      dst_test_len = BLI_str_escape(dst_test, item[0], SIZE_MAX);
+      EXPECT_STREQ(dst_test, item[1]);
+      EXPECT_EQ(dst_test_len, strlen(dst_test));
+    }
+  }
+};
+
+TEST_F(StringEscape, Simple)
+{
+  const CompareWordsArray equal{
+      {"", ""},
+      {"/", "/"},
+      {"'", "'"},
+      {"?", "?"},
+  };
+
+  const CompareWordsArray escaped{
+      {"\\", "\\\\"},
+      {"A\\", "A\\\\"},
+      {"\\A", "\\\\A"},
+      {"A\\B", "A\\\\B"},
+      {"?", "?"},
+      {"\"\\", "\\\"\\\\"},
+      {"\\\"", "\\\\\\\""},
+      {"\"\\\"", "\\\"\\\\\\\""},
+
+      {"\"\"\"", "\\\"\\\"\\\""},
+      {"\\\\\\", "\\\\\\\\\\\\"},
+  };
+
+  testEscapeWords(equal);
+  testEscapeWords(escaped);
+}
+
+TEST_F(StringEscape, Control)
+{
+  const CompareWordsArray escaped{
+      {"\n", "\\n"},
+      {"\r", "\\r"},
+      {"\t", "\\t"},
+      {"A\n", "A\\n"},
+      {"\nA", "\\nA"},
+      {"\n\r\t", "\\n\\r\\t"},
+      {"\n_\r_\t", "\\n_\\r_\\t"},
+      {"\n\\\r\\\t", "\\n\\\\\\r\\\\\\t"},
+  };
+
+  testEscapeWords(escaped);
+}