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.

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

test: disable test-crypto-secure-heap with asan #36900

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 12, 2021

The asan checks don't play well currently with persistent secure
heap allocations.

Signed-off-by: James M Snell jasnell@gmail.com

Refs: #36881

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 12, 2021
@jasnell jasnell added fast-track PRs that do not need to wait for 48 hours to land. request-ci Add this label to start a Jenkins CI on a PR. and removed test Issues and PRs related to the tests. labels Jan 12, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2021
@nodejs-github-bot
Copy link
Collaborator

@danielleadams danielleadams mentioned this pull request Jan 12, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jan 12, 2021

fast-track?

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member Author

jasnell commented Jan 12, 2021

Just waiting on the last CI run to finish then will land this...

@jasnell
Copy link
Member Author

jasnell commented Jan 12, 2021

Landed in eef75a957c38

@aduh95
Copy link
Contributor

aduh95 commented Jan 12, 2021

There is no commit eef75a957c38 on master..

@aduh95 aduh95 reopened this Jan 12, 2021
The asan checks don't play well currently with persistent secure
heap allocations.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36900
Refs: nodejs#36881
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@aduh95 aduh95 force-pushed the disable-secure-heap-test-for-asan branch from 3460b40 to 6e3a832 Compare January 12, 2021 22:52
@aduh95
Copy link
Contributor

aduh95 commented Jan 12, 2021

Landed in 6e3a832

@aduh95 aduh95 merged commit 6e3a832 into nodejs:master Jan 12, 2021
danielleadams pushed a commit that referenced this pull request Jan 13, 2021
The asan checks don't play well currently with persistent secure
heap allocations.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36900
Refs: #36881
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@addaleax
Copy link
Member

Okay, but shouldn't there be at least a TODO comment for actually fixing this memory leak? It doesn't look like there's a call to CRYPTO_secure_malloc_done anywhere in the code.

@jasnell
Copy link
Member Author

jasnell commented Jan 17, 2021

Forgot the todo but adding the CRYPTO_secure_malloc_done alone does not appear to resolve the issue. See https://github.com/nodejs/node/pull/36976/checks?check_run_id=1717392479. I'm still tracking down exactly what the issue is but there appears to be something internally with openssl not freeing the dbrg properly when secure heap is used. Still investigating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants