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

build: fix HAVE_OPENSSL macro for cctest #17461

Closed
wants to merge 1 commit into from
Closed

build: fix HAVE_OPENSSL macro for cctest #17461

wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

I found this bug while writing tests for #14901, because Environment::handle_wrap_queue(), Environment::req_wrap_queue() and Environment::context()weren't working as expected inside C++ tests.

cctest build target wasn't defining the HAVE_OPENSSL macro when node_use_openssl was true, causing inconsistencies on most node::Environment member's addresses (every member defined after AsyncHooks async_hooks_). For example, if someone wanted to access the context of an environment by using Environment::context(), the object returned by the function was pointing to an invalid address, causing a segmentation fault.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

build: fix HAVE_OPENSSL macro for cctest
e093ebd 
cctest build target wasn't defining the HAVE_OPENSSL macro when
node_use_openssl was true, causing incosistencies on most
`node::Environment` member's addresses. For example, if someone wanted
to access the context of an environment by using
`node::Environment::context()`, the object returned by the function was
pointing to an invalid address.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Dec 4, 2017
@mmarchini mmarchini mentioned this pull request Dec 4, 2017
Closed
2 tasks
@joyeecheung
Copy link
Member

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 8, 2017
@apapirovski
Copy link
Member

Landed in c1689dc

@apapirovski apapirovski closed this Dec 9, 2017
apapirovski pushed a commit that referenced this pull request Dec 9, 2017
@apapirovski
build: define HAVE_OPENSSL macro for cctest
c1689dc 
cctest build target wasn't defining the HAVE_OPENSSL macro when
node_use_openssl was true, causing inconsistencies on most
`node::Environment` member's addresses. For example, if someone
wanted to access the context of an environment by using
`node::Environment::context()`, the object returned by the
function was pointing to an invalid address.

PR-URL: #17461
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@MylesBorins
build: define HAVE_OPENSSL macro for cctest
491db47 
cctest build target wasn't defining the HAVE_OPENSSL macro when
node_use_openssl was true, causing inconsistencies on most
`node::Environment` member's addresses. For example, if someone
wanted to access the context of an environment by using
`node::Environment::context()`, the object returned by the
function was pointing to an invalid address.

PR-URL: #17461
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@MylesBorins
build: define HAVE_OPENSSL macro for cctest
5d1463a 
cctest build target wasn't defining the HAVE_OPENSSL macro when
node_use_openssl was true, causing inconsistencies on most
`node::Environment` member's addresses. For example, if someone
wanted to access the context of an environment by using
`node::Environment::context()`, the object returned by the
function was pointing to an invalid address.

PR-URL: #17461
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@gibfahn
build: define HAVE_OPENSSL macro for cctest
9c93b97 
cctest build target wasn't defining the HAVE_OPENSSL macro when
node_use_openssl was true, causing inconsistencies on most
`node::Environment` member's addresses. For example, if someone
wanted to access the context of an environment by using
`node::Environment::context()`, the object returned by the
function was pointing to an invalid address.

PR-URL: #17461
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@gibfahn
build: define HAVE_OPENSSL macro for cctest
19c1c71 
cctest build target wasn't defining the HAVE_OPENSSL macro when
node_use_openssl was true, causing inconsistencies on most
`node::Environment` member's addresses. For example, if someone
wanted to access the context of an environment by using
`node::Environment::context()`, the object returned by the
function was pointing to an invalid address.

PR-URL: #17461
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@gibfahn
build: define HAVE_OPENSSL macro for cctest
6ed3306 
cctest build target wasn't defining the HAVE_OPENSSL macro when
node_use_openssl was true, causing inconsistencies on most
`node::Environment` member's addresses. For example, if someone
wanted to access the context of an environment by using
`node::Environment::context()`, the object returned by the
function was pointing to an invalid address.

PR-URL: #17461
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mmarchini
Copy link
Contributor Author

I'm +1 on dont-land-on-v6.x here, looking at node.gypi and async-wrap.h I'm almost sure this bug is not present on v6.x (couldn't test it though). Besides that, there are only a couple of cctests on v6.x (none of them using node::Environment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbev danbev danbev approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mmarchini @joyeecheung @apapirovski @gibfahn @danbev @jasnell @addaleax @BridgeAR @nodejs-github-bot