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.

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

v8.9.3 proposal #17532

Merged
merged 20 commits into from
Dec 8, 2017
Merged

v8.9.3 proposal #17532

merged 20 commits into from
Dec 8, 2017

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Dec 7, 2017

2017-12-08, Version 8.9.3 'Carbon' (LTS), @MylesBorins

This is a security release. All Node.js users should consult the security release summary at https://nodejs.org/en/blog/vulnerability/december-2017-security-releases/ for details on patched vulnerabilities.

Fixes for the following CVEs are included in this release:

Notable Changes

  • buffer:
    • buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428
  • deps:
    • openssl updated to 1.0.2n (Shigeki Ohtsu) #17526

Notable Changes

  • buffer:
    • buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428
  • deps:
    • openssl updated to 1.0.2n (Shigeki Ohtsu) #17526

Commits

addaleax and others added 9 commits December 7, 2017 13:18
PR-URL: #17428
Backport-PR-URL: #17467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Zero-fill when `Buffer.alloc()` receives invalid fill data.

A solution like #17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of `buffer.fill()` untouched,
since any change to it would be a breaking change, and lets
`Buffer.alloc()` check whether any filling took place or not.

PR-URL: #17428
Backport-PR-URL: #17467
Refs: #17427
Refs: #17423
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This replaces all sources of openssl-1.0.2n.tar.gz into
deps/openssl/openssl

PR-URL: #17526
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
All symlink files in `deps/openssl/openssl/include/openssl/`
are removed and replaced with real header files to avoid
issues on Windows. Two files of opensslconf.h in crypto and
include dir are replaced to refer config/opensslconf.h.

PR-URL: #17526
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reapply b910613 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: #1461
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Regenerate asm files with Makefile and CC=gcc and ASM=nasm where gcc
version was 5.4.0 and nasm version was 2.11.08.

Also asm files in asm_obsolete dir to support old compiler and
assembler are regenerated without CC and ASM envs.

PR-URL: #17526
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. openssl Issues and PRs related to the OpenSSL dependency. v8.x labels Dec 7, 2017
@MylesBorins
Copy link
Contributor Author

@MylesBorins
Copy link
Contributor Author

@jasnell there is a failure on linux1 related to http2. Are there commits that we should be including here that are currently missing?

@jasnell
Copy link
Member

jasnell commented Dec 7, 2017

Shouldn't be, but it's possible that one of the other openssl changes is causing a change in the timing on that one test. I haven't seen that one fail before and this one http2 change should not be causing it.

We can do a quick fix on this to remove the use of TLS in the test case (it's not strictly necessary for this particular case, but it would be a patch over). So long as the other tests are passing, another thing we can do is mark this one as flaky and I'll investigate further after the release

@apapirovski
Copy link
Member

apapirovski commented Dec 7, 2017

It's failing on most of the systems. That's pretty concerning... It's also failing on 9.2.1.

@apapirovski
Copy link
Member

It's passing on OS X just fine, even under heavy load. (And seemed to pass on the CI.) This might be mostly Linux specific.

@MylesBorins
Copy link
Contributor Author

I've managed to backport all the commits necessary to get the http2 tests passing again. Some of these were tagged as semver-minor, but we are going to land them as semver-patch due to http2 being experimental.

There are more commits here then I would generally like to include in a security release, but it is unfortunately the only way to get the http2 tests passing with the security fix that I have found so far. The specific commit that fixes things is 7ceccb1, so if someone can work out a way to get that to land without everything else and fix the timeouts I'm all for it.

Plan is to get this out the door tomorrow

@MylesBorins
Copy link
Contributor Author

@MylesBorins
Copy link
Contributor Author

/cc @nodejs/release @nodejs/http2 @nodejs/tsc so y'all know what is going on

@addaleax
Copy link
Member

addaleax commented Dec 7, 2017

@jasnell @apapirovski ^^^

@MylesBorins
Copy link
Contributor Author

Hooray for whack a mole

now getting failures for parallel/test-http2-client-http1-server

@refack
Copy link
Contributor

refack commented Dec 8, 2017

@MylesBorins I would say do what you're describing but with just d607718, and push sequential/test-http2-timeout-large-write to know-issues. Just because it's minimizes the change-set, and this test assert a fix for an edge case (stream timing out because of a very slow client).

I'm looking into why the first test is failing:

not ok 1957 sequential/test-http2-timeout-large-write
  ---
  duration_ms: 0.733
  severity: fail
  stack: |-
    (node:15186) ExperimentalWarning: The http2 module is an experimental API.
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: Should not timeout
        at ServerHttp2Stream.stream.on (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/sequential/test-http2-timeout-large-write.js:43:12)
        at emitNone (events.js:106:13)
        at ServerHttp2Stream.emit (events.js:208:7)
        at emit (internal/http2/core.js:140:8)
        at _combinedTickCallback (internal/process/next_tick.js:138:11)
        at process._tickCallback (internal/process/next_tick.js:180:9)
  ...

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Dec 8, 2017

@refack the rest of the commits are necessary to fix sequential/test-http2-timeout-large-write. TBH I feel a lot better about landing these extra commits and breaking a test that never worked vs moving a test that was working to known issues

more specifically it is not until 7ceccb1 that the test is fixed... the rest of the commits are necessary to avoid conflicts

@XadillaX
Copy link
Contributor

XadillaX commented Dec 8, 2017

How about adding this PR? #14891

@MylesBorins
Copy link
Contributor Author

@XadillaX currently it appears that the error is getting swallowed internally by nghttp2, and I'm not sure the PR you are suggesting will fix that.

One thing worth mentioning, 9.x is working with more or less the same change sets against the http2 implementation.

@apapirovski
Copy link
Member

apapirovski commented Dec 8, 2017

@MylesBorins we're missing this PR for http2 #17183 and parts of #17328 (there seems to be two commits from it in here but not the rest)

@MylesBorins
Copy link
Contributor Author

@apapirovski the challenge here is that we have things working in 9.x but not 8.x with a nearly identical changeset.

#17183 has not landed on 9.x and taking only part of #17328 is intentional... none of the commits missing from that PR exist on 9.x

@apapirovski
Copy link
Member

@MylesBorins Good to know. I'll see what else could be responsible.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Dec 8, 2017

I've gone ahead and added the failing test to known_issues and rebased that change into
572dcac which introduced the test. This release now has a few more commits than I would like to see, but it at the very least has me the most confident with what is in the http2 folder at this time. I've kicked off another round of CI and will kick off a build as well, so we should be able to promote this in a couple hours if we can't find a better solution

CI: https://ci.nodejs.org/job/node-test-pull-request/11974/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1138/

jasnell and others added 11 commits December 8, 2017 08:54
Previously, we were using a shared stack allocated buffer to hold
the serialized outbound data but that runs into issues if the
outgoing stream does not write or copy immediately. Instead,
allocate a buffer each time. Slight additional overhead here,
but necessary.

Later on, once we've analyzed this more, we might be able to
switch to a stack allocated ring or slab buffer but that's a
bit more complicated than what we strictly need right now.

PR-URL: #16669
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
`WriteWrap` instances may contain extra storage space.
`self_size()` returns the size of the *entire* struct, member fields as
well as storage space, so it is not an accurate measure for the
storage space available.

Add a method `ExtraSize()` (like the existing `Extra()` for accessing
the storage memory) that yields the wanted value, and use it
in the HTTP2 impl to fix a crash.

PR-URL: #16727
Refs: #16669
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
* eliminate pooling of Nghttp2Stream instances. After testing,
  the pooling is not having any tangible benefit
  and makes things more complicated. Simplify. Simplify.

* refactor inbound headers

* Enforce MAX_HEADERS_LIST setting and limit the number of header
  pairs accepted from the peer. Use the ENHANCE_YOUR_CALM error
  code when receiving either too many headers or too many octets.
  Use a vector to store the headers instead of a queue

PR-URL: #16676
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Do not call destroy each time rstStream is called since the
first call (or receipt of rst frame) will always trigger
destroy. Expand existing test for this behaviour.

PR-URL: #16753
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
The first group of tests makes one more connection and leave the server
alive for longer. Otherwise the test is just catching that the server
has closed the socket, depending on timing.

This does not quite make the test pass yet, however. There are some
quirks with how the http2 code handles errors which actually affect
1.0.2 as well.

PR-URL: #16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Fix a compiler warning that was introduced in commit 4db1bc8
("http2: allocate on every chunk send") by adding an `override` keyword.

PR-URL: #16726
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This update does several significant things:

1. It eliminates the base Nghttp2* classes and folds those
   in to node::http2::Http2Session and node::http2::Http2Stream
2. It makes node::http2::Http2Stream a StreamBase instance and
   sends that out to JS-land to act as the [kHandle] for the
   JavaScript Http2Stream class.
3. It shifts some of the callbacks from C++ off of the JavaScript
   Http2Session class to the Http2Stream class.
4. It refactors the data provider structure for FD and Stream
   based sending to help encapsulate those functions easier
5. It streamlines some of the functions at the C++ layer to
   eliminate now unnecessary redirections
6. It cleans up node_http2.cc for better readability and
   maintainability
7. It refactors some of the debug output
8. Because Http2Stream instances are now StreamBases, they are
   now also trackable using async-hooks
9. The Stream::OnRead algorithm has been simplified with a
   couple bugs fixed.
10. I've eliminated node_http2_core.h and node_http2_core-inl.h
11. Detect invalid handshake a report protocol error to session
12. Refactor out of memory error, improve other errors
13. Add Http2Session.prototype.ping

PR-URL: #17105
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
* fixup js debug messages
* simplify and improve rstStream
* improve and simplify _read
* simplify and improve priority
* simplify on ready a bit
* simplify and improve respond/push
* reduce duplication with _unrefActive
* simplify stream close handling

PR-URL: #17209
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
This is a security release. All Node.js users should consult the
security release summary at
https://nodejs.org/en/blog/vulnerability/december-2017-security-releases/
for details on patched vulnerabilities.

Fixes for the following CVEs are included in this release:

* CVE-2017-15896
* CVE-2017-15897
* CVE-2017-3738 (from the openssl project)

Notable Changes:

* buffer:
  * buffer allocated with an invalid content will now be zero filled
    (Anna Henningsen)
    #17428
* deps:
  * openssl updated to 1.0.2n (Shigeki Ohtsu)
    #17526

PR-URL: #17532
@MylesBorins MylesBorins merged commit 8a44289 into v8.x Dec 8, 2017
MylesBorins added a commit that referenced this pull request Dec 8, 2017
MylesBorins added a commit that referenced this pull request Dec 8, 2017
This is a security release. All Node.js users should consult the
security release summary at
https://nodejs.org/en/blog/vulnerability/december-2017-security-releases/
for details on patched vulnerabilities.

Fixes for the following CVEs are included in this release:

* CVE-2017-15896
* CVE-2017-15897
* CVE-2017-3738 (from the openssl project)

Notable Changes:

* buffer:
  * buffer allocated with an invalid content will now be zero filled
    (Anna Henningsen)
    #17428
* deps:
  * openssl updated to 1.0.2n (Shigeki Ohtsu)
    #17526

PR-URL: #17532
@gibfahn gibfahn deleted the v8.9.3-proposal branch December 8, 2017 20:03
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.