Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-36311: [C++] Fix integer overflows in utf8_slice_codeunits #36575

Merged
merged 5 commits into from
Jul 11, 2023
Merged

GH-36311: [C++] Fix integer overflows in utf8_slice_codeunits #36575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a5f286
Avoid overflows in `SliceCodunitsTransform`
benibus Jul 9, 2023
40a7afc
Handle `INT64_MIN` in `SliceCodeunitsTransform`
benibus Jul 10, 2023
f6e013e
Update Python bindings for `SliceOptions`
benibus Jul 10, 2023
03fa729
Test `stop=None` for Python
benibus Jul 10, 2023
58eb873
Fix negation UB
benibus Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_string_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2091,6 +2091,14 @@ TYPED_TEST(TestStringKernels, SliceCodeunitsPosPos) {
options_step_neg.stop = 0;
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ","𝑓öõḍš"])",
this->type(), R"(["", "", "ö", "õ", "ḍö", "šõ"])", &options_step_neg);

constexpr auto max = std::numeric_limits<int64_t>::max();
SliceOptions options_max_step{1, max, 2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "", "ö", "ö", "öḍ", "öḍ"])", &options_max_step);
SliceOptions options_max_step_neg{1, max, -2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "", "", "", "", ""])", &options_max_step_neg);
}

TYPED_TEST(TestStringKernels, SliceCodeunitsPosNeg) {
Expand All @@ -2107,6 +2115,15 @@ TYPED_TEST(TestStringKernels, SliceCodeunitsPosNeg) {
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ","𝑓öõḍš"])",
this->type(), R"(["", "𝑓", "ö", "õ𝑓", "ḍö", "ḍö"])",
&options_step_neg);

constexpr auto min = std::numeric_limits<int64_t>::min();
SliceOptions options_min_step{2, min, 2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "", "", "", "", ""])", &options_min_step);
SliceOptions options_min_step_neg{2, min, -2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "𝑓", "ö", "õ𝑓", "õ𝑓", "õ𝑓"])",
&options_min_step_neg);
}

TYPED_TEST(TestStringKernels, SliceCodeunitsNegNeg) {
Expand All @@ -2123,6 +2140,15 @@ TYPED_TEST(TestStringKernels, SliceCodeunitsNegNeg) {
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "𝑓", "ö", "õ𝑓", "ḍö", "šõ"])",
&options_step_neg);

constexpr auto min = std::numeric_limits<int64_t>::min();
SliceOptions options_min_step{-2, min, 2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "", "", "", "", ""])", &options_min_step);
SliceOptions options_min_step_neg{-2, min, -2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "", "𝑓", "ö", "õ𝑓", "ḍö"])",
&options_min_step_neg);
}

TYPED_TEST(TestStringKernels, SliceCodeunitsNegPos) {
Expand All @@ -2138,6 +2164,15 @@ TYPED_TEST(TestStringKernels, SliceCodeunitsNegPos) {
options_step_neg.stop = 0;
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "", "ö", "õ", "ḍö", "šõ"])", &options_step_neg);

constexpr auto max = std::numeric_limits<int64_t>::max();
SliceOptions options_max_step{-3, max, 2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "𝑓", "𝑓", "𝑓õ", "öḍ", "õš"])",
&options_max_step);
SliceOptions options_max_step_neg{-3, max, -2};
this->CheckUnary("utf8_slice_codeunits", R"(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])",
this->type(), R"(["", "", "", "", "", ""])", &options_max_step_neg);
}

#endif // ARROW_WITH_UTF8PROC
Expand Down
18 changes: 13 additions & 5 deletions cpp/src/arrow/compute/kernels/scalar_string_utf8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,7 +1090,8 @@ struct SliceCodeunitsTransform : StringSliceTransformBase {
// on the resulting slice lengths, so return a worst case estimate.
return input_ncodeunits;
}
int64_t max_slice_codepoints = (opt.stop - opt.start + opt.step - 1) / opt.step;
int64_t stop = std::clamp(opt.stop, -input_ncodeunits, input_ncodeunits);
int64_t max_slice_codepoints = (stop - opt.start + opt.step - 1) / opt.step;
// The maximum UTF8 byte size of a codepoint is 4
return std::min(input_ncodeunits,
4 * ninputs * std::max<int64_t>(0, max_slice_codepoints));
Expand Down Expand Up @@ -1133,7 +1134,7 @@ struct SliceCodeunitsTransform : StringSliceTransformBase {
} else if (opt.stop < 0) {
// or from the end (but we will never need to < begin_sliced)
RETURN_IF_UTF8_ERROR(arrow::util::UTF8AdvanceCodepointsReverse(
begin_sliced, end, &end_sliced, -opt.stop));
begin_sliced, end, &end_sliced, Negate(opt.stop)));
} else {
// zero length slice
return 0;
Expand All @@ -1158,7 +1159,7 @@ struct SliceCodeunitsTransform : StringSliceTransformBase {
// or begin_sliced), but begin_sliced and opt.start can be 'out of sync',
// for instance when start=-100, when the string length is only 10.
RETURN_IF_UTF8_ERROR(arrow::util::UTF8AdvanceCodepointsReverse(
begin_sliced, end, &end_sliced, -opt.stop));
begin_sliced, end, &end_sliced, Negate(opt.stop)));
} else {
// zero length slice
return 0;
Expand Down Expand Up @@ -1214,11 +1215,12 @@ struct SliceCodeunitsTransform : StringSliceTransformBase {

// similar to opt.start
if (opt.stop >= 0) {
int64_t length = std::min(opt.stop, std::numeric_limits<int64_t>::max() - 1) + 1;
RETURN_IF_UTF8_ERROR(
arrow::util::UTF8AdvanceCodepoints(begin, end, &end_sliced, opt.stop + 1));
arrow::util::UTF8AdvanceCodepoints(begin, end, &end_sliced, length));
} else {
RETURN_IF_UTF8_ERROR(arrow::util::UTF8AdvanceCodepointsReverse(
begin, end, &end_sliced, -opt.stop - 1));
begin, end, &end_sliced, Negate(opt.stop) - 1));
}
end_sliced--;

Expand All @@ -1240,6 +1242,12 @@ struct SliceCodeunitsTransform : StringSliceTransformBase {
}

#undef RETURN_IF_UTF8_ERROR

private:
static int64_t Negate(int64_t v) {
constexpr auto max = std::numeric_limits<int64_t>::max();
return -max > v ? max : -v;
}
};

template <typename Type>
Expand Down
2 changes: 2 additions & 0 deletions python/pyarrow/_compute.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,8 @@ class SliceOptions(_SliceOptions):
def __init__(self, start, stop=None, step=1):
if stop is None:
stop = sys.maxsize
if step < 0:
stop = -stop
self._set_options(start, stop, step)


Expand Down
2 changes: 1 addition & 1 deletion python/pyarrow/tests/test_compute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ def test_trim():
def test_slice_compatibility():
arr = pa.array(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])
for start in range(-6, 6):
for stop in range(-6, 6):
for stop in itertools.chain(range(-6, 6), [None]):
for step in [-3, -2, -1, 1, 2, 3]:
expected = pa.array([k.as_py()[start:stop:step]
for k in arr])
Expand Down