Bug 1388789 - clean up \0 emission in nsTextFormatter; r?froydnj draft
authorTom Tromey <tom@tromey.com>
Wed, 06 Sep 2017 09:38:58 -0600
changeset 667051 51b1dc2f4f868b9efa5c9f18861bb64e89ed8238
parent 667050 86b722d69c8732b69ef158fc3c795d6c7f4cecdd
child 732284 ea3597d9f3ce462199abf0dfc615cee42a0625e2
push id80599
push userbmo:ttromey@mozilla.com
push dateTue, 19 Sep 2017 16:49:35 +0000
reviewersfroydnj
bugs1388789
milestone57.0a1
Bug 1388789 - clean up \0 emission in nsTextFormatter; r?froydnj nsTextFormatter unconditionally emitted a trailing \0, leading some code elsewhere to have to work around this. This changes the code to only emit it in snprintf. MozReview-Commit-ID: G3CBpAPp9Tn
dom/svg/SVGPathSegUtils.cpp
xpcom/string/nsTextFormatter.cpp
xpcom/tests/gtest/TestTextFormatter.cpp
--- a/dom/svg/SVGPathSegUtils.cpp
+++ b/dom/svg/SVGPathSegUtils.cpp
@@ -70,25 +70,16 @@ SVGPathSegUtils::GetValueAsString(const 
       break;
 
     default:
       MOZ_ASSERT(false, "Unknown segment type");
       aValue = u"<unknown-segment-type>";
       return;
     }
   }
-
-  // nsTextFormatter::ssprintf is one of the nsTextFormatter methods that
-  // randomly appends '\0' to its output string, which means that the length
-  // of the output string is one too long. We need to manually remove that '\0'
-  // until nsTextFormatter is fixed.
-  //
-  if (aValue[aValue.Length() - 1] == char16_t('\0')) {
-    aValue.SetLength(aValue.Length() - 1);
-  }
 }
 
 
 static float
 CalcDistanceBetweenPoints(const Point& aP1, const Point& aP2)
 {
   return NS_hypot(aP2.x - aP1.x, aP2.y - aP1.y);
 }
--- a/xpcom/string/nsTextFormatter.cpp
+++ b/xpcom/string/nsTextFormatter.cpp
@@ -473,24 +473,25 @@ nsTextFormatter::dosprintf(SprintfStateS
   static const char16_t hex[] = u"0123456789abcdef";
   static char16_t HEX[] = u"0123456789ABCDEF";
   static const BoxedValue emptyString(u"");
 
   char16_t c;
   int flags, width, prec, radix;
 
   const char16_t* hexp;
-  int rv;
 
   // Next argument for non-numbered arguments.
   size_t nextNaturalArg = 0;
   // True if we ever saw a numbered argument.
   bool sawNumberedArg = false;
 
   while ((c = *aFmt++) != 0) {
+    int rv;
+
     if (c != '%') {
       rv = (*aState->stuff)(aState, aFmt - 1, 1);
       if (rv < 0) {
         return rv;
       }
       continue;
     }
 
@@ -823,34 +824,25 @@ nsTextFormatter::dosprintf(SprintfStateS
         MOZ_ASSERT(0);
     }
 
     if (rv < 0) {
       return rv;
     }
   }
 
-  /* Stuff trailing NUL */
-  char16_t null = '\0';
-
-  rv = (*aState->stuff)(aState, &null, 1);
-
-  return rv;
+  return 0;
 }
 
 /************************************************************************/
 
 int
 nsTextFormatter::StringStuff(nsTextFormatter::SprintfStateStr* aState, const char16_t* aStr,
                              uint32_t aLen)
 {
-  if (*aStr == '\0') {
-    return 0;
-  }
-
   ptrdiff_t off = aState->cur - aState->base;
 
   nsAString* str = static_cast<nsAString*>(aState->stuffclosure);
   str->Append(aStr, aLen);
 
   aState->base = str->BeginWriting();
   aState->cur = aState->base + off;
 
@@ -890,33 +882,39 @@ nsTextFormatter::LimitStuff(SprintfState
   return 0;
 }
 
 uint32_t
 nsTextFormatter::vsnprintf(char16_t* aOut, uint32_t aOutLen,
                            const char16_t* aFmt, mozilla::Span<BoxedValue> aValues)
 {
   SprintfStateStr ss;
-  uint32_t n;
 
   MOZ_ASSERT((int32_t)aOutLen > 0);
   if ((int32_t)aOutLen <= 0) {
     return 0;
   }
 
   ss.stuff = LimitStuff;
   ss.base = aOut;
   ss.cur = aOut;
   ss.maxlen = aOutLen;
   int result = dosprintf(&ss, aFmt, aValues);
 
-  /* If we added chars, and we didn't append a null, do it now. */
-  if ((ss.cur != ss.base) && (*(ss.cur - 1) != '\0')) {
-    *(--ss.cur) = '\0';
+  if (ss.cur == ss.base) {
+    return 0;
   }
 
+  // Append a NUL.  However, be sure not to count it in the returned
+  // length.
+  if (ss.cur - ss.base >= ptrdiff_t(ss.maxlen)) {
+    --ss.cur;
+  }
+  *ss.cur = '\0';
+
+  // Check the result now, so that an unterminated string can't
+  // possibly escape.
   if (result < 0) {
     return -1;
   }
 
-  n = ss.cur - ss.base;
-  return n ? n - 1 : n;
+  return ss.cur - ss.base;
 }
--- a/xpcom/tests/gtest/TestTextFormatter.cpp
+++ b/xpcom/tests/gtest/TestTextFormatter.cpp
@@ -211,8 +211,22 @@ TEST(TextFormatterTestMismatch, format_c
   // just for completeness, this is our format, and works
   nsTextFormatter::ssprintf(out, u"%c", 'c');
   EXPECT_EQ(1u, out.Length());
   EXPECT_EQ(u'c', out.CharAt(0));
   nsTextFormatter::ssprintf(out, u"%c", u'c');
   EXPECT_EQ(1u, out.Length());
   EXPECT_EQ(u'c', out.CharAt(0));
 }
+
+TEST(TextFormatterTestResults, Tests)
+{
+  char16_t buf[10];
+
+  EXPECT_EQ(nsTextFormatter::snprintf(buf, 10, u"%s", "more than 10 characters"), 9u);
+  EXPECT_EQ(buf[9], '\0');
+  EXPECT_STREQ("more than", NS_ConvertUTF16toUTF8(buf).get());
+
+  nsString out;
+  nsTextFormatter::ssprintf(out, u"%s", "more than 10 characters");
+  // The \0 isn't written here.
+  EXPECT_EQ(out.Length(), 23u);
+}