Merge v0.10 to v0.12 #8886

trevnorris · 2014-12-16T23:28:31Z

There were a lot of changes to crypto. @indutny has one of those fixes.

Also an issue with additional tests to child process in v0.10 that are failing in v0.12.

Currently, a TypeError is incorrectly thrown if the second argument is an object. This commit allows the args argument to be properly omitted. Fixes: #6068 Reviewed-by: Trevor Norris <trev.norris@gmail.com>

Expands the paragraph in the transform stream implementation docs about the callback that is passed to the _transform method to include details about how two arguments may be passed, error and data. A code example is also included. Reviewed-By: Fedor Indutny <fedor@indutny.com>

Otherwise the warning could be printed on some systems. fix #8419

PR-URL: #8553 Reviewed-By: Fedor Indutny <fedor@indutny.com>

PR-URL: #8551 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Because of constant-timeness change made in openssl-1.0.1j the error is no longer returned from EVP_DecryptFinal_ex. Now it just return 0, and thus the error message does not contain proper error code. Adapt to this change, there is not much that we could do about it.

This change disables SSLv2/SSLv3 use by default, and introduces a command line flag to opt into using SSLv2/SSLv3. SSLv2 and SSLv3 are considered unsafe, and should only be used in situations where compatibility with other components is required and they cannot be upgrade to support newer forms of TLS.

The order of the callbacks is non-deterministic, so don't expect the error messages to come back in the same order every time, instead just verify they are expected messages.

In the case of a pipe'd input, i.e. from the CI the fd will be a PIPE and when listen() is called it will return ENOTSOCK instead of EINVAL. Backport: cd2d3ae

Always set ssl2/ssl3 disabled based on whether they are enabled in Node. In some corner-case scenario, node with OPENSSL_NO_SSL3 defined could be linked to openssl that has SSL3 enabled.

adds a note to the crypto docs passing along the advice that openssl gives about what key derivation function they recommend. PR-URL: #8580 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Force-enable SSLv2/v3 when `secureProtocol` is explicitly set to `SSLv2_method` or `SSLv3_method`. see discussion at #8551

* openssl: Update to 1.0.1j (Addressing multiple CVEs) * uv: Update to v0.10.29 * child_process: properly support optional args (cjihrig) * crypto: Disable autonegotiation for SSLv2/3 by default (Fedor Indutny, Timothy J Fontaine, Alexis Campailla) This is a behavior change, by default we will not allow the negotiation to SSLv2 or SSLv3. If you want this behavior, run Node.js with either `--enable-ssl2` or `--enable-ssl3` respectively. This does not change the behavior for users specifically requesting `SSLv2_method` or `SSLv3_method`. While this behavior is not advised, it is assumed you know what you're doing since you're specifically asking to use these methods.

Specifying honorCipherOrder should not change the SSLv2/SSLv3 defaults for a TLS server. Use secureOptions logic in both lib/tls.js and lib/crypto.js

Reuse the secureProtocol and secureOptions of the server when creating the secure context for incoming clients.

Add a test that goes through the whole matrix of: - command line options (--enable-ssl*) - secureOptions - secureProtocols and makes sure that compatible test setups actually work as expected. The test works by spawning two processes for each test case: one client and one server. The test passes if a SSL/TLS connection from the client to the server is successful and the test case was supposed to pass, or if the connection couldn't be established and the test case was supposed to fail. The test is currently located in the directory 'test/external' because it has external dependencies.

Fixes usage of global object 'Buffer' in lib/* files by ensuring that each file does an explicit require('buffer').Buffer. Previously, when running a repl, due to usage of global 'Buffer', any redefinition of Buffer would cause a crash eg var Buffer = {}. Fixes: #8588 PR-URL: #8603 Reviewed-by: Trevor Norris <trev.norris@gmail.com>

Reverted caeb677 for being unable to port the change to deps/v8. The change will be ported directly in a later commit. Conflicts: ChangeLog configure doc/api/child_process.markdown doc/api/tls.markdown doc/api/url.markdown lib/assert.js lib/child_process.js lib/crypto.js lib/dgram.js lib/http.js lib/net.js lib/timers.js lib/tls.js src/node.cc src/node.h src/node.js src/node_crypto.cc src/node_version.h test/common.js test/simple/test-child-process-spawn-typeerror.js tools/certdata.txt

misterdjules · 2014-12-20T03:57:41Z

I pushed misterdjules@dc9f78d to my fork. It fixes the tests failing on our CentOS Jenkins agent, and doesn't introduce any regression.

/cc @tjfontaine @trevnorris @cjihrig @chrisdickinson

cjihrig · 2014-12-20T04:00:22Z

@misterdjules the commit LGTM. I wouldn't mind a more in depth explanation of why this is necessary (I get that there are env variables that are being lost). Was this not a problem in the past, or was it just not noticed?

misterdjules · 2014-12-20T07:58:46Z

@cjihrig CentOS 5.7 ships with a recent g++, but a less recent C++ standard library. It turns out that node binaries built from the v0.12 branch require a newer C++ standard library than the one installed on CentOS 5.7.

Running a binary that requires a newer C++ standard library than the one installed on the system can be done by using the LD_LIBRARY_PATH environment variable, which is what we do to run builds and tests on our CentOS Jenkins node.

If tests that spawn child processes reset their environment variables (which they do if they pass an empty object as their environment), the child process cannot find the newer C++ standard library, and thus the linking fails at runtime.

In my opinion it is semantically correct to append to the environment instead of overriding it during tests. We really mean that we want these additional variables to be present, not that we want the child processes' environment to be reset.

It affects CI platforms, but also any development setup where environment variables are required to make node run properly.

Now why do node binaries built from the v0.12 branch need a newer C++ standard library? Here's the C++ standard library against which a node binary built from the v0.10 branch is linked:

$ ./node --version
v0.10.35-pre
$ ldd ./out/Release/node 
    linux-vdso.so.1 =>  (0x00007fff36167000)
    libdl.so.2 => /lib64/libdl.so.2 (0x00007fb08dbce000)
    librt.so.1 => /lib64/librt.so.1 (0x00007fb08d9c5000)
    libstdc++.so.6 => /usr/lib64/libstdc++.so.6 (0x00007fb08d6c4000)
    libm.so.6 => /lib64/libm.so.6 (0x00007fb08d441000)
    libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fb08d233000)
    libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fb08d016000)
    libc.so.6 => /lib64/libc.so.6 (0x00007fb08ccbd000)
    /lib64/ld-linux-x86-64.so.2 (0x00007fb08dddd000)
$ ls -l /usr/lib64/libstdc++.so.6
lrwxrwxrwx 1 root root 18 Jul 26  2013 /usr/lib64/libstdc++.so.6 -> libstdc++.so.6.0.8
$ strings /usr/lib64/libstdc++.so.6  | grep GLIBCXX
GLIBCXX_3.4
GLIBCXX_3.4.1
GLIBCXX_3.4.2
GLIBCXX_3.4.3
GLIBCXX_3.4.4
GLIBCXX_3.4.5
GLIBCXX_3.4.6
GLIBCXX_3.4.7
GLIBCXX_3.4.8
GLIBCXX_FORCE_NEW
$

That shows that the system's standard C++ library is at version 8, and it supports binaries using up to the version GLIBCXX_3.4.8 of the C++ ABI. The node binary in question runs fine.

Now, if we build a node binary from the v0.12 branch and we try to run it on the same machine, here's what we get:

$ ./node --version
./node: /usr/lib64/libstdc++.so.6: version `GLIBCXX_3.4.15' not found (required by ./node)
$

The runtime linker can't find the a C++ standard library that has the version 3.4.15. As we saw above, the system's standard C++ library is at version 3.4.8, so that's expected.

Now, let's see why this binary requires a newer version of the C++ standard library:

$ nm ./out/Release/node | c++filt | grep GLIBCXX_3.4.15
                 U std::__detail::_List_node_base::_M_hook(std::__detail::_List_node_base*)@@GLIBCXX_3.4.15
$

So it would seem that Node.js' v0.12 branch requires a newer version of the standard C++ library just because it uses std::list, which is confirmed by:

$ git status | grep branch
# On branch v0.10
$ grep -rHn 'include <list>' .
$ grep -rHn 'std::list' .
$ git checkout v0.12
$ git status | grep branch
# On branch v0.12
$ grep -rHn 'include <list>' .
./deps/v8/test/cctest/compiler/test-instruction-selector-arm.cc:5:#include <list>
./deps/v8/src/compiler/graph-reducer.h:8:#include <list>
./deps/v8/src/third_party/vtune/vtune-jit.cc:77:#include <list>
./deps/v8/src/arm64/assembler-arm64.h:8:#include <list>
./deps/v8/src/arm64/decoder-arm64.h:8:#include <list>
$ grep -rHn 'std::list' .
./deps/v8/test/cctest/compiler/test-instruction-selector-arm.cc:26:class DPIs V8_FINAL : public std::list<DPI>, private HandleAndZoneScope {
./deps/v8/test/cctest/compiler/test-instruction-selector-arm.cc:52:class ODPIs V8_FINAL : public std::list<ODPI>, private HandleAndZoneScope {
./deps/v8/test/cctest/compiler/test-instruction-selector-arm.cc:65:class Immediates V8_FINAL : public std::list<int32_t> {
./deps/v8/test/cctest/compiler/test-instruction-selector-arm.cc:89:class Shifts V8_FINAL : public std::list<Shift>, private HandleAndZoneScope {
./deps/v8/src/compiler/graph-reducer.h:68:  typedef std::list<Reducer*, zone_allocator<Reducer*> > Reducers;
./deps/v8/src/third_party/vtune/vtune-jit.cc:104:  std::list<LineNumInfo>* GetLineNumInfo() {
./deps/v8/src/third_party/vtune/vtune-jit.cc:112:  std::list<LineNumInfo> line_num_info_;
./deps/v8/src/third_party/vtune/vtune-jit.cc:211:            std::list<JITCodeLineInfo::LineNumInfo>* vtunelineinfo =
./deps/v8/src/third_party/vtune/vtune-jit.cc:219:            std::list<JITCodeLineInfo::LineNumInfo>::iterator Iter;
./deps/v8/src/arm64/instrument-arm64.h:77:  std::list<Counter*> counters_;
./deps/v8/src/arm64/decoder-arm64.h:118:  std::list<DecoderVisitor*> visitors_;
./deps/v8/src/arm64/instrument-arm64.cc:131:  std::list<Counter*>::iterator it;
./deps/v8/src/arm64/instrument-arm64.cc:158:  std::list<Counter*>::const_iterator it;
./deps/v8/src/arm64/instrument-arm64.cc:170:  std::list<Counter*>::const_iterator it;
./deps/v8/src/arm64/instrument-arm64.cc:200:  std::list<Counter*>::const_iterator it;
./deps/v8/src/arm64/instrument-arm64.cc:218:  std::list<Counter*>::iterator it;
./deps/v8/src/arm64/instrument-arm64.cc:226:  std::list<Counter*>::iterator it;
./deps/v8/src/arm64/decoder-arm64.cc:33:  std::list<DecoderVisitor*>::iterator it;
./deps/v8/src/arm64/decoder-arm64.cc:50:  std::list<DecoderVisitor*>::iterator it;
./deps/v8/src/arm64/decoder-arm64.cc:75:    std::list<DecoderVisitor*>::iterator it;                     \
$

cjihrig · 2014-12-21T19:51:04Z

@misterdjules thanks for the detailed explanation

trevnorris · 2014-12-23T00:36:55Z

@misterdjules Yes, thanks for uncovering the source of that error.

misterdjules · 2014-12-23T04:47:25Z

@trevnorris FYI, @cjihrig fixed test/simple/test-child-process-spawn-typeerror.js failing with PR #8928.

trevnorris · 2014-12-30T18:32:05Z

@misterdjules Should I just squash that onto this commit?

cjihrig · 2015-01-02T16:50:22Z

@trevnorris yes, you can just squash it and close the PR. The only thing left to do on that PR is reword the commit message.

misterdjules · 2015-01-02T21:07:34Z

@trevnorris I'm off until January 4th, until then I won't have time to look at any PR or issue on GitHub. Did you get my latest email @nodejs.org about this merge and about my time off?

indutny · 2015-01-02T23:29:40Z

@trevnorris going to fix that crypto thing.

misterdjules · 2015-01-05T19:52:09Z

@trevnorris What's the status on this merge? What do we need to do to move this forward?

misterdjules · 2015-01-10T00:38:15Z

@trevnorris @tjfontaine Just FYI, I started the merge from scratch as it's not clear if we can re-use the resolve conflicts info from the previous merge. I'll keep you posted on my progress.

trevnorris · 2015-01-13T02:12:34Z

Being done in #9018.

cjihrig and others added 30 commits September 17, 2014 15:13

child_process: properly support optional args

542ac7f

Currently, a TypeError is incorrectly thrown if the second argument is an object. This commit allows the args argument to be properly omitted. Fixes: #6068 Reviewed-by: Trevor Norris <trev.norris@gmail.com>

hdr: always define NODE_WANT_INTERNALS

d87ae24

Otherwise the warning could be printed on some systems. fix #8419

test: check for multi-localhost support

2b7c8a2

doc: build branch versioned docs

641cea0

doc: update design to match nodejs.org

1524d48

tls add secureOptions documentation

6a95e9f

PR-URL: #8553 Reviewed-By: Fedor Indutny <fedor@indutny.com>

configure: disable ssl2/ssl3 by default

0ec78c9

PR-URL: #8551 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

doc: document why SSL2/SSL3 is disabled

d671291

PR-URL: #8551 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

deps: update openssl to 1.0.1j

e0e38c2

build: revert change to disable ssl2 and ssl3

c1f4aac

test: crypto-domains avoid spurious failures

2afa3d8

The order of the callbacks is non-deterministic, so don't expect the error messages to come back in the same order every time, instead just verify they are expected messages.

test: fix test-net-listen-fd0 for pipes

3859fbd

In the case of a pipe'd input, i.e. from the CI the fd will be a PIPE and when listen() is called it will return ENOTSOCK instead of EINVAL. Backport: cd2d3ae

crypto: extra caution in setting ssl options

3139fa2

Always set ssl2/ssl3 disabled based on whether they are enabled in Node. In some corner-case scenario, node with OPENSSL_NO_SSL3 defined could be linked to openssl that has SSL3 enabled.

docs: update api location

b259f24

doc: clarify poodle mitigation

226c986

crypto: move disaling SSLv2/3 into JavaScript

6c8593d

doc: add note about key derivation

7dbc024

adds a note to the crypto docs passing along the advice that openssl gives about what key derivation function they recommend. PR-URL: #8580 Reviewed-By: Fedor Indutny <fedor@indutny.com>

crypto: allow forcing SSLv2/v3 via secureProtocol

1349b68

Force-enable SSLv2/v3 when `secureProtocol` is explicitly set to `SSLv2_method` or `SSLv3_method`. see discussion at #8551

uv: Update to v0.10.29

3544386

tls: honorCipherOrder should not degrade defaults

b9283cf

Specifying honorCipherOrder should not change the SSLv2/SSLv3 defaults for a TLS server. Use secureOptions logic in both lib/tls.js and lib/crypto.js

tls: enforce secureOptions on incoming clients

69080f5

Reuse the secureProtocol and secureOptions of the server when creating the secure context for incoming clients.

Merge branch 'v0.10.33-release' into v0.10

e59eca5

Now working on 0.10.34

ea69dd7

Merge remote-tracking branch 'joyent/v0.10' into v0.10

d435f4b

Nodejs-Jenkins added the v0.12 label Dec 16, 2014

trevnorris force-pushed the merge-review2 branch 4 times, most recently from 09b06be to 71656a9 Compare December 17, 2014 22:24

trevnorris mentioned this pull request Dec 17, 2014

child_process: use correct spawn() #8887

Closed

trevnorris force-pushed the merge-review2 branch from 71656a9 to fdba3aa Compare December 19, 2014 23:29

trevnorris force-pushed the merge-review2 branch from fdba3aa to ae6444d Compare December 20, 2014 00:31

misterdjules added this to the 0.11.15 milestone Dec 20, 2014

trevnorris mentioned this pull request Dec 23, 2014

Timers broken in 0.10.34 #8897

Closed

This was referenced Dec 23, 2014

Backport fix in test/simple/test-child-process-spawn-typeerror.js from v0.12 to v0.10 #8930

Closed

v0.11.15 #8841

Closed

cjihrig mentioned this pull request Jan 8, 2015

test: use executable in spawn() test #8928

Closed

misterdjules mentioned this pull request Jan 9, 2015

dist: windows x64 build of 0.11.14 has nightly tag #8460

Closed

trevnorris closed this Jan 13, 2015

orangemocha removed the in progress label Jan 13, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge v0.10 to v0.12 #8886

Merge v0.10 to v0.12 #8886

trevnorris commented Dec 16, 2014

misterdjules commented Dec 20, 2014

cjihrig commented Dec 20, 2014

misterdjules commented Dec 20, 2014

cjihrig commented Dec 21, 2014

trevnorris commented Dec 23, 2014

misterdjules commented Dec 23, 2014

trevnorris commented Dec 30, 2014

cjihrig commented Jan 2, 2015

misterdjules commented Jan 2, 2015

indutny commented Jan 2, 2015

misterdjules commented Jan 5, 2015

misterdjules commented Jan 10, 2015

trevnorris commented Jan 13, 2015

Merge v0.10 to v0.12 #8886

Merge v0.10 to v0.12 #8886

Conversation

trevnorris commented Dec 16, 2014

misterdjules commented Dec 20, 2014

cjihrig commented Dec 20, 2014

misterdjules commented Dec 20, 2014

cjihrig commented Dec 21, 2014

trevnorris commented Dec 23, 2014

misterdjules commented Dec 23, 2014

trevnorris commented Dec 30, 2014

cjihrig commented Jan 2, 2015

misterdjules commented Jan 2, 2015

indutny commented Jan 2, 2015

misterdjules commented Jan 5, 2015

misterdjules commented Jan 10, 2015

trevnorris commented Jan 13, 2015