Skip to content

build: fix HAVE_OPENSSL macro for cctest#17461

Closed
mmarchini wants to merge 1 commit into
nodejs:masterfrom
mmarchini:fix-openssl-cctests
Closed

build: fix HAVE_OPENSSL macro for cctest#17461
mmarchini wants to merge 1 commit into
nodejs:masterfrom
mmarchini:fix-openssl-cctests

Conversation

@mmarchini
Copy link
Copy Markdown
Contributor

@mmarchini mmarchini commented Dec 4, 2017

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

joyeecheung commented Dec 5, 2017

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

apapirovski commented Dec 9, 2017

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

mmarchini commented Dec 26, 2017

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

Labels

build Issues and PRs related to build files or the CI.

9 participants

@mmarchini @joyeecheung @apapirovski @gibfahn @danbev @jasnell @addaleax @BridgeAR @nodejs-github-bot