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: export openssl symbols on windows (v4.x) #7676

Closed
wants to merge 205 commits into from
Closed

build: export openssl symbols on windows (v4.x) #7676

wants to merge 205 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 12, 2016 •
edited
Loading

R=@thealphanerd

TBD if it should be back-ported to LTS. There was at least one report of add-on breakage caused by exporting openssl symbols (ref: #7608) although that was arguably more of an issue with the add-on.

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 12, 2016
@bnoordhuis bnoordhuis mentioned this pull request Jul 12, 2016
Merged
@mscdex mscdex added windows Issues and PRs related to the Windows platform. openssl Issues and PRs related to the OpenSSL dependency. v4.x labels Jul 12, 2016
@MylesBorins
Copy link
Contributor

@bnoordhuis can you rebase? I'm going to set this for the LTS agenda so we can discuss the potential breakages before landing.

@ghost
Copy link

ghost commented Jul 12, 2016

If this gets to land, you should also look to land this: #7576

@bnoordhuis bnoordhuis force-pushed the backport-pr6274-v4.x branch from ecbcea2 to 63dfd85 Compare July 13, 2016 04:33
@bnoordhuis
Copy link
Member Author

Rebased. New CI: https://ci.nodejs.org/job/node-test-pull-request/3274/

@bnoordhuis
Copy link
Member Author

There is no technical reason #7576 couldn't land together with this PR but it sets a weird precedent of landing changes in the LTS branch that haven't landed in the stable branch yet. I don't think we want to go there but I'll leave that decision to our release people.

@bnoordhuis
Copy link
Member Author

There is something funny going on with the freebsd buildbot:

./node: not found
./node: not found
./node: not found
# etc.

The build does seem to succeed though. Other buildbots are green.

@MylesBorins MylesBorins mentioned this pull request Jul 13, 2016
Closed
@MylesBorins
Copy link
Contributor

In today's LTS WG meeting we discussed this PR. The opinion was that The potential breakages are a red flag, but this is definitely a candidate if it is going to fix building addons with openssl for windows (without requiring weird hacks).

@bnoordhuis would you be willing to put together a Postmortem on this bug. Specifically how long it has been open, and when these changes landed. There are quite a few different threads and it isn't exactly clear when things have happened / landed.

@bnoordhuis
Copy link
Member Author

If by postmortem you mean a quick comment summarizing the chain of events, sure.

  1. Add-ons not linking against the bundled OpenSSL on Windows - forever.
  2. Me putting together a smoke test (test: add openssl add-on test #6274) - April this year.
  3. Chipping away at the problem - on and off in April, May and June (11 attempts!)
  4. Victory - just over a week ago, landed in v6.3.0.
  5. Reported add-on build breakage - five days ago in Windows Node 6.3.0 binary exports SSL symbols #7608. Probably a configuration issue in that add-on exposed by test: add openssl add-on test #6274, see Windows Node 6.3.0 binary exports SSL symbols #7608 (comment).

@MylesBorins
Copy link
Contributor

@bnoordhuis this is going to need to be rebased one more time. Since I don't think it is going to land in v4.5.0 I think you should hold off on doing so until after the release to avoid doing it again. I'll ping you when it will be a good time

@jasnell jasnell mentioned this pull request Aug 5, 2016
Closed
@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

The @nodejs/lts WG discussed this today and couldn't come up with a pressing justification to land it in v4.x. @bnoordhuis, if you think this absolutely should be backported, let us know, otherwise we're inclined to leave it be in v4.x

@bnoordhuis bnoordhuis force-pushed the backport-pr6274-v4.x branch from 63dfd85 to e9d7232 Compare August 9, 2016 11:13
@bnoordhuis
Copy link
Member Author

Without this change, it's impossible to build add-ons on Windows that link against the bundled openssl. That seems like pretty good justification to me.

I've rebased the PR, by the way.

@ghost
Copy link

ghost commented Aug 9, 2016

So what about the two accompanying build fixes: #7983 #7576

Especially 7576 as that fix is a direct response to this fix (we missed exporting all OpenSSL functions, so some addons, like my own, still cannot use OpenSSL without 7576).

Same goes with zlib which is available on OS X and Linux but not Windows, until 7983.

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

I don't think anyone felt super strongly about not having it in v4.x. The key concern would be whether or not there's a non-zero chance of this breaking anyone.

@bnoordhuis
Copy link
Member Author

The only bug report was from someone with an in-house add-on with an ad hoc build system.

I think a compelling case can be made that this change is good for security. Add-ons on Windows won't have to ship (and keep up to date) their own copy of openssl anymore.

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

Ok, @nodejs/lts ... given that, do any of you feel strongly about this not being backported?

@bnoordhuis
build: add --enable-d8 configure option
e4690c2 
Add an option to the configure script for building d8.  Useful for
testing V8 standalone.

PR-URL: nodejs#7538
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Trott and others added 18 commits October 10, 2016 19:11
@Trott
test: add assert.notDeepStrictEqual() tests
275386e 
There is no test coverage for `assert.notDeepStrictEqual()`. Add some
minimal tests.

PR-URL: nodejs#8177
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
test: refactor test-tick-processor
ec99640 
The test does some extra work that isn't necessary because of the way
temp directories are handled. The test removes all files from the temp
directory with `common.refreshTmpDir()` but still filters the results
even though only its files will be in the directory).

Refactor to remove that unneeded logic.

PR-URL: nodejs#8180
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@TimothyGu
doc/json: make sure links are correctly passed to marked
e8aa558 
Previously, an attempt was made to make sure the links state is
inherited. Unfortunately, this support was not complete, which
results in various unresolved links in the JSON output (as an
example, [1] contains `initialized by calling
[<code>buf.fill(fill, encoding)</code>][<code>buf.fill()</code>]`).

This commit completes that attempt. After this commit, individual
instances of the parser (for descriptions) inherit the links state
from the root lexer, so that individual Markdown links in descriptions
could be resolved. That same example is now substituted with
`initialized by calling <a href=\"#buffer_buf_fill_value_offset_end_encoding\"><code>buf.fill(fill, encoding)</code></a>`.

[1]: https://nodejs.org/api/buffer.json

PR-URL: nodejs#8494
Reviewed-By: Roman Reiss <me@silverwind.io>
@ravindrabarthwal
doc: add missing semicolon
c795494 
PR-URL: nodejs#7915
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex
doc: fix broken link in dgram doc
75a40d1 
PR-URL: nodejs#8365
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@joaocgreis
win,build: fail on invalid option in vcbuild
bc8431e 
PR-URL: nodejs#8412
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@joaocgreis
win,build: skip finding VS when not needed
91a23b1 
PR-URL: nodejs#8412
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@joaocgreis
win,build: exit when addons fail to build
18f0348 
PR-URL: nodejs#8412
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@joaocgreis
win,build: forward release_urlbase to configure
1b318e8 
The RELEASE_URLBASE environment variable is used in releases as
a prefix for links in the process.release object. The Makefile picks
it and forwards it to configure, but vcbuild.bat did not. Hence, in
Windows, Node releases have a correct process.release because it uses
the default URL, but nightlies, RCs and so on do not, breaking
node-gyp. This enables native modules to be built with such versions
of Node.

PR-URL: nodejs#8430
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
@thefourtheye
tls: copy the Buffer object before using
2847490 
`convertNPNProtocols` and `convertALPNProtocols' uses the `protocols`
buffer object as it is, and if it is modified outside of core, it
might have an impact. This patch makes a copy of the buffer object,
before using it.

PR-URL: nodejs#8055
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@bnoordhuis
src: fix handle leak in Buffer::New()
961b354 
Fix handle leaks in Buffer::New() and Buffer::Copy() by creating the
handle scope before looking up the env with Environment::GetCurrent().

Environment::GetCurrent() calls v8::Isolate::GetCurrentContext(), which
creates a handle in the current scope, i.e., the scope created by the
caller of Buffer::New() or Buffer::Copy().

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis
src: fix handle leak in BuildStatsObject()
11f1da8 
Create a handle scope before performing a check that creates a handle,
otherwise the handle is leaked into the handle scope of the caller.

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis
src: fix handle leak in UDPWrap::Instantiate()
d6cac6d 
Create a handle scope before performing a check that creates a handle,
otherwise the handle is leaked into the handle scope of the caller.

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis
src: remove unnecessary HandleScopes
6f3ac5d 
API function callbacks run inside an implicit HandleScope.  We don't
need to explicitly create one and in fact introduce some unnecessary
overhead when we do.

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bagder @MylesBorins
deps: avoid single-byte buffer overwrite
1f900b6 
Incorrect string length calculation when passing escaped dot.

- CVE: CVE-2016-5180
- Upstream bug: https://c-ares.haxx.se/adv_20160929.html

Ref: nodejs#9037
PR-URL: nodejs#8849
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@addaleax
build: run npm install for doc builds in tarball
f820e02 
Run `npm install` before building the documentation from release
tarballs. The doctool currently depends on `js-yaml`, which
is imported from the `tools/eslint` subtree; however, release
tarballs don’t contain that directory.

Running `npm install` is clearly not a beautiful solution,
but it works.

Fixes: nodejs#7872
PR-URL: nodejs#8413
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
build: export openssl symbols on windows
c97d1c2 
Export symbols from the bundled openssl for add-ons to link against.

Fixes: nodejs/node-v0.x-archive#4051
PR-URL: nodejs#6274
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
build: export more openssl symbols on Windows
5087643 
This exports even more openssl symbols when building
on Windows. SSL_set_fd is one example of added symbol.

PR-URL: nodejs#7576
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
@bnoordhuis bnoordhuis force-pushed the backport-pr6274-v4.x branch from e9d7232 to 5087643 Compare October 18, 2016 16:14
@bnoordhuis
Copy link
Member Author

@nodejs/lts Rebased and included #7576.

@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 24, 2016
@MylesBorins
Copy link
Contributor

@bnoordhuis I've added the semver minor tag for the inclusion of #7576.

@MylesBorins MylesBorins added this to the v4.7.0 milestone Oct 24, 2016
@MylesBorins
Copy link
Contributor

landed in 279e30c...84849f1

@MylesBorins MylesBorins removed this from the 4.7.0 milestone Nov 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. semver-minor PRs that contain new features and should be released in the next minor version. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.