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

v4.8.3 proposal #12499

Merged
merged 15 commits into from
May 2, 2017
Merged

v4.8.3 proposal #12499

merged 15 commits into from
May 2, 2017

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Apr 19, 2017 •
edited
Loading

Node.js ChangeLog

2017-05-02, Version 4.8.3 'Argon' (Maintenance), @MylesBorins

Notable Changes

  • module:
  • src:
    • fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995
  • tls:
    • fix rare segmentation faults when using TLS

Commits

  • [44260806a6] - Partial revert "tls: keep track of stream that is closed" (Trevor Norris) #11947
  • [ab3fdf531f] - deps: cherry-pick ca0f9573 from V8 upstream (Ali Ijaz Sheikh) #11940
  • [07b92a3c0b] - doc: add supported platforms list for v4.x (Michael Dawson) #12091
  • [ba91c41478] - module: fix loading from global folders on Windows (Richard Lau) #9283
  • [b5b78b12b8] - src: add fcntl.h include to node.cc (Bartosz Sosnowski) #12540
  • [eb393f9ae1] - src: fix base64 decoding (Nikolai Vavilov) #11995
  • [8ed18a1429] - src: ensure that fd 0-2 are valid on windows (Bartosz Sosnowski) #11863
  • [ff1d61c11b] - stream_base,tls_wrap: notify on destruct (Trevor Norris) #11947
  • [6040efd7dc] - test: fix flaky test-tls-wrap-timeout (Rich Trott) #7857
  • [7a1920dc84] - test: add hasCrypto check to tls-socket-close (Daniel Bevenius) #11911
  • [1dc6b38dcf] - test: add test for loading from global folders (Richard Lau) #9283
  • [54f5258582] - tls: fix segfault on destroy after partial read (Ben Noordhuis) #11898
  • [99749dccfe] - tls: keep track of stream that is closed (jBarz) #11776
  • [6d3aaa72a8] - tls: TLSSocket emits 'error' on handshake failure (Mariusz 'koder' Chwalba) #8805

lekoder and others added 12 commits April 18, 2017 20:02
@lekoder @MylesBorins
tls: TLSSocket emits 'error' on handshake failure
6d3aaa7 
Removes branch that would make TLSSocket emit '_tlsError' event if
error occured on handshake and control was not released, as it was
never happening.  Added test for tls.Server to ensure it still emits
'tlsClientError' as expected.

Note that 'tlsClientError' does not exist in the v4.x branch so this
back-port emits 'clientError' instead.  See also pull request #4557.

Fixes: #8803
PR-URL: #8805
Refs: #4557
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@mhdawson @MylesBorins
doc: add supported platforms list for v4.x
07b92a3 
PR-URL: #12091
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: JoãReis <reis@janeasystems.com>
Reviewed-By: Johan Bergströbugs@bergstroem.nu>
@richardlau @MylesBorins
test: add test for loading from global folders
1dc6b38 
Test executes with a copy of the node executable since $PREFIX/lib/node
is relative to the executable location.

PR-URL: #9283
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@richardlau @MylesBorins
module: fix loading from global folders on Windows
ba91c41 
Code was calculating $PREFIX/lib/node relative to process.execPath, but
on Windows process.execPath is $PREFIX\node.exe whereas everywhere else
process.execPath is $PREFIX/bin/node (where $PREFIX is the root of the
installed Node.js).

PR-URL: #9283
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jBarz @MylesBorins
tls: keep track of stream that is closed
99749dc 
TLSWrap object keeps a pointer reference to the underlying
TCPWrap object. This TCPWrap object could be closed and deleted
by the event-loop which leaves us with a dangling pointer.
So the TLSWrap object needs to track the "close" event on the
TCPWrap object.

PR-URL: #11776
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@trevnorris @MylesBorins
stream_base,tls_wrap: notify on destruct
ff1d61c 
The TLSWrap constructor is passed a StreamBase* which it stores as
TLSWrap::stream_, and is used to receive/send data along the pipeline
(e.g. tls -> tcp). Problem is the lifetime of the instance that stream_
points to is independent of the lifetime of the TLSWrap instance. So
it's possible for stream_ to be delete'd while the TLSWrap instance is
still alive, allowing potential access to a then invalid pointer.

Fix by having the StreamBase destructor null out TLSWrap::stream_;
allowing all TLSWrap methods that rely on stream_ to do a check to see
if it's available.

While the test provided is fixed by this commit, it was also previously
fixed by 478fabf. Regardless, leave the test in for better testing.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@trevnorris @MylesBorins
Partial revert "tls: keep track of stream that is closed"
4426080 
This partually reverts commit 4cdb0e8.

A nullptr check in TSLWrap::IsAlive() and the added test were left.

PR-URL: #11947
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@bnoordhuis @MylesBorins
tls: fix segfault on destroy after partial read
54f5258 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ofrobots @MylesBorins
deps: cherry-pick ca0f9573 from V8 upstream
ab3fdf5 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

  Review-Url: https://codereview.chromium.org/2641953002
  Cr-Commit-Position: refs/heads/master@{#42511}

PR-URL: #11940
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bzoz @MylesBorins
src: ensure that fd 0-2 are valid on windows
8ed18a1 
Check that stdin, stdout and stderr are valid file descriptors on
Windows. If not, reopen them with 'nul' file.

Refs: #875
Fixes: #11656
PR-URL: #11863
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danbev @MylesBorins
test: add hasCrypto check to tls-socket-close
7a1920d 
Currently test-tls-socket-close will fail if node
was built using --without-ssl. This commit adds a check to
verify is crypto support exists and if not skip this test.

PR-URL: #11911
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@seishun @MylesBorins
src: fix base64 decoding
eb393f9 
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory. 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 Apr 19, 2017
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 19, 2017 •
edited
Loading

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

RC.0: https://nodejs.org/download/rc/v4.8.3-rc.0/

@MylesBorins
Copy link
Contributor Author

/cc @nodejs/platform-windows looks like something wrong here too

@mscdex mscdex removed build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency. labels Apr 19, 2017
@refack
Copy link
Contributor

refack commented Apr 19, 2017

Same thing #11863

@sam-github
Copy link
Contributor

@MylesBorins There are about 90 missing PRs from v6.x, where missing means the PR isn't labelled as dont-land, see https://gist.github.com/sam-github/eee701f571b54fca3acc46a3cb53e150 (this excludes all majors and minors, meta, etc.).

Is this WIP, or is dont-land not being applied consistently?

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 20, 2017 •
edited
Loading

@sam-github this is a maintenance release... no longer doing active backporting. I only backported breaking bugs

edit: I am no longer applying do-not-land as there is not an active backporting / audit for v4 anymore. Landing things as requested or when I see breaking changes that need it

@sam-github
Copy link
Contributor

@addaleax searching around for the docs you are adding for LTS labels right now, that info in that last comment should be added (that the labels stop being applied when a release goes into maintenance - as opposed to when a release goes out of support).

@addaleax
Copy link
Member

@sam-github done! (#12431)

@refack refack mentioned this pull request Apr 22, 2017
Closed
29 tasks
@bzoz @MylesBorins
src: add fcntl.h include to node.cc
b5b78b1 
#11863 adds _O_RDWR to node.cc
which is defined in fcntl.h. This adds this include directly to
node.cc.

PR-URL: #12540
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins
Copy link
Contributor Author

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

rc.1: https://nodejs.org/download/rc/v4.8.3-rc.1/

@MylesBorins
Copy link
Contributor Author

single failure on arm https://ci.nodejs.org/job/node-test-binary-arm/7558/RUN_SUBSET=3,label=pi1-raspbian-wheezy/console

/cc @nodejs/platform-arm

@MylesBorins
Copy link
Contributor Author

One more CI: https://ci.nodejs.org/job/node-test-pull-request/7727/
One more citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/745/

@MylesBorins MylesBorins reopened this Apr 28, 2017
@MylesBorins
Copy link
Contributor Author

CI is good... citgm is really sad

v4.8.2: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/747/
v4.8.3-rc.1: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/748/

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 29, 2017 •
edited by gibfahn
Loading

Auditing citgm failures and comparing platforms.

fs-extra failures are omitted as they were not related to release and are now fixed in citgm

aix61-ppc64

  • All failures are install based or unchanged since last release EDIT(gib): not supported for Node 4

ubuntu1604-64

  • readable stream is infra failure on our end
  • uglify-js passes on a fresh ubuntu-1604 box

win-vs2015

  • All failures are timeouts or the same as prior release

debian8-64

  • watchify ~ passes on fresh debian 8 install
  • mkdirp ~ passes on fresh box
  • radium ~ phantom failing on fresh box

fedora23 / fedora 22

  • ember-cli ~ failures are timeout related

Based on the above results I'm willing to sign off on this release. @nodejs/build I would really like to dig into why our infra is causing these specific failures

@Trott @MylesBorins
test: fix flaky test-tls-wrap-timeout
6040efd 
Competing timers were causing a race condition and thus the test was
flaky. Instead, we check an object property on process exit.

Fixes: #7650
Backport-PR-URL: #12567
PR-URL: #7857
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins force-pushed the v4.8.3-proposal branch 2 times, most recently from d7126df to 049b4ee Compare May 2, 2017 15:45
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
788f280 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
@MylesBorins MylesBorins merged commit 788f280 into v4.x May 2, 2017
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
Working on v4.8.4
681cebb 
PR-URL:  PR-URL: #12499
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
9802d46 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
9c2a002 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
27a8c43 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@gibfahn gibfahn deleted the v4.8.3-proposal branch May 2, 2017 22:39
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
38a7454 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs#11947
   * (Ben Noordhuis) nodejs#11898
   * (jBarz) nodejs#11776

PR-URL: nodejs#12499
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
meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.