zlib: warn before crash on invalid internals usage by addaleax · Pull Request #16657 · nodejs/node

addaleax · 2017-10-31T23:50:27Z

I realize this is a pretty unusual patch but it might make sense to do this given that a lot of people running into #16649 might not understand what is happening or how to fix it.

Refs: #16649
Refs: #14161

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

zlib

Refs: nodejs#16649 Refs: nodejs#14161

evanlucas

I think this is a good idea and it doesn’t really cost us anything

MylesBorins

A++

We should likely bundle this with the openssl patch.

Should we remove it after npm releases their next release?

refack · 2017-11-01T00:39:48Z

Should we remove it after npm releases their next release?

I think it should stay, since the combination of node@9.x and 5.4.0 < npm < 5.5.1 might exist in the wild for a while...

addaleax · 2017-11-01T00:40:01Z

@MylesBorins I have no idea when we should remove it, but no, there’s no rush – this won’t show up if you’re using a broken version.

Trott · 2017-11-01T03:46:25Z

src/node_zlib.cc

+    if (args.Length() == 5) {
+      fprintf(stderr,
+          "WARNING: You are likely using a version of node-tar or npm that "
+          "uses the internals of Node.js in an invalid way.\nPlease use "


Micro-nit (totally not blocking): s/uses the internals of Node.js in an invalid way/is incompatible with this version of Node.js/

I think I prefer the current wording because it gives more of a hint on why the error is occurring. That the versions are incompatible kind of follows from the rest of the message, right?

I guess I'm trying to avoid "uses the internals of Node.js in an invalid way" as it seems likely to be seen as throwing a bit of shade at npm. But I may be overthinking it.

Beyond that, my thinking was that the user needs to know the versions are incompatible, and why (or whose responsible for the situation) are secondary.

All that said, I'm fine with the current wording.

@Trott yup, updated!

Trott · 2017-11-01T03:47:08Z

src/node_zlib.cc

+          "uses the internals of Node.js in an invalid way.\nPlease use "
+          "either the version of npm that is bundled with Node.js, or "
+          "a version of npm (> 5.5.1 or < 5.4.0) or node-tar (> 4.0.1) "
+          "that is compatible with Node 9 and above.\n");


Nit: s/Node 9/Node.js 9/

Trott

My nits are not objections, just comments. Feel free ot ignore if you disagree with them or whatever.

Trott · 2017-11-01T20:28:33Z

Is this intended to be more-or-less permanent or something that gets removed in the foreseeable future? No objections either way. Just trying to be reasonably informed.

addaleax · 2017-11-01T22:50:58Z

@Trott I would expect this to be removed at some point, but that can probably be just about whenever.

bnoordhuis

LGTM but for my understanding, this is because tar/minizlib calls process.binding('zlib')? It's graceful-fs all over!

I looked at minizlib/index.js and I can't for the life of me figure out why it does that. It should be able to get along just fine through the public API.

addaleax · 2017-11-03T00:01:36Z

LGTM but for my understanding, this is because tar/minizlib calls process.binding('zlib')? It's graceful-fs all over!

Yup. The reason is that they’re trying to avoid the overhead that comes with the streams API, which is reasonable but really something we should be providing from core.

addaleax · 2017-11-03T00:02:45Z

CI: https://ci.nodejs.org/job/node-test-commit/13692/

This should be ready.

addaleax · 2017-11-06T17:55:19Z

Landed in 0300565

PR-URL: #16657 Refs: #16649 Refs: #14161 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2017-11-06T20:37:25Z

I've landed this on 9.x-staging to ensure it comes in the next release.

MylesBorins · 2017-11-14T16:48:03Z

@addaleax I've added dont-land-on labels for v6.x and v8.x as they are unaffected by the zlib refactor. please lmk if I am mistaken

zlib: warn before crash on invalid internals usage

0f3e3e5

Refs: nodejs#16649 Refs: nodejs#14161

addaleax added npm Issues and PRs related to the npm client dependency or the npm registry. zlib Issues and PRs related to the zlib subsystem. labels Oct 31, 2017

addaleax requested a review from a team October 31, 2017 23:50

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. zlib Issues and PRs related to the zlib subsystem. labels Oct 31, 2017

evanlucas approved these changes Nov 1, 2017

View reviewed changes

refack approved these changes Nov 1, 2017

View reviewed changes

MylesBorins approved these changes Nov 1, 2017

View reviewed changes

Trott reviewed Nov 1, 2017

View reviewed changes

tniessen approved these changes Nov 1, 2017

View reviewed changes

watilde approved these changes Nov 1, 2017

View reviewed changes

joyeecheung approved these changes Nov 1, 2017

View reviewed changes

[squash] nit

87885e7

[squash] nit

7903320

bnoordhuis approved these changes Nov 2, 2017

View reviewed changes

addaleax closed this Nov 6, 2017

addaleax deleted the zlib-warn-crash branch November 6, 2017 17:55

cjihrig mentioned this pull request Nov 6, 2017

v9.1.0 proposal #16851

Merged

amarchino mentioned this pull request Nov 13, 2017

Error message possible typo in node_zlib #16987

Closed

MylesBorins added dont-land-on-v6.x labels Nov 14, 2017

Uh oh!

Conversation

addaleax commented Oct 31, 2017

Checklist

Affected core subsystem(s)

Uh oh!

evanlucas left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 1, 2017

Uh oh!

addaleax commented Nov 1, 2017

Uh oh!

Trott Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 1, 2017

Uh oh!

addaleax commented Nov 1, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 3, 2017

Uh oh!

addaleax commented Nov 3, 2017

Uh oh!

addaleax commented Nov 6, 2017

Uh oh!

MylesBorins commented Nov 6, 2017

Uh oh!

MylesBorins commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Trott Nov 1, 2017 •

edited

Loading