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

v4.8.1 proposal #11760

Merged
merged 148 commits into from
Mar 21, 2017
Merged

v4.8.1 proposal #11760

merged 148 commits into from
Mar 21, 2017

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Mar 9, 2017

2017-03-21, Version 4.8.1 'Argon' (LTS), @MylesBorins

This LTS release comes with 147 commits. This includes 55 which are test related,
41 which are doc related, 11 which are build / tool related,
and 1 commits which are updates to dependencies.

Notable Changes

Coming Soon

Commits

cristiancavalli and others added 30 commits March 8, 2017 17:23
Original commit message:
  [debug] load correct stack slot for frame details.

  [email protected]
  BUG=v8:5071

  Review URL: https://codereview.chromium.org/2045863002 .

  Cr-Commit-Position: refs/heads/master@{#36769}

PR-URL: #10873
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
An application using node built as a shared library may legitimately
implement its own signal handling routines. Current behaviour is
to squash all signal handlers on node startup. This change will
stop that behaviour when node is built as a shared library.

PR-URL: #10539
Fixes: #10520
Refs: #615
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
SecureContext::AddRootCerts only parses the root certificates once and
keeps the result in root_cert_store, a global X509_STORE. This change
addresses the following issues:

1. SecureContext::AddCACert would add certificates to whatever
X509_STORE was being used, even if that happened to be root_cert_store.
Thus adding a CA certificate to a SecureContext would also cause it to
be included in unrelated SecureContexts.

2. AddCRL would crash if neither AddRootCerts nor AddCACert had been
called first.

3. Calling AddCACert without calling AddRootCerts first, and with an
input that didn't contain any certificates, would leak an X509_STORE.

4. AddCRL would add the CRL to whatever X509_STORE was being used. Thus,
like AddCACert, unrelated SecureContext objects could be affected.

The following, non-obvious behaviour remains: calling AddRootCerts
doesn't /add/ them, rather it sets the CA certs to be the root set and
overrides any previous CA certificates.

Points 1–3 are probably unimportant because the SecureContext is
typically configured by `createSecureContext` in `lib/_tls_common.js`.
This function either calls AddCACert or AddRootCerts and only calls
AddCRL after setting up CA certificates. Point four could still apply in
the unlikely case that someone configures a CRL without explicitly
configuring the CAs.

PR-URL: #9409
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Fix leaking the BIO in the error path.  Introduced in commit 34febfb
("crypto: fix handling of root_cert_store").

PR-URL: #9604
Refs: #9409
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
This makes sure that we dump a backtrace and use raise(SIGABRT) on
Windows.

PR-URL: #9613
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Use of abort() was added in 34febfb, and changed to ABORT()
in 21826ef, but conditional+ABORT() is better expressesed
using a CHECK_xxx() macro.

See: #9409 (comment)

PR-URL: #10413
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This test is newly added to v4.x stream and is consistently failing.
We have a couple of issues with pseudo-tty tests in AIX, and while
the investigation is going on, need to skip this test to make CI green.

PR-URL: #11602
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.

There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.

PR-URL: #8923
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: not-an-aardvark <[email protected]>
PR-URL: #9649
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
The node -> out/*/node symlink is getting recreated in parallel with
other targets in the makefile which require it (e.g. test-ci) and
this seems to be causing a race condition which is showing up on AIX

Fixes: #9825
PR-URL: #9827
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-by: Michael Dawson <[email protected]>
Currently when running make node_g the following error is displayed:
if [ ! -r node -o ! -L  ]; then ln -fs out/Debug/node node_g; fi
/bin/sh: line 0: [: argument expected

It looks like there was a typo for the NODE_EXE where node became
lowercase instead of uppercase.

Ref: #9827
PR-URL: #10153
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Provide details for fields of rinfo object of UDP message event.

PR-URL: #10050
Reviewed-By: James M Snell <[email protected]>
The COLLABORATOR_GUIDE was still listing v0.10 and v0.12 as LTS when
they are EOL now.

PR-URL: #10720
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
git apply does not preserve the original commit message. These updated
instructions offer a simpler flow for backporting.

PR-URL: #10665
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
The guide for writing tests is missing information on how tests are
named. This adds that information.

There is also some copy-editing done on the first paragraph of the
guide.

PR-URL: #10584
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
PR-URL: #10616
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Add links to the engine classes for the zlib single-call
convenience methods.

PR-URL: #10829
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This commit clarifies variables in the Filesystem docs.
Prior, the documentation for fs.write() had an ambiguous
remark on the parameters of offset and length.

Therefore, this commit makes explicit that the length parameter
in fs.write() is used to denote the number of bytes, which is
a clearer reference for its usage.

PR-URL: #9792
Ref: #7868
Reviewed-By: Sam Roberts <[email protected]>
PR-URL: #10883
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
PR-URL: #10954
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Changed authors listing from `Noah Rose` to  `Noah Rose Ledesma`.

PR-URL: #10945
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
* use common.mustCall() where appropriate
* Buffer.allocUnsafe() -> Buffer.alloc()
* do crypto check before loading any additional modules
* specify 1ms duration for `setTimeout()`

PR-URL: #10225
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
We were transporting the heap statistics as uint32 values to JS land but
those wrap around for values > 4 GB.  Use 64 bits floats instead, those
should last us a while.

Fixes: #10185
PR-URL: #10186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
new year new alias

PR-URL: #10586
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: #10579
Fixes: #9063
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Fixes: nodejs/CTC#41

PR-URL: #10604
Fixes: https://github.com/nodejs/CTC#41
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: #11513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Add documentation for http clientRequest.aborted.

PR-URL: #11544
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Communicate about leaked globals via `AssertionError` rather than
`console.log()`.

PR-URL: #11547
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Because any call to util.inspect() with an object results in
inspectPromise() being called, Debug was being initialized even when
it's not needed. Instead, the initialization is placed after the
isPromise check.

PR-URL: #8452
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. v4.x v8 engine Issues and PRs related to the V8 dependency. labels Mar 9, 2017
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 9, 2017

This commit adds a guard against an out of bounds access of
arguments, and replaces another use of arguments with a named
function parameter.

Refs: #10323
`request.abort()` did not destroy the socket if it was called
before a socket was assigned to the request and the request
did not use an `Agent` or a Unix Domain Socket was used.

Fixes: #10812
PR-URL: #10818
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 21, 2017

Left it too long, one more run:

ci: https://ci.nodejs.org/job/node-test-pull-request/6949/
citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/664/

edit:

CI is good!

edit 2:

Three failures to look into for CITGM


ubuntu 1404 ember-cli

1) eslint should have no errors in tests/unit:
      Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
   
 Mocha Tests Running Time: 78824ms
 warn: Skipping node_modules directory while scanning for yuidoc.json
 You cannot call the addon-import blueprint directly.
 The `ember new` command requires a name to be specified. For more details, use `ember help`.
 The `ember new` command requires a name to be specified. For more details, use `ember help`.
 Directory 'bar' already exists.
 You cannot use the new command inside an ember-cli project.
 The specified command unknownCommand is invalid. For available options, see `ember help`.
 foo
 Error: foo
     at Context.<anonymous> (/tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/tests/unit/cli/cli-test.js:637:48)
     at callFn (/tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runnable.js:345:21)
     at Test.Runnable.run (/tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runnable.js:337:7)
     at Runner.runTest (/tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runner.js:444:10)
     at /tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runner.js:550:12
     at next (/tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runner.js:361:14)
     at /tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runner.js:371:7
     at next (/tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runner.js:295:14)
     at Immediate._onImmediate (/tmp/0ce36871-0615-4017-84be-de01c44688b8/ember-cli/node_modules/mocha/lib/runner.js:339:5)
     at processImmediate [as _immediateCallback] (timers.js:396:17)
 The specified command fake-command is invalid. For available options, see `ember help`.
 The specified command fake-command is invalid. For available options, see `ember help`.
 The specified command fake-command is invalid. For available options, see `ember help`.
 npm ERR! Test failed.  See above for more details.

/cc @stefanpenner

it looks like the failing test was a timeout... but the error messages in the output are concerning


ubuntu 1604 readable-stream

Looks like the tmpdir /home/iojs/tmp is being passed around to be used, but doesn't exist. Is this a problem with the test suite on this platform?

/cc @mcollina


osx ws

this is really weird, will attempt to reproduce locally

@Fishrock123
Copy link
Contributor

Notable changes seems significantly smaller here.

### Notable Changes

* **buffer**: The performance of `.toJSON()` is now up to 2859% faster on average. (Brian White) [#10895](https://github.com/nodejs/node/pull/10895)
* **IPC**: Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) [#10677](https://github.com/nodejs/node/pull/10677)
  - Performance gains may be up to 40% for some workloads.
* **http**:
  - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) [#8923](https://github.com/nodejs/node/pull/8923)
* **node**: Heap statistics now support values larger than 4GB. (Ben Noordhuis) [#10186](https://github.com/nodejs/node/pull/10186)

Notable Changes:

* buffer:
  - The performance of `.toJSON()` is now up to 2859% faster on average
    (Brian White) #10895

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186
@MylesBorins MylesBorins merged commit 9551665 into v4.x Mar 21, 2017
MylesBorins added a commit that referenced this pull request Mar 21, 2017
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable Changes:

* buffer:
  - The performance of `.toJSON()` is now up to 2859% faster on average
    (Brian White) #10895

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

PR-URL: #11760
@gibfahn gibfahn deleted the v4.8.1-proposal branch March 21, 2017 22:21
@gibfahn
Copy link
Member

gibfahn commented Mar 22, 2017

It's not an issue, but for posterity the head commit on v4.x (89a83a5) says Working on v4.8.1 rather than Working on v4.8.2. The node_version.h change is correct though.

cc/ @BethGriggs (who noticed)

@mcollina
Copy link
Member

@MylesBorins

ubuntu 1604 readable-stream

Looks like the tmpdir /home/iojs/tmp is being passed around to be used, but doesn't exist. Is this a > problem with the test suite on this platform?

This is the issue: https://github.com/nodejs/build/blob/master/setup/ubuntu16.04/resources/jenkins.service.j2#L17

but it is not present in the others: https://github.com/nodejs/build/blob/master/setup/fedora22/resources/jenkins.service.

@MylesBorins
Copy link
Contributor Author

good eye. I've gone ahead and force pushed to fix that.

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. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.