[C++] Builder would ub when AppendValues called with empty vector

[mapleFU]

Describe the enhancement requested

When AppendValues called with empty vector, this would cause memcpy

  /// \brief Append a sequence of elements in one shot
  /// \param[in] values a contiguous C array of values
  /// \param[in] length the number of values to append
  /// \param[in] is_valid an std::vector<bool> indicating valid (1) or null
  /// (0). Equal in length to values
  /// \return Status
  Status AppendValues(const value_type* values, int64_t length,
                      const std::vector<bool>& is_valid) {
    ARROW_RETURN_NOT_OK(Reserve(length));
    data_builder_.UnsafeAppend(values, length);
    // length_ is update by these
    ArrayBuilder::UnsafeAppendToBitmap(is_valid);
    return Status::OK();
  }

  /// \brief Append a sequence of elements in one shot
  /// \param[in] values a std::vector of values
  /// \param[in] is_valid an std::vector<bool> indicating valid (1) or null
  /// (0). Equal in length to values
  /// \return Status
  Status AppendValues(const std::vector<value_type>& values,
                      const std::vector<bool>& is_valid) {
    return AppendValues(values.data(), static_cast<int64_t>(values.size()), is_valid);
  }

In this case, unsafe append directly memcpy, and when size == 0, it would be a undefined behavior

Component(s)

C++

[raulcd]

I have zero knowledge here so bear with me if there is something obvious I am missing. I was trying to understand what the issue is and after some investigation it seems that memcpy should just handle the size 0 without problem, could you expand on what would be the undefined behavior?
Was your expectation to have some kind of check for length or am I misinterpreting the issue?

  Status AppendValues(const value_type* values, int64_t length,
                      const std::vector<bool>& is_valid) {
    if (length > 0) {
        ARROW_RETURN_NOT_OK(Reserve(length));
        data_builder_.UnsafeAppend(values, length);
        // length_ is update by these
        ArrayBuilder::UnsafeAppendToBitmap(is_valid);
    }
    return Status::OK();
  }

[mapleFU]

@raulcd There is few steps for this problem

1. AppendValues might has std::vector as arguments, std::vector::data might be used
2. std::vector::data might returns nullptr if size == 0: https://en.cppreference.com/w/cpp/container/vector/data
3. https://en.cppreference.com/w/cpp/string/byte/memcpy memcpy says, "If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero."

[mapleFU]

diff --git a/cpp/src/arrow/array/builder_primitive.h b/cpp/src/arrow/array/builder_primitive.h
index de7af1b46b..be9761fb46 100644
--- a/cpp/src/arrow/array/builder_primitive.h
+++ b/cpp/src/arrow/array/builder_primitive.h
@@ -211,6 +211,9 @@ class NumericBuilder
   /// \return Status
   Status AppendValues(const std::vector<value_type>& values,
                       const std::vector<bool>& is_valid) {
+    if (values.empty()) {
+      return Status::OK();
+    }
     return AppendValues(values.data(), static_cast<int64_t>(values.size()), is_valid);
   }
 
@@ -218,6 +221,9 @@ class NumericBuilder
   /// \param[in] values a std::vector of values
   /// \return Status
   Status AppendValues(const std::vector<value_type>& values) {
+    if (values.empty()) {
+      return Status::OK();
+    }
     return AppendValues(values.data(), static_cast<int64_t>(values.size()));
     return AppendValues(values.data(), static_cast<int64_t>(values.size()));
   }

@pitrou @bkietz @felipecrv Should I just guard the vector interface? Or should prevent from all AppendValues?

[pitrou]

@pitrou @bkietz @felipecrv Should I just guard the vector interface? Or should prevent from all AppendValues?

Can you submit a PR and we can review that?

[mapleFU]

done #44794

[mapleFU]

Issue resolved by pull request 44794
#44794