GH-41407: [C++] Use static method to fill scalar scratch space to prevent ub #41421

zanmato1984 · 2024-04-28T14:03:22Z

Rationale for this change

In #40237, I introduced scalar scratch space filling in concrete scalar sub-class constructor, in which there is a static down-casting of this to sub-class pointer. Though this is common in CRTP, it happens in base cast constructor. And this is reported in #41407 to be UB by UBSAN's "vptr" sanitizing.

I'm not a language lawyer to tell if this is a true/false-positive. So I proposed two approaches:

The easy way: add suppression in [1], like we already did for shared_ptr. But apparently this won't be feasible if this is a true-positive (need some language lawyer's help to confirm).
The hard way: totally avoid this so-to-speak UB but may introduce more boilerplate code. This PR is the hard way.

[1] https://github.com/apache/arrow/blob/main/r/tools/ubsan.supp

What changes are included in this PR?

Make FillScratchSpace static.

Are these changes tested?

The existing UT should cover it well.

Are there any user-facing changes?

None.

GitHub Issue: [C++][R] R Sanitizer nightly error #41407

github-actions · 2024-04-28T14:03:47Z

⚠️ GitHub issue #41407 has been automatically assigned in GitHub to PR creator.

jonkeane · 2024-04-28T14:15:39Z

@github-actions crossbow submit test-fedore-r-clang-sanitizer test-ubuntu-r-sanitizer

github-actions · 2024-04-28T14:16:47Z

Unable to match any tasks for `test-fedore-r-clang-sanitizer`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/8868210807

jonkeane · 2024-04-28T14:30:12Z

@github-actions crossbow submit test-fedora-r-clang-sanitizer test-ubuntu-r-sanitizer

github-actions · 2024-04-28T14:32:35Z

Revision: 0f85a7b

Submitted crossbow builds: ursacomputing/crossbow @ actions-b62c1c107c

Task	Status
test-fedora-r-clang-sanitizer
test-ubuntu-r-sanitizer

….4 cleanups (#41403) ### Rationale for this change Keep up with the state of the world, ensure we are maintaining backwards compatibility. Resolves #41402 ### What changes are included in this PR? * Bump to 4.4 as the release * Remove old 3.6 jobs now that we no longer support that; clean up code where we hardcode things fro 3.6 and below * Move many of our CI jobs to [rhub's new containers](https://github.com/r-hub/containers). We were accidentally running stale R devel (from December 2023) because the other rhub images stopped being updated. (One exception to be done as a follow on: #41416) * Resolve a number of extended test failures With this PR R extended tests should be all green with the exceptions of: * Two sanitizer jobs (test-fedora-r-clang-sanitizer, test-ubuntu-r-sanitizer) — which are being investigated / fixed in #41421 * Valgrind — I'm running one last run with a new suppression file. * Binary jobs — these work but fail at upload, see #41403 (comment) * Windows R Release — failing on main, #41398 ### Are these changes tested? By definition. ### Are there any user-facing changes? No. * GitHub Issue: #41402 Lead-authored-by: Jonathan Keane <jkeane@gmail.com> Co-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

jorisvandenbossche · 2024-04-30T07:19:47Z

@github-actions crossbow submit test-ubuntu-22.04-cpp-emscripten

jorisvandenbossche · 2024-04-30T07:21:19Z

(ignore my crossbow trigger, this PR is not going to fix that)

github-actions · 2024-04-30T07:22:04Z

Revision: 0f85a7b

Submitted crossbow builds: ursacomputing/crossbow @ actions-87bb998c79

Task	Status
test-ubuntu-22.04-cpp-emscripten

bkietz

I think this approach is preferable to a suppression, LGTM.

bkietz · 2024-04-30T15:57:06Z

cpp/src/arrow/scalar.h

+ BinaryScalar(std::string s, std::shared_ptr<DataType> type)
+ : BaseBinaryScalar(std::move(s), std::move(type)),
+ ArraySpanFillFromScalarScratchSpace(this->value) {}


FWIW, from my read I think this was actually UB because base class constructors run first. For example in the case of BinaryScalar, the constructor for ArraySpanFillFromScalarScratchSpace<BinaryScalar> ran first and downcast itself to a BinaryScalar which hadn't been constructed yet. The way you have it written now, BaseBinaryScalar will be constructed (including its value) before ArraySpanFillFromScalarScratchSpace references it.

Yeah, I agree. Thank you for the elaboration.

cpp/src/arrow/scalar.cc

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

zanmato1984 · 2024-04-30T16:46:41Z

Comments addressed. PTAL @bkietz . And would you please help to trigger test-fedora-r-clang-sanitizer and test-ubuntu-r-sanitizer CI tasks? I want to make sure the updated change still pass them. Thank you!

jonkeane · 2024-04-30T16:47:10Z

@github-actions crossbow submit test-fedora-r-clang-sanitizer test-ubuntu-r-sanitizer

jonkeane · 2024-04-30T16:48:05Z

I've triggered the sanitizer jobs.

IIUC, I think you can trigger them on your own with a comment like the one I sent @zanmato1984 I don't believe there are restrictions on who is allowed to send that comment (and our CI here looks for the comment and then does the rest)

github-actions · 2024-04-30T16:49:26Z

Revision: a8d8b67

Submitted crossbow builds: ursacomputing/crossbow @ actions-c36b6c7141

Task	Status
test-fedora-r-clang-sanitizer
test-ubuntu-r-sanitizer

zanmato1984 · 2024-04-30T16:55:07Z

I've triggered the sanitizer jobs.

IIUC, I think you can trigger them on your own with a comment like the one I sent @zanmato1984 I don't believe there are restrictions on who is allowed to send that comment (and our CI here looks for the comment and then does the rest)

Wow, thank you for the tips. I'll try myself next time :)

conbench-apache-arrow · 2024-05-01T00:25:33Z

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

There were no benchmark performance regressions. 🎉

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

…er R 4.4 cleanups (apache#41403) ### Rationale for this change Keep up with the state of the world, ensure we are maintaining backwards compatibility. Resolves apache#41402 ### What changes are included in this PR? * Bump to 4.4 as the release * Remove old 3.6 jobs now that we no longer support that; clean up code where we hardcode things fro 3.6 and below * Move many of our CI jobs to [rhub's new containers](https://github.com/r-hub/containers). We were accidentally running stale R devel (from December 2023) because the other rhub images stopped being updated. (One exception to be done as a follow on: apache#41416) * Resolve a number of extended test failures With this PR R extended tests should be all green with the exceptions of: * Two sanitizer jobs (test-fedora-r-clang-sanitizer, test-ubuntu-r-sanitizer) — which are being investigated / fixed in apache#41421 * Valgrind — I'm running one last run with a new suppression file. * Binary jobs — these work but fail at upload, see apache#41403 (comment) * Windows R Release — failing on main, apache#41398 ### Are these changes tested? By definition. ### Are there any user-facing changes? No. * GitHub Issue: apache#41402 Lead-authored-by: Jonathan Keane <jkeane@gmail.com> Co-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

…to prevent ub (apache#41421) ### Rationale for this change In apache#40237, I introduced scalar scratch space filling in concrete scalar sub-class constructor, in which there is a static down-casting of `this` to sub-class pointer. Though this is common in CRTP, it happens in base cast constructor. And this is reported in apache#41407 to be UB by UBSAN's "vptr" sanitizing. I'm not a language lawyer to tell if this is a true/false-positive. So I proposed two approaches: 1. The easy way: add suppression in [1], like we already did for `shared_ptr`. But apparently this won't be feasible if this is a true-positive (need some language lawyer's help to confirm). 2. The hard way: totally avoid this so-to-speak UB but may introduce more boilerplate code. This PR is the hard way. [1] https://github.com/apache/arrow/blob/main/r/tools/ubsan.supp ### What changes are included in this PR? Make `FillScratchSpace` static. ### Are these changes tested? The existing UT should cover it well. ### Are there any user-facing changes? None. * GitHub Issue: apache#41407 Lead-authored-by: Ruoxi Sun <zanmato1984@gmail.com> Co-authored-by: Rossi Sun <zanmato1984@gmail.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>

…vent ub (#41421) ### Rationale for this change In #40237, I introduced scalar scratch space filling in concrete scalar sub-class constructor, in which there is a static down-casting of `this` to sub-class pointer. Though this is common in CRTP, it happens in base cast constructor. And this is reported in #41407 to be UB by UBSAN's "vptr" sanitizing. I'm not a language lawyer to tell if this is a true/false-positive. So I proposed two approaches: 1. The easy way: add suppression in [1], like we already did for `shared_ptr`. But apparently this won't be feasible if this is a true-positive (need some language lawyer's help to confirm). 2. The hard way: totally avoid this so-to-speak UB but may introduce more boilerplate code. This PR is the hard way. [1] https://github.com/apache/arrow/blob/main/r/tools/ubsan.supp ### What changes are included in this PR? Make `FillScratchSpace` static. ### Are these changes tested? The existing UT should cover it well. ### Are there any user-facing changes? None. * GitHub Issue: #41407 Lead-authored-by: Ruoxi Sun <zanmato1984@gmail.com> Co-authored-by: Rossi Sun <zanmato1984@gmail.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>

…to prevent ub (apache#41421) ### Rationale for this change In apache#40237, I introduced scalar scratch space filling in concrete scalar sub-class constructor, in which there is a static down-casting of `this` to sub-class pointer. Though this is common in CRTP, it happens in base cast constructor. And this is reported in apache#41407 to be UB by UBSAN's "vptr" sanitizing. I'm not a language lawyer to tell if this is a true/false-positive. So I proposed two approaches: 1. The easy way: add suppression in [1], like we already did for `shared_ptr`. But apparently this won't be feasible if this is a true-positive (need some language lawyer's help to confirm). 2. The hard way: totally avoid this so-to-speak UB but may introduce more boilerplate code. This PR is the hard way. [1] https://github.com/apache/arrow/blob/main/r/tools/ubsan.supp ### What changes are included in this PR? Make `FillScratchSpace` static. ### Are these changes tested? The existing UT should cover it well. ### Are there any user-facing changes? None. * GitHub Issue: apache#41407 Lead-authored-by: Ruoxi Sun <zanmato1984@gmail.com> Co-authored-by: Rossi Sun <zanmato1984@gmail.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>

…er R 4.4 cleanups (apache#41403) ### Rationale for this change Keep up with the state of the world, ensure we are maintaining backwards compatibility. Resolves apache#41402 ### What changes are included in this PR? * Bump to 4.4 as the release * Remove old 3.6 jobs now that we no longer support that; clean up code where we hardcode things fro 3.6 and below * Move many of our CI jobs to [rhub's new containers](https://github.com/r-hub/containers). We were accidentally running stale R devel (from December 2023) because the other rhub images stopped being updated. (One exception to be done as a follow on: apache#41416) * Resolve a number of extended test failures With this PR R extended tests should be all green with the exceptions of: * Two sanitizer jobs (test-fedora-r-clang-sanitizer, test-ubuntu-r-sanitizer) — which are being investigated / fixed in apache#41421 * Valgrind — I'm running one last run with a new suppression file. * Binary jobs — these work but fail at upload, see apache#41403 (comment) * Windows R Release — failing on main, apache#41398 ### Are these changes tested? By definition. ### Are there any user-facing changes? No. * GitHub Issue: apache#41402 Lead-authored-by: Jonathan Keane <jkeane@gmail.com> Co-authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

…to prevent ub (apache#41421) ### Rationale for this change In apache#40237, I introduced scalar scratch space filling in concrete scalar sub-class constructor, in which there is a static down-casting of `this` to sub-class pointer. Though this is common in CRTP, it happens in base cast constructor. And this is reported in apache#41407 to be UB by UBSAN's "vptr" sanitizing. I'm not a language lawyer to tell if this is a true/false-positive. So I proposed two approaches: 1. The easy way: add suppression in [1], like we already did for `shared_ptr`. But apparently this won't be feasible if this is a true-positive (need some language lawyer's help to confirm). 2. The hard way: totally avoid this so-to-speak UB but may introduce more boilerplate code. This PR is the hard way. [1] https://github.com/apache/arrow/blob/main/r/tools/ubsan.supp ### What changes are included in this PR? Make `FillScratchSpace` static. ### Are these changes tested? The existing UT should cover it well. ### Are there any user-facing changes? None. * GitHub Issue: apache#41407 Lead-authored-by: Ruoxi Sun <zanmato1984@gmail.com> Co-authored-by: Rossi Sun <zanmato1984@gmail.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>

…h space to prevent ub (apache#41421)" This reverts commit 0ef7351.

zanmato1984 added 2 commits April 28, 2024 21:34

Use static method to fill scalar scratch space to prevent ub

0bdd082

Refine ctors

5257476

github-actions bot added Component: C++ awaiting review Awaiting review labels Apr 28, 2024

zanmato1984 mentioned this pull request Apr 28, 2024

[C++][R] R Sanitizer nightly error #41407

Closed

Fix lint

0f85a7b

jonkeane mentioned this pull request Apr 28, 2024

GH-41402: [CI][R] Update our backwards compatibility CI any other R 4.4 cleanups #41403

Merged

bkietz self-requested a review April 30, 2024 15:45

bkietz requested changes Apr 30, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 30, 2024

zanmato1984 and others added 2 commits May 1, 2024 00:35

Update cpp/src/arrow/scalar.cc

7254992

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

Update cpp/src/arrow/scalar.cc

93c89d9

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 30, 2024

Address comment

a8d8b67

bkietz approved these changes Apr 30, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 30, 2024

bkietz merged commit 0ef7351 into apache:main Apr 30, 2024
36 of 38 checks passed

bkietz removed the awaiting merge Awaiting merge label Apr 30, 2024

jonkeane mentioned this pull request Jun 20, 2024

[CI][R] Resolve Valgrind errors #42234

Closed

jonkeane added a commit to jonkeane/arrow that referenced this pull request Jun 21, 2024

Revert "apacheGH-41407: [C++] Use static method to fill scalar scratc…

109db2d

…h space to prevent ub (apache#41421)" This reverts commit 0ef7351.

jonkeane mentioned this pull request Jun 21, 2024

[WIP] Revert "GH-41407: [C++] Use static method to fill scalar scratch spac… #42238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41407: [C++] Use static method to fill scalar scratch space to prevent ub #41421

GH-41407: [C++] Use static method to fill scalar scratch space to prevent ub #41421

zanmato1984 commented Apr 28, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Apr 28, 2024

jonkeane commented Apr 28, 2024

github-actions bot commented Apr 28, 2024

jonkeane commented Apr 28, 2024

github-actions bot commented Apr 28, 2024

jorisvandenbossche commented Apr 30, 2024

jorisvandenbossche commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

bkietz left a comment

bkietz Apr 30, 2024

zanmato1984 Apr 30, 2024

zanmato1984 commented Apr 30, 2024

jonkeane commented Apr 30, 2024

jonkeane commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

zanmato1984 commented Apr 30, 2024

conbench-apache-arrow bot commented May 1, 2024

GH-41407: [C++] Use static method to fill scalar scratch space to prevent ub #41421

GH-41407: [C++] Use static method to fill scalar scratch space to prevent ub #41421

Conversation

zanmato1984 commented Apr 28, 2024 • edited by github-actions bot 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 Apr 28, 2024

jonkeane commented Apr 28, 2024

github-actions bot commented Apr 28, 2024

jonkeane commented Apr 28, 2024

github-actions bot commented Apr 28, 2024

jorisvandenbossche commented Apr 30, 2024

jorisvandenbossche commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

bkietz left a comment

Choose a reason for hiding this comment

bkietz Apr 30, 2024

Choose a reason for hiding this comment

zanmato1984 Apr 30, 2024

Choose a reason for hiding this comment

zanmato1984 commented Apr 30, 2024

jonkeane commented Apr 30, 2024

jonkeane commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

zanmato1984 commented Apr 30, 2024

conbench-apache-arrow bot commented May 1, 2024

zanmato1984 commented Apr 28, 2024 •

edited by github-actions bot

Loading