FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: disable test-crypto-secure-heap with asan by jasnell · Pull Request #36900 · nodejs/node · GitHub

/ node Public

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

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
jasnell:disable-secure-heap-test-for-asan
Jan 12, 2021
ConversationCommits (1)ChecksFiles changed
Merged

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

Conversation

jasnell commented Jan 12, 2021

Copy link
Copy Markdown
Member

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 added the test Issues and PRs related to the tests. label Jan 12, 2021
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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2021

nodejs-github-bot commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

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

danielleadams mentioned this pull request Jan 12, 2021
Merged

aduh95 commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

fast-track?

nodejs-github-bot commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

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

jasnell commented Jan 12, 2021

Copy link
Copy Markdown
Member Author

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

jasnell commented Jan 12, 2021

Copy link
Copy Markdown
Member Author

Landed in eef75a957c38

jasnell closed this Jan 12, 2021
aduh95 mentioned this pull request Jan 12, 2021
Closed

aduh95 commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

There is no commit eef75a957c38 on master..

aduh95 reopened this Jan 12, 2021
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 <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 force-pushed the disable-secure-heap-test-for-asan branch from 3460b40 to 6e3a832 Compare January 12, 2021 22:52

aduh95 commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

Landed in 6e3a832

aduh95 merged commit 6e3a832 into nodejs:master Jan 12, 2021
danielleadams pushed a commit that referenced this pull request Jan 13, 2021
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 <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 commented Jan 16, 2021

Copy link
Copy Markdown
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 commented Jan 17, 2021

Copy link
Copy Markdown
Member Author

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

panva panva approved these changes
lpinca lpinca approved these changes
richardlau richardlau approved these changes
aduh95 aduh95 approved these changes
+1 more reviewer
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 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL