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

[C++][CI] Debug memory pool interferes with ASAN check #39973

Closed
zanmato1984 opened this issue Feb 7, 2024 · 4 comments · Fixed by #39975
Closed

[C++][CI] Debug memory pool interferes with ASAN check #39973

zanmato1984 opened this issue Feb 7, 2024 · 4 comments · Fixed by #39975
Assignees
@zanmato1984
Labels
Component: C++ Component: Continuous Integration Type: bug
Milestone
16.0.0

Comments

@zanmato1984
Copy link
Contributor

Describe the bug, including details regarding any error messages, version, and platform.

Debug memory pool, introduced in #12330, allocates additional bytes at the end of a buffer to identify unintended writes beyond the buffer’s boundary. However this will prevent ASAN from detecting invalid reads in these extra bytes.

A demo of this problem in action could be found in this CI run https://github.com/zanmato1984/arrow/actions/runs/7752895694/job/21143188221?pr=3#step:6:3370 of PR zanmato1984#3. It indicates that a legacy case is already failing the ASAN check, which I’ll address in a separate issue.

Besides, I’m afraid that some “tail bytes”-kind bugs that I have been recently working on, such as #32570, #39577, #39583, and #39778, could potentially have been caught at a better chance if ASAN check was fully effective.

We should consider disabling debug memory pool for ASAN check in CI.

cc @pitrou @bkietz

Component(s)

C++, Continuous Integration
@zanmato1984
Copy link
Contributor Author

I'm working on adjusting the CI scripts to disable debug memory pool for ASAN. Will send out the PR later.

@zanmato1984
Copy link
Contributor Author

take

@pitrou
Copy link
Member

pitrou commented Feb 7, 2024

That's a good point. We could similarly do this for the Valgrind CI job.

@zanmato1984
Copy link
Contributor Author

That's a good point. We could similarly do this for the Valgrind CI job.

OK, will do.

@github-actions github-actions bot mentioned this issue Feb 7, 2024
Merged
@zanmato1984 zanmato1984 mentioned this issue Feb 7, 2024
Closed
@kou kou changed the title [C++] [CI] Debug memory pool interferes with ASAN check [C++][CI] Debug memory pool interferes with ASAN check Feb 8, 2024
pitrou pushed a commit that referenced this issue Feb 8, 2024
@zanmato1984
GH-39973: [C++][CI] Disable debug memory pool for ASAN and Valgrind (#…
a946214 
…39975)

### Rationale for this change

Disable debug memory pool for ASAN and Valgrind so that they can detect more subtle memory issues regarding to buffer tail bytes.

### What changes are included in this PR?

1. Add a `none` option to debug memory pool env var to make other things slightly easier.
2. Change `*_test.sh` scripts to conditionally set debug memory pool env var.
3. Top-level docker compose change to pass none to debug memory pool env var for ASAN and Valgrind.

### Are these changes tested?

The CI should cover it well.

### Are there any user-facing changes?

No.

* Closes: #39973

Authored-by: Ruoxi Sun <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@pitrou pitrou added this to the 16.0.0 milestone Feb 8, 2024
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@zanmato1984 @dgreiss
apacheGH-39973: [C++][CI] Disable debug memory pool for ASAN and Valg…
516f823 
…rind (apache#39975)

### Rationale for this change

Disable debug memory pool for ASAN and Valgrind so that they can detect more subtle memory issues regarding to buffer tail bytes.

### What changes are included in this PR?

1. Add a `none` option to debug memory pool env var to make other things slightly easier.
2. Change `*_test.sh` scripts to conditionally set debug memory pool env var.
3. Top-level docker compose change to pass none to debug memory pool env var for ASAN and Valgrind.

### Are these changes tested?

The CI should cover it well.

### Are there any user-facing changes?

No.

* Closes: apache#39973

Authored-by: Ruoxi Sun <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
zanmato1984 added a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
@zanmato1984
apacheGH-39973: [C++][CI] Disable debug memory pool for ASAN and Valg…
440c1ae 
…rind (apache#39975)

### Rationale for this change

Disable debug memory pool for ASAN and Valgrind so that they can detect more subtle memory issues regarding to buffer tail bytes.

### What changes are included in this PR?

1. Add a `none` option to debug memory pool env var to make other things slightly easier.
2. Change `*_test.sh` scripts to conditionally set debug memory pool env var.
3. Top-level docker compose change to pass none to debug memory pool env var for ASAN and Valgrind.

### Are these changes tested?

The CI should cover it well.

### Are there any user-facing changes?

No.

* Closes: apache#39973

Authored-by: Ruoxi Sun <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
@zanmato1984 @thisisnic
apacheGH-39973: [C++][CI] Disable debug memory pool for ASAN and Valg…
8921173 
…rind (apache#39975)

### Rationale for this change

Disable debug memory pool for ASAN and Valgrind so that they can detect more subtle memory issues regarding to buffer tail bytes.

### What changes are included in this PR?

1. Add a `none` option to debug memory pool env var to make other things slightly easier.
2. Change `*_test.sh` scripts to conditionally set debug memory pool env var.
3. Top-level docker compose change to pass none to debug memory pool env var for ASAN and Valgrind.

### Are these changes tested?

The CI should cover it well.

### Are there any user-facing changes?

No.

* Closes: apache#39973

Authored-by: Ruoxi Sun <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zanmato1984 zanmato1984

Labels
Component: C++ Component: Continuous Integration Type: bug
Projects
None yet
Milestone
16.0.0
Development

Successfully merging a pull request may close this issue.

GH-39973: [C++][CI] Disable debug memory pool for ASAN and Valgrind zanmato1984/arrow
2 participants
@zanmato1984 @pitrou