Skip to content

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

Merged
aduh95 merged 1 commit intonodejs:masterfrom
jasnell:disable-secure-heap-test-for-asan
Jan 12, 2021
Merged

test: disable test-crypto-secure-heap with asan#36900
aduh95 merged 1 commit intonodejs:masterfrom
jasnell:disable-secure-heap-test-for-asan

Conversation

@jasnell
Copy link
Copy Markdown
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 [email protected]

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 72 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
Copy Markdown
Collaborator

nodejs-github-bot commented Jan 12, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/35478/

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

aduh95 commented Jan 12, 2021

fast-track?

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Jan 12, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/35479/

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Jan 12, 2021

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

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Jan 12, 2021

Landed in eef75a957c38

@jasnell jasnell closed this Jan 12, 2021
@aduh95 aduh95 mentioned this pull request Jan 12, 2021
Closed
@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented Jan 12, 2021

There is no commit eef75a957c38 on master..

@aduh95 aduh95 reopened this Jan 12, 2021
@jasnell @aduh95
test: disable test-crypto-secure-heap with asan
6e3a832 
The asan checks don't play well currently with persistent secure
heap allocations.

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#36900
Refs: nodejs#36881
Reviewed-By: Danielle Adams <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@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
Copy Markdown
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
@jasnell @danielleadams
test: disable test-crypto-secure-heap with asan
abc2ff4 
The asan checks don't play well currently with persistent secure
heap allocations.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #36900
Refs: #36881
Reviewed-By: Danielle Adams <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@addaleax
Copy link
Copy Markdown
Member

addaleax commented Jan 16, 2021

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
Copy Markdown
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

Reviewers

@panva panva panva approved these changes

@lpinca lpinca lpinca approved these changes

@richardlau richardlau richardlau approved these changes

@aduh95 aduh95 aduh95 approved these changes

+1 more reviewer

@danielleadams danielleadams danielleadams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-track PRs that do not need to wait for 72 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jasnell @nodejs-github-bot @aduh95 @addaleax @panva @lpinca @richardlau @danielleadams @targos