Copilot commented on code in PR #50479:
URL: https://github.com/apache/arrow/pull/50479#discussion_r3566571472
##########
cpp/src/arrow/compute/kernels/scalar_string_ascii.cc:
##########
@@ -1337,27 +1343,45 @@ struct RegexSubstringMatcher {
template <typename Type, typename Matcher>
struct MatchSubstringImpl {
- using offset_type = typename Type::offset_type;
-
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out,
const Matcher* matcher) {
- StringBoolTransform<Type>(
- ctx, batch,
- [&matcher](const void* raw_offsets, const uint8_t* data, int64_t
length,
- int64_t output_offset, uint8_t* output) {
- const offset_type* offsets = reinterpret_cast<const
offset_type*>(raw_offsets);
- FirstTimeBitmapWriter bitmap_writer(output, output_offset, length);
- for (int64_t i = 0; i < length; ++i) {
- const char* current_data = reinterpret_cast<const char*>(data +
offsets[i]);
- int64_t current_length = offsets[i + 1] - offsets[i];
- if (matcher->Match(std::string_view(current_data,
current_length))) {
- bitmap_writer.Set();
+ if constexpr (is_binary_view_like_type<Type>::value) {
+ // Views have no packed offset buffer, so iterate per element. Null slots
+ // are matched too (masked by output validity); reading their zero-filled
+ // view header is safe, as StringPredicateFunctor also assumes.
+ const ArraySpan& input = batch[0].array;
Review Comment:
The view-type fast path evaluates the matcher on null slots (relying on the
view header being zero-filled). For BinaryView/StringView, null slot bytes are
not validated and may contain arbitrary buffer_index/offset, so calling
util::FromBinaryView via ArrayIterator can dereference out-of-bounds buffers
and crash even though the output validity will mask the result. This path
should avoid reading view payloads for nulls.
##########
cpp/src/arrow/compute/kernels/scalar_string_internal.h:
##########
@@ -240,6 +240,8 @@ void AddUnaryStringPredicate(std::string name,
FunctionRegistry* registry,
auto exec = GenerateVarBinaryToVarBinary<StringPredicateFunctor,
Predicate>(ty);
ARROW_DCHECK_OK(func->AddKernel({ty}, boolean(), std::move(exec)));
}
+ ARROW_DCHECK_OK(func->AddKernel(
Review Comment:
Registering utf8_view with StringPredicateFunctor makes the predicate
kernels evaluate Predicate::Call on null view slots. ArrayIterator for view
types calls util::FromBinaryView unconditionally; null slot bytes are not
validated (ValidateBinaryView skips nulls), so a null slot can carry an
arbitrary buffer_index/offset and trigger out-of-bounds access/crash even
though output validity masks the value bit. The functor should skip null slots
for view inputs (or use VisitBitBlocks/VisitArrayValuesInline) before calling
FromBinaryView/Predicate::Call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
