GH-39577: [C++] Fix tail-word access cross buffer boundary in `CompareBinaryColumnToRow` by zanmato1984 · Pull Request #39606 · apache/arrow

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


 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

- 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

+ 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>

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

github-actionsBot 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 CompareBinaryColumnToRowJan 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-actionsBot 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:mainJan 16, 2024

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

pitrou modified the milestone: 15.0.0 Jan 16, 2024


		arrow_install_all_headers("arrow/compute/row")

		add_arrow_compute_test(compare_test SOURCES compare_test.cc)

Uh oh!

Conversation

zanmato1984 commented Jan 15, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actionsBot commented Jan 15, 2024

Uh oh!

pitrouJan 16, 2024

Choose a reason for hiding this comment

Uh oh!

zanmato1984Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

pitrouJan 16, 2024

Choose a reason for hiding this comment

Uh oh!

zanmato1984Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

pitrouJan 16, 2024

Choose a reason for hiding this comment

Uh oh!

pitrouJan 16, 2024

Choose a reason for hiding this comment

Uh oh!

zanmato1984Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jan 16, 2024

Uh oh!

zanmato1984 commented Jan 16, 2024

Uh oh!

pitrou commented Jan 16, 2024

Uh oh!

github-actionsBot commented Jan 16, 2024

Uh oh!

pitrou commented Jan 16, 2024

Uh oh!

raulcd commented Jan 16, 2024

Uh oh!

pitrou commented Jan 16, 2024

Uh oh!

conbench-apache-arrowBot commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zanmato1984 commented Jan 15, 2024•
edited
Loading