GH-39577: [C++] Fix tail-word access cross buffer boundary in `CompareBinaryColumnToRow` #39606

zanmato1984 · 2024-01-15T09:08:55Z

Rationale for this change

Default buffer alignment (64b) doesn't guarantee the safety of tail-word access in KeyCompare::CompareBinaryColumnToRow. Comment #39577 (comment) is a concrete example.

What changes are included in this PR?

Make KeyCompare::CompareBinaryColumnToRow tail-word safe.

Are these changes tested?

UT included.

Are there any user-facing changes?

No.

Closes: [C++][Acero] ASAN reports heap buffer overflow in arrow::compute::KeyCompare::CompareBinaryColumnToRow #39577

github-actions · 2024-01-15T09:09:23Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

pitrou · 2024-01-16T14:53:08Z

cpp/src/arrow/compute/row/CMakeLists.txt

@@ -19,3 +19,5 @@
 # in a row-major order.

 arrow_install_all_headers("arrow/compute/row")
+
+add_arrow_compute_test(compare_test SOURCES compare_test.cc)


Could you add this to internals_test instead?

Sure. Done.

pitrou · 2024-01-16T14:59:38Z

cpp/src/arrow/compute/row/compare_internal.cc

-          uint64_t key_left = util::SafeLoad(key_left_ptr + i);
-          uint64_t key_right = key_right_ptr[i];
+          uint64_t key_left = 0;
+          memcpy(&key_left, key_left_ptr + i, length - num_loops_less_one * 8);


length - num_loops_less_one * 8 is used several times in this closure, can you factor it out and give it a meaningful name (perhaps num_tail_bytes)?

But, actually, isn't length - num_loops_less_one * 8 simply equal to length?

length - num_loops_less_one * 8 is used several times in this closure, can you factor it out and give it a meaningful name (perhaps num_tail_bytes)?

Yeah, that's reasonable. Done.

But, actually, isn't length - num_loops_less_one * 8 simply equal to length?

Sorry I don't quite understand how this is coming. For instance, for fsb(19) type, length will be 19 and length - num_loops_less_one * 8 (aka. num_tail_bytes) will be 3.

You're right, thank you.

pitrou · 2024-01-16T15:00:03Z

cpp/src/arrow/compute/row/compare_internal.cc

+          uint64_t key_left = 0;
+          memcpy(&key_left, key_left_ptr + i, length - num_loops_less_one * 8);
+          uint64_t key_right = 0;
+          memcpy(&key_right, key_right_ptr + i, length - num_loops_less_one * 8);
          result_or |= tail_mask & (key_left ^ key_right);


Is tail_mask still useful here? We're extracting exactly the desired number of bytes.

Good catch! Removed tail_mask.

pitrou · 2024-01-16T15:06:34Z

Thanks a lot for finding this out @zanmato1984 ! I posted some comments above.

zanmato1984 · 2024-01-16T15:38:11Z

Thanks a lot for finding this out @zanmato1984 ! I posted some comments above.

Thank you for looking and the very helpful comments @pitrou ! Change updated.

pitrou · 2024-01-16T15:44:36Z

@github-actions crossbow submit -g cpp

github-actions · 2024-01-16T15:47:13Z

Revision: f8cce17

Submitted crossbow builds: ursacomputing/crossbow @ actions-71248b8771

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

pitrou · 2024-01-16T17:14:34Z

@raulcd I think this is a good candidate for 15.0.0.

raulcd · 2024-01-16T17:26:41Z

@pitrou is this worth a new Release Candidate? I created RC1 a couple of hours ago and have already generated all binaries (just waiting for one job to finish) for it successfully, see: #39641

pitrou · 2024-01-16T17:47:01Z

@raulcd Not in itself. But if you have to craft a RC2 for other reasons, this would be nice to include.

conbench-apache-arrow · 2024-01-17T08:55:11Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit cd3321b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…CompareBinaryColumnToRow` (apache#39606) ### Rationale for this change Default buffer alignment (64b) doesn't guarantee the safety of tail-word access in `KeyCompare::CompareBinaryColumnToRow`. Comment apache#39577 (comment) is a concrete example. ### What changes are included in this PR? Make `KeyCompare::CompareBinaryColumnToRow` tail-word safe. ### Are these changes tested? UT included. ### Are there any user-facing changes? No. * Closes: apache#39577 Authored-by: zanmato1984 <zanmato1984@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

…eBinaryColumnToRow` (#39606) ### Rationale for this change Default buffer alignment (64b) doesn't guarantee the safety of tail-word access in `KeyCompare::CompareBinaryColumnToRow`. Comment #39577 (comment) is a concrete example. ### What changes are included in this PR? Make `KeyCompare::CompareBinaryColumnToRow` tail-word safe. ### Are these changes tested? UT included. ### Are there any user-facing changes? No. * Closes: #39577 Authored-by: zanmato1984 <zanmato1984@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

…CompareBinaryColumnToRow` (apache#39606) ### Rationale for this change Default buffer alignment (64b) doesn't guarantee the safety of tail-word access in `KeyCompare::CompareBinaryColumnToRow`. Comment apache#39577 (comment) is a concrete example. ### What changes are included in this PR? Make `KeyCompare::CompareBinaryColumnToRow` tail-word safe. ### Are these changes tested? UT included. ### Are there any user-facing changes? No. * Closes: apache#39577 Authored-by: zanmato1984 <zanmato1984@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

Fix fsb by-word comparison exceeding buffer boundary for tail bytes

22dde21

github-actions bot added Component: C++ awaiting review Awaiting review labels Jan 15, 2024

zanmato1984 changed the title ~~[C++] Fix tail-word access cross buffer boundary in CompareBinaryColumnToRow~~ GH-39577: [C++] Fix tail-word access cross buffer boundary in CompareBinaryColumnToRow Jan 15, 2024

Add case to reproduce the issue

d995836

zanmato1984 marked this pull request as ready for review January 15, 2024 17:18

Fix lint

02f975d

pitrou requested changes Jan 16, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 16, 2024

Address comments

f8cce17

pitrou approved these changes Jan 16, 2024

View reviewed changes

pitrou merged commit cd3321b into apache:main Jan 16, 2024
37 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Jan 16, 2024

pitrou modified the milestone: 15.0.0 Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39577: [C++] Fix tail-word access cross buffer boundary in `CompareBinaryColumnToRow` #39606

GH-39577: [C++] Fix tail-word access cross buffer boundary in `CompareBinaryColumnToRow` #39606

zanmato1984 commented Jan 15, 2024 •

edited

Loading

github-actions bot commented Jan 15, 2024

pitrou Jan 16, 2024

zanmato1984 Jan 16, 2024

pitrou Jan 16, 2024

zanmato1984 Jan 16, 2024

pitrou Jan 16, 2024

pitrou Jan 16, 2024

zanmato1984 Jan 16, 2024

pitrou commented Jan 16, 2024

zanmato1984 commented Jan 16, 2024

pitrou commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

pitrou commented Jan 16, 2024

raulcd commented Jan 16, 2024

pitrou commented Jan 16, 2024

conbench-apache-arrow bot commented Jan 17, 2024

GH-39577: [C++] Fix tail-word access cross buffer boundary in CompareBinaryColumnToRow #39606

GH-39577: [C++] Fix tail-word access cross buffer boundary in CompareBinaryColumnToRow #39606

Conversation

zanmato1984 commented Jan 15, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jan 15, 2024

pitrou Jan 16, 2024

Choose a reason for hiding this comment

zanmato1984 Jan 16, 2024

Choose a reason for hiding this comment

pitrou Jan 16, 2024

Choose a reason for hiding this comment

zanmato1984 Jan 16, 2024

Choose a reason for hiding this comment

pitrou Jan 16, 2024

Choose a reason for hiding this comment

pitrou Jan 16, 2024

Choose a reason for hiding this comment

zanmato1984 Jan 16, 2024

Choose a reason for hiding this comment

pitrou commented Jan 16, 2024

zanmato1984 commented Jan 16, 2024

pitrou commented Jan 16, 2024

github-actions bot commented Jan 16, 2024

pitrou commented Jan 16, 2024

raulcd commented Jan 16, 2024

pitrou commented Jan 16, 2024

conbench-apache-arrow bot commented Jan 17, 2024

GH-39577: [C++] Fix tail-word access cross buffer boundary in `CompareBinaryColumnToRow` #39606

GH-39577: [C++] Fix tail-word access cross buffer boundary in `CompareBinaryColumnToRow` #39606

zanmato1984 commented Jan 15, 2024 •

edited

Loading