apacheGH-41044: [C++] formatting.h: Make sure space is allocated for …

…the 'Z' when formatting timestamps (apache#41045) ### What changes are included in this PR? A test that reproduces an issue found by the fuzzer and a fix for it. ### Are these changes tested? - A test - Comments clarifying somethings around `formatting.h` - Increasing the size of the local buffer used to format timestamps The issue was introduced only recently (unreleased): apache#39272 * GitHub Issue: apache#41044 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
Bit-Quill · Apr 6, 2024 · 6aa3321 · 6aa3321
1 parent 6f6b3b4
commit 6aa3321
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 3 deletions.
diff --git a/cpp/src/arrow/util/formatting.h b/cpp/src/arrow/util/formatting.h
@@ -126,8 +126,10 @@ namespace detail {
 ARROW_EXPORT extern const char digit_pairs[];
 
 // Based on fmtlib's format_int class:
-// Write digits from right to left into a stack allocated buffer
-inline void FormatOneChar(char c, char** cursor) { *--*cursor = c; }
+// Write digits from right to left into a stack allocated buffer.
+// \pre *cursor points to the byte after the one that will be written.
+// \post *cursor points to the byte that was written.
+inline void FormatOneChar(char c, char** cursor) { *(--(*cursor)) = c; }
 
 template <typename Int>
 void FormatOneDigit(Int value, char** cursor) {
@@ -326,6 +328,7 @@ class StringFormatter<DoubleType> : public FloatToStringFormatterMixin<DoubleTyp
 namespace detail {
 
 constexpr size_t BufferSizeYYYY_MM_DD() {
+  // "-"? "99999-12-31"
   return 1 + detail::Digits10(99999) + 1 + detail::Digits10(12) + 1 +
          detail::Digits10(31);
 }
@@ -352,6 +355,7 @@ inline void FormatYYYY_MM_DD(arrow_vendored::date::year_month_day ymd, char** cu
 
 template <typename Duration>
 constexpr size_t BufferSizeHH_MM_SS() {
+  // "23:59:59" ("." "9"+)?
   return detail::Digits10(23) + 1 + detail::Digits10(59) + 1 + detail::Digits10(59) + 1 +
          detail::Digits10(Duration::period::den) - 1;
 }
@@ -505,8 +509,9 @@ class StringFormatter<TimestampType> {
       timepoint_days -= days(1);
     }
 
+    // YYYY_MM_DD " " HH_MM_SS "Z"?
     constexpr size_t buffer_size =
-        detail::BufferSizeYYYY_MM_DD() + 1 + detail::BufferSizeHH_MM_SS<Duration>();
+        detail::BufferSizeYYYY_MM_DD() + 1 + detail::BufferSizeHH_MM_SS<Duration>() + 1;
 
     std::array<char, buffer_size> buffer;
     char* cursor = buffer.data() + buffer_size;

diff --git a/cpp/src/arrow/util/formatting_util_test.cc b/cpp/src/arrow/util/formatting_util_test.cc
@@ -533,6 +533,14 @@ TEST(Formatting, Timestamp) {
     }
   }
 
+  {
+    constexpr int64_t kMillisInDay = 24 * 60 * 60 * 1000;
+    auto ty = timestamp(TimeUnit::MILLI, "+01:00");
+    StringFormatter<TimestampType> formatter(ty.get());
+    AssertFormatting(formatter, -15000 * 365 * kMillisInDay + 1,
+                     "-13021-12-17 00:00:00.001Z");
+  }
+
   {
     auto ty = timestamp(TimeUnit::MILLI, "Pacific/Maruesas");
     StringFormatter<TimestampType> formatter(ty.get());