GH-41460: [C++] Use ASAN to poison temp vector stack memory #41695

zanmato1984 · 2024-05-16T14:51:04Z

Rationale for this change

See #41460. And reduce the overhead of current manual poisoning (filling the entire stack space with 0xFFs) that happens even in release mode.

What changes are included in this PR?

Use ASAN API to replace the current manual poisoning of the temp vector stack memory.

Are these changes tested?

Wanted to add cases to assert that ASAN poison/unpoison is functioning. However I found it too tricky to catch an ASAN error because ASAN directly uses signals that are hard to interoperate in C++/gtest. So I just manually checked poisoning is working in my local, e.g. by intentionally not unpoisoning the allocated buffer and seeing ASAN unhappy.

Just leveraging existing cases that use temp stack such as acero tests, which should cover this change well.

Are there any user-facing changes?

None.

GitHub Issue: [C++][Acero] Use ASAN to replace current temp stack memory poisoning #41460

github-actions · 2024-05-16T14:51:31Z

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

zanmato1984 · 2024-05-16T16:40:59Z

Hi @felipecrv , this is a followup of our discussion in #41335 (comment). Think you might want to take a look. Thanks.

zanmato1984 · 2024-05-16T16:42:45Z

cpp/src/arrow/compute/util_internal.cc

@@ -20,16 +20,29 @@
 #include "arrow/compute/util.h"
 #include "arrow/memory_pool.h"

+#ifdef ADDRESS_SANITIZER


Some platforms don't have ASAN, so wrap the ASAN related code with a flag that is only defined with ASAN.

zanmato1984 · 2024-05-16T16:45:05Z

cpp/src/arrow/compute/util_internal.h

+  TempVectorStack() = default;
+  ~TempVectorStack();
+
+  TempVectorStack(const TempVectorStack&) = delete;
+  TempVectorStack& operator=(const TempVectorStack&) = delete;
+
+  TempVectorStack(TempVectorStack&&) = default;
+  TempVectorStack& operator=(TempVectorStack&&) = default;


Boilerplate code to cope with the explicit destructor, otherwise the compiler will complain that implicit copy constructor is deleted due to a member of unique_ptr.

There are two handy macros for you here ->

arrow/cpp/src/arrow/util/macros.h

Lines 27 to 37 in f5ac05c

#ifndef ARROW_DISALLOW_COPY_AND_ASSIGN

#define ARROW_DISALLOW_COPY_AND_ASSIGN(TypeName) \

TypeName(const TypeName&) = delete; \

void operator=(const TypeName&) = delete

#endif

#ifndef ARROW_DEFAULT_MOVE_AND_ASSIGN

#define ARROW_DEFAULT_MOVE_AND_ASSIGN(TypeName) \

TypeName(TypeName&&) = default; \

TypeName& operator=(TypeName&&) = default

#endif

Wow, thanks for tips! Will update.

zanmato1984 · 2024-05-16T16:46:30Z

cpp/src/arrow/compute/util_internal.cc

+  if (buffer_) {
+    ASAN_UNPOISON_MEMORY_REGION(buffer_->mutable_data(), buffer_size_);
+  }


Must unpoison the buffer before releasing.

felipecrv · 2024-05-16T23:53:32Z

cpp/src/arrow/compute/util_internal.h

+  TempVectorStack() = default;
+  ~TempVectorStack();
+
+  TempVectorStack(const TempVectorStack&) = delete;
+  TempVectorStack& operator=(const TempVectorStack&) = delete;
+
+  TempVectorStack(TempVectorStack&&) = default;
+  TempVectorStack& operator=(TempVectorStack&&) = default;


There are two handy macros for you here ->

arrow/cpp/src/arrow/util/macros.h

Lines 27 to 37 in f5ac05c

#ifndef ARROW_DISALLOW_COPY_AND_ASSIGN

#define ARROW_DISALLOW_COPY_AND_ASSIGN(TypeName) \

TypeName(const TypeName&) = delete; \

void operator=(const TypeName&) = delete

#endif

#ifndef ARROW_DEFAULT_MOVE_AND_ASSIGN

#define ARROW_DEFAULT_MOVE_AND_ASSIGN(TypeName) \

TypeName(TypeName&&) = default; \

TypeName& operator=(TypeName&&) = default

#endif

felipecrv · 2024-05-17T00:32:27Z

cpp/src/arrow/compute/util_internal.h

  static int64_t PaddedAllocationSize(int64_t num_bytes);

  void alloc(uint32_t num_bytes, uint8_t** data, int* id);
  void release(int id, uint32_t num_bytes);
-  static constexpr uint64_t kGuard1 = 0x3141592653589793ULL;
-  static constexpr uint64_t kGuard2 = 0x0577215664901532ULL;


I think you should keep these, write them after the poisoning. Poisoning can only help us with invalid reads, but not with undesirable writes.

They were added to help detecting bugs like #11335

Thanks @felipecrv , I'll add them back.

But I'm also putting down some notes here about what I've been thinking, perhaps just as a memo to myself.

I'm removing these guards because ASAN can prevent both invalid reads and writes to poisoned memory. I don't think I can replicate the exact issue that #11335 was trying to fix, but I simply reverted #11335 and ran hash join ut, seeing ASAN successfully caught write after poison.

However there could be more tricky cases like below:

func1 allocates a temp vector v1 and reads/writes to v1;

func1 calls func2 (note: w/o releasing v1 - this is the essential of a "stack" and the tricky part);

func2 allocates another temp vector v2 and reads/writes to v2;

func2 releases v2 and returns to func1;

func1 reads/writes to v1 again;

func1 releases v1 and returns to its caller.

A) Suppose writes to v1 at offset >= v1.size() happen in 1, then ASAN can catch it because that piece of memory is already poisoned.
B) Suppose writes to v1 at offset < 0 happen in 1, then whether ASAN can catch it depends on the validity of the memory address lower than v1, but the guards have a better chance.
C) Suppose writes to v2 at offset >= v2.size() happen in 3, same as A.
D) Suppose writes to v2 at offset < 0 happen in 3, then ASAN has zero chance to catch it as long as the offset is within v1.size(), but the guards have a better chance.
E) Is it possible that func1 writes to v1 at offset >= v1.size() while v2 is still alive? So it is corrupting v2's content without ASAN noticing because v2's memory is unpoisoned throughout v2's lifespan. The answer is NO, the control flow will be in func2 as long as v2 is alive so func1 won't be able to execute any code.

To summarize, ASAN poisoning is not able to 100% cover what the guards can do. The guards have a better chance to catch invalid writes at negative offsets (B and D). But other than that, ASAN is equally good as the guards.

felipecrv · 2024-05-18T03:22:47Z

Thank you for doing this! ASAN is so cool.

conbench-apache-arrow · 2024-05-18T06:55:21Z

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

There were no benchmark performance regressions. 🎉

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

…ache#41695) ### Rationale for this change See apache#41460. And reduce the overhead of current manual poisoning (filling the entire stack space with `0xFF`s) that happens even in release mode. ### What changes are included in this PR? Use ASAN API to replace the current manual poisoning of the temp vector stack memory. ### Are these changes tested? Wanted to add cases to assert that ASAN poison/unpoison is functioning. However I found it too tricky to catch an ASAN error because ASAN directly uses signals that are hard to interoperate in C++/gtest. So I just manually checked poisoning is working in my local, e.g. by intentionally not unpoisoning the allocated buffer and seeing ASAN unhappy. Just leveraging existing cases that use temp stack such as acero tests, which should cover this change well. ### Are there any user-facing changes? None. * GitHub Issue: apache#41460 Authored-by: Ruoxi Sun <zanmato1984@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zanmato1984 added 3 commits May 16, 2024 22:43

WIP

e8d7ab4

WIP

bf9abee

Finalize

e6e5dd8

github-actions bot added Component: C++ awaiting review Awaiting review labels May 16, 2024

zanmato1984 added 3 commits May 16, 2024 22:51

Remove dead code

9da611e

Add macro check for asan code

b75b804

Fix

4100c32

zanmato1984 marked this pull request as ready for review May 16, 2024 16:39

zanmato1984 commented May 16, 2024

View reviewed changes

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

zanmato1984 commented May 16, 2024

View reviewed changes

felipecrv requested changes May 17, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 17, 2024

zanmato1984 added 2 commits May 18, 2024 00:00

Use macros for copy/assign and move/assign

3f5a3f0

Add guard back

142419c

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 17, 2024

felipecrv approved these changes May 18, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 18, 2024

felipecrv merged commit dcdf4e6 into apache:main May 18, 2024
39 of 40 checks passed

felipecrv removed the awaiting merge Awaiting merge label May 18, 2024

felipecrv mentioned this pull request May 18, 2024

[C++][Acero] Use ASAN to replace current temp stack memory poisoning #41460

Closed

zanmato1984 mentioned this pull request May 20, 2024

[C++] Corner case of temp vector stack overflow check #41738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41460: [C++] Use ASAN to poison temp vector stack memory #41695

GH-41460: [C++] Use ASAN to poison temp vector stack memory #41695

zanmato1984 commented May 16, 2024 •

edited

github-actions bot commented May 16, 2024

zanmato1984 commented May 16, 2024

zanmato1984 May 16, 2024

zanmato1984 May 16, 2024

felipecrv May 16, 2024

zanmato1984 May 17, 2024

zanmato1984 May 17, 2024

zanmato1984 May 16, 2024

felipecrv May 16, 2024

felipecrv May 17, 2024

zanmato1984 May 17, 2024

zanmato1984 May 17, 2024

felipecrv commented May 18, 2024

conbench-apache-arrow bot commented May 18, 2024

	#ifndef ARROW_DISALLOW_COPY_AND_ASSIGN
	#define ARROW_DISALLOW_COPY_AND_ASSIGN(TypeName) \
	TypeName(const TypeName&) = delete; \
	void operator=(const TypeName&) = delete
	#endif

	#ifndef ARROW_DEFAULT_MOVE_AND_ASSIGN
	#define ARROW_DEFAULT_MOVE_AND_ASSIGN(TypeName) \
	TypeName(TypeName&&) = default; \
	TypeName& operator=(TypeName&&) = default
	#endif

GH-41460: [C++] Use ASAN to poison temp vector stack memory #41695

GH-41460: [C++] Use ASAN to poison temp vector stack memory #41695

Conversation

zanmato1984 commented May 16, 2024 • edited

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 May 16, 2024

zanmato1984 commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipecrv commented May 18, 2024

conbench-apache-arrow bot commented May 18, 2024

zanmato1984 commented May 16, 2024 •

edited