Merge remote-tracking branch 'upstream/main' into limited-api-direct-replacements

Author: mroeschke

.github/CODEOWNERS

@@ -55,9 +55,7 @@ compose.yaml @assignUser @jonkeane @kou @raulcd
 
 ## C++
 /cpp/src/arrow/ @pitrou
-/cpp/src/arrow/acero @westonpace
 /cpp/src/arrow/adapters/orc @wgtmac
-/cpp/src/arrow/engine @westonpace
 /cpp/src/arrow/flight/ @lidavidm
 /cpp/src/gandiva @dmitry-chirkov-dremio @lriggs @akravchukdremio @xxlaykxx
 /cpp/src/parquet @wgtmac @pitrou

.pre-commit-config.yaml

@@ -298,6 +298,7 @@ repos:
           ?^ci/scripts/cpp_test\.sh$|
           ?^ci/scripts/download_tz_database\.sh$|
           ?^ci/scripts/install_azurite\.sh$|
+          ?^ci/scripts/install_bison\.sh$|
           ?^ci/scripts/install_ccache\.sh$|
           ?^ci/scripts/install_chromedriver\.sh$|
           ?^ci/scripts/install_cmake\.sh$|

ci/scripts/install_bison.sh

@@ -34,7 +34,7 @@ wget -q "${url}" -O - | tar -xzf - --directory /tmp/bison --strip-components=1
 
 pushd /tmp/bison
 ./configure --prefix="${prefix}"
-make -j$(nproc)
+make -j"$(nproc)"
 make install
 popd
 

cpp/src/gandiva/precompiled/string_ops.cc

@@ -208,7 +208,12 @@ gdv_int32 utf8_length_ignore_invalid(const char* data, gdv_int32 data_len) {
     }
     for (int j = 1; j < char_len; ++j) {
       if ((data[i + j] & 0xC0) != 0x80) {  // bytes following head-byte of glyph
-        char_len += 1;
+        // Only the bytes up to the mismatch belong to this (invalid) glyph, so
+        // advance past them and let the outer loop re-parse the rest. Keeping
+        // char_len at its declared width would swallow valid characters that
+        // fall inside the truncated sequence's window.
+        char_len = j;
+        break;
       }
     }
     ++count;
@@ -2472,6 +2477,21 @@ const char* byte_substr_binary_int32_int32(gdv_int64 context, const char* text,
     return "";
   }
 
+  int32_t startPos = 0;
+  if (offset >= 0) {
+    startPos = offset - 1;
+  } else if (text_len + offset >= 0) {
+    startPos = text_len + offset;
+  }
+
+  // an offset past the end of the text leaves nothing to copy; without this the
+  // truncation below yields a negative *out_len that memcpy reads as a huge size.
+  // check before allocating so a past-end offset needs no output buffer at all
+  if (startPos >= text_len) {
+    *out_len = 0;
+    return "";
+  }
+
   char* ret =
       reinterpret_cast<gdv_binary>(gdv_fn_context_arena_malloc(context, text_len));
 
@@ -2481,15 +2501,11 @@ const char* byte_substr_binary_int32_int32(gdv_int64 context, const char* text,
     return "";
   }
 
-  int32_t startPos = 0;
-  if (offset >= 0) {
-    startPos = offset - 1;
-  } else if (text_len + offset >= 0) {
-    startPos = text_len + offset;
-  }
-
-  // calculate end position from length and truncate to upper value bounds
-  if (startPos + length > text_len) {
+  // calculate end position from length and truncate to upper value bounds.
+  // startPos < text_len is guaranteed above, so text_len - startPos is positive;
+  // comparing against it avoids the startPos + length overflow when length is
+  // near INT32_MAX, which would otherwise leave *out_len huge for the memcpy.
+  if (length > text_len - startPos) {
     *out_len = text_len - startPos;
   } else {
     *out_len = length;

cpp/src/gandiva/precompiled/string_ops_test.cc

@@ -18,7 +18,9 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include <cstring>
 #include <limits>
+#include <memory>
 
 #include "gandiva/execution_context.h"
 #include "gandiva/precompiled/types.h"
@@ -1608,6 +1610,60 @@ TEST(TestStringOps, TestRpadString) {
   EXPECT_EQ(std::string(out_str + 5000, 2), "α");
 }
 
+TEST(TestStringOps, TestPadMalformedUtf8NoOverread) {
+  gandiva::ExecutionContext ctx;
+  uint64_t ctx_ptr = reinterpret_cast<gdv_int64>(&ctx);
+  gdv_int32 out_len = 0;
+
+  // A 4-byte utf8 lead byte followed by non-continuation bytes and no trailing
+  // space. utf8_length_ignore_invalid() used to extend the glyph length past
+  // the end of the buffer while scanning the continuation bytes. The input is
+  // held in an exactly-sized heap buffer so any over-read trips AddressSanitizer.
+  const char bytes[] = {'\xF0', 'a', 'a', 'a'};
+  const auto text_len = static_cast<gdv_int32>(sizeof(bytes));
+  std::unique_ptr<char[]> text(new char[text_len]);
+  std::memcpy(text.get(), bytes, text_len);
+  const std::string text_str(text.get(), text_len);
+
+  // The lone lead byte counts as one invalid glyph and the three 'a's as one
+  // each, so the length is 4 and padding to width 6 adds two fill characters.
+  const char* out_str =
+      lpad_utf8_int32_utf8(ctx_ptr, text.get(), text_len, 6, " ", 1, &out_len);
+  EXPECT_EQ(out_len, 6);
+  EXPECT_EQ(std::string(out_str, out_len), "  " + text_str);
+
+  out_str = rpad_utf8_int32_utf8(ctx_ptr, text.get(), text_len, 6, " ", 1, &out_len);
+  EXPECT_EQ(out_len, 6);
+  EXPECT_EQ(std::string(out_str, out_len), text_str + "  ");
+}
+
+TEST(TestStringOps, TestPadMalformedUtf8KeepsValidGlyph) {
+  gandiva::ExecutionContext ctx;
+  uint64_t ctx_ptr = reinterpret_cast<gdv_int64>(&ctx);
+  gdv_int32 out_len = 0;
+
+  // {0xF0, 'a', 0xE2, 0x82, 0xAC}: malformed 4-byte lead + ASCII 'a' + U+20AC €.
+  // 0xF0 alone counts as one invalid glyph, then 'a' and € follow on their own,
+  // so the count is 3. If the inner scan kept char_len at 4 it would advance the
+  // outer loop past 'a', 0xE2, 0x82 and only see the orphaned 0xAC, giving 2.
+  // The input sits in an exactly-sized heap buffer so any over-read trips ASAN.
+  const char bytes[] = {'\xF0', 'a', '\xE2', '\x82', '\xAC'};
+  const auto text_len = static_cast<gdv_int32>(sizeof(bytes));
+  std::unique_ptr<char[]> text(new char[text_len]);
+  std::memcpy(text.get(), bytes, text_len);
+  const std::string text_str(text.get(), text_len);
+
+  // 3 glyphs padded to width 5 adds two fill characters, out_len = 2 + 5 = 7.
+  const char* out_str =
+      lpad_utf8_int32_utf8(ctx_ptr, text.get(), text_len, 5, " ", 1, &out_len);
+  EXPECT_EQ(out_len, 7);
+  EXPECT_EQ(std::string(out_str, out_len), "  " + text_str);
+
+  out_str = rpad_utf8_int32_utf8(ctx_ptr, text.get(), text_len, 5, " ", 1, &out_len);
+  EXPECT_EQ(out_len, 7);
+  EXPECT_EQ(std::string(out_str, out_len), text_str + "  ");
+}
+
 TEST(TestStringOps, TestRtrim) {
   gandiva::ExecutionContext ctx;
   uint64_t ctx_ptr = reinterpret_cast<gdv_int64>(&ctx);
@@ -1903,6 +1959,20 @@ TEST(TestStringOps, TestByteSubstr) {
   out_str = byte_substr_binary_int32_int32(ctx_ptr, "TestString", 10, -100, 10, &out_len);
   EXPECT_EQ(std::string(out_str, out_len), "TestString");
   EXPECT_FALSE(ctx.has_error());
+
+  // offset past the end of the text must yield an empty result, not a negative
+  // length that memcpy reads as an out-of-bounds copy
+  out_str = byte_substr_binary_int32_int32(ctx_ptr, "TestString", 10, 15, 10, &out_len);
+  EXPECT_EQ(out_len, 0);
+  EXPECT_EQ(std::string(out_str, out_len), "");
+  EXPECT_FALSE(ctx.has_error());
+
+  // a huge length must be truncated to the remaining bytes, not overflow
+  // startPos + length and copy far past the end of the text
+  out_str =
+      byte_substr_binary_int32_int32(ctx_ptr, "TestString", 10, 2, 2147483647, &out_len);
+  EXPECT_EQ(std::string(out_str, out_len), "estString");
+  EXPECT_FALSE(ctx.has_error());
 }
 
 TEST(TestStringOps, TestStrPos) {

cpp/src/gandiva/tests/date_time_test.cc

@@ -105,9 +105,9 @@ int32_t DaysSince(time_t base_line, int32_t yy, int32_t mm, int32_t dd, int32_t
   given_ts.tm_min = min;
   given_ts.tm_sec = sec;
 
-  time_t ts = mktime(&given_ts);
+  time_t ts = timegm(&given_ts);
   if (ts == static_cast<time_t>(-1)) {
-    ARROW_LOG(FATAL) << "mktime() failed";
+    ARROW_LOG(FATAL) << "timegm() failed";
   }
   // time_t is an arithmetic type on both POSIX and Windows, we can simply
   // subtract to get a duration in seconds.

docs/source/cpp/security.rst

@@ -120,6 +120,22 @@ variants, but these typically fall into two categories:
   arguments (this is typical of the :ref:`array builders <cpp-api-array-builders>`
   and :ref:`buffer builders <cpp-api-buffer-builders>`).
 
+Controlling and restricting memory allocation
+---------------------------------------------
+
+By construction, many Arrow C++ APIs can allocate large amounts of memory, depending
+on their input parameters. Arrow C++ allows customizing the memory allocator for
+such large data requests through the :ref:`MemoryPool <cpp_memory_pool>` interface.
+
+You can therefore implement a MemoryPool class enforcing the restrictions
+of your choice (for example to limit the total number of allocated bytes), and pass
+it to any Arrow C++ APIs you use.
+
+.. note::
+   Unlike memory used for Arrow data, smaller metadata structures (such as field
+   names, etc.) instead rely on the C++ standard library allocators for convenience.
+   They will therefore be invisible to the MemoryPool memory accounting.
+
 Ingesting untrusted data
 ========================
 
@@ -144,6 +160,13 @@ from an untrusted source), you **must** follow these steps:
 2. If the API returned successfully, validate the returned Arrow data in full
    (see "Full validity" above)
 
+Furthermore, both the IPC and the Parquet format allow for powerful forms of
+compression, and can therefore exhibit large expansion factors when reading.
+If you need to guard against potential denial-of-service attacks that would
+exhaust available memory, we recommend you enforce memory allocation limits
+using a dedicated MemoryPool implementation (see "Controlling and restricting
+memory allocation" above).
+
 CSV reader
 ----------
 

docs/source/format/Columnar.rst

@@ -1391,6 +1391,8 @@ have two entries in each RecordBatch. For a RecordBatch of this schema with
     buffer 12: col2    data
     buffer 13: col2    data
 
+.. _buffer-compression:
+
 Compression
 -----------
 

docs/source/format/Security.rst

@@ -180,6 +180,11 @@ their own risks. For example, buffer offsets and sizes encoded in IPC messages
 may be out of bounds for the IPC stream; Flatbuffers-encoded metadata payloads
 may carry incorrect offsets pointing outside of the designated metadata area.
 
+In addition, the IPC format provides optional :ref:`buffer compression <buffer-compression>`
+using general-purpose compression algorithms. It is therefore possible to craft an IPC
+stream or file that acts as a decompression bomb by consuming all available memory,
+opening a potential channel for denial-of-service attacks.
+
 Advice for users
 ----------------
 
@@ -194,6 +199,9 @@ It is **extremely recommended** to run dedicated validation checks when decoding
 the IPC format, to make sure that the decoding can not induce unwanted behavior.
 Failing those checks should return a well-known error to the caller, not crash.
 
+It is **recommended** to provide facilities for users to control the memory
+allocation behavior when reading an IPC file or stream (for example by making
+the allocator customizable).
 
 Extension Types
 ===============