zlib: fix raw inflate with custom dictionary #8512

thusoy · 2016-09-13T05:54:45Z

Checklist

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

Affected core subsystem(s)

zlib

Description of change

Moves inflateSetDictionary right after inflateInit2 when mode is
INFLATERAW, since without the wrapper in appears zlib won't return
Z_NEED_DICT as it would otherwise, and will thus attempt inflating
without the dictionary, leading to an error.

Closes #8507.

addaleax · 2016-09-13T12:23:05Z

src/node_zlib.cc

+      case INFLATERAW:
+        ctx->err_ = inflateInit2(&ctx->strm_, ctx->windowBits_);
+        ctx->env()->isolate()
+            ->AdjustAmountOfExternalAllocatedMemory(kInflateContextSize);


Just a suggestion, but if you want to avoid duplicating this, you can just turn the break; statement above into if (ctx->mode_ != INFLATERAW) break; and leave the case INFLATERAW: where it is.

Even better: Could this be moved into SetDictionary?

addaleax · 2016-09-13T12:33:17Z

This looks awesome so far, thanks for putting this together!

thusoy · 2016-09-13T16:16:02Z

Thanks for the feedback! I'll address the points you raised tonight.

thusoy · 2016-09-14T05:43:27Z

@addaleax I moved the dictionary initialization into SetDictionary like you suggested as it was much cleaner, thanks! Since the extra test file I added was nearly identical to the existing dictionary test file I fixed the same issues you raised in the new one in the old one as well. Do you think I should merge these two files since they are so similar, or should they be kept separate?

addaleax · 2016-09-14T07:48:26Z

test/parallel/test-zlib-dictionary-raw.js

+    inflate.end();
+  });
+
+  inflate.on('end', function() {


Could you wrap all of these functions which will be called exactly once in a common.mustCall()? (You can do that in test-zlib-dictionary.js too, if you want)

addaleax · 2016-09-14T07:51:39Z

Since the extra test file I added was nearly identical to the existing dictionary test file I fixed the same issues you raised in the new one in the old one as well. Do you think I should merge these two files since they are so similar, or should they be kept separate?

That’s your call, if you think it’s easy to merge them together, why not?

This looks good to me (LGTM) with common.mustCall() and a green CI.

Here’s a CI run: https://ci.nodejs.org/job/node-test-commit/5048/

thusoy · 2016-09-17T20:32:00Z

@addaleax Merged the files and added common.mustCall to the callbacks.

addaleax

LGTM with CI: https://ci.nodejs.org/job/node-test-commit/5105/

addaleax · 2016-09-17T20:36:22Z

test/parallel/test-zlib-dictionary.js

+  deflate.on('data', common.mustCall(function(chunk) {
+    if (doneReset)
+      inflate.write(chunk);
+  }, 2));


I’d say common.mustCall() for data callbacks is optional, I don’t think we’re currently guaranteeing anything about the exact chunking or want to do that (I think this is okay if the CI is green with it included, so it’s your call).

Fair point, seems like a leaky implementation detail. I removed the mustCalls from non-assertion functions, as if the assertion passes the others are guaranteed to have run, and it will fail if the assertion isn't called.

thusoy · 2016-09-19T05:26:42Z

Pushed new (final?) changeset without mustCall for non-assertion functions. Thanks for the feedback so far @addaleax.

addaleax

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

This looks great, I’ll try to merge this by tomorrow unless somebody else wants to review/objects.

jasnell

LGTM

addaleax · 2016-09-20T18:29:19Z

Eek, merge conflicts… @thusoy can you rebase this? Apparently one of thea general cleanup PRs of the tests here got in the way of your own cleanup… sorry for that!

Moves inflateSetDictionary right after inflateInit2 when mode is INFLATERAW, since without the wrapper in appears zlib won't return Z_NEED_DICT as it would otherwise, and will thus attempt inflating without the dictionary, leading to an error. Fixes: nodejs#8507

Uses const where possible, removes inaccurate comments, prefers strictEqual where possible, ensures functions with assertions are called and enures the inflater has correct encoding set.

thusoy · 2016-09-20T18:46:34Z

@addaleax Yup, done.

addaleax · 2016-09-20T19:03:10Z

That was fast! Still LGTM.

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

I’m going to land this if it’s green.

Moves inflateSetDictionary right after inflateInit2 when mode is INFLATERAW, since without the wrapper in appears zlib won't return Z_NEED_DICT as it would otherwise, and will thus attempt inflating without the dictionary, leading to an error. Fixes: #8507 PR-URL: #8512 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Uses const where possible, removes inaccurate comments, prefers strictEqual where possible, ensures functions with assertions are called and enures the inflater has correct encoding set. PR-URL: #8512 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax · 2016-09-20T21:10:47Z

Landed in c775514 & 2b5acda. Thank you, this was a great contribution!

thusoy · 2016-09-20T21:45:27Z

Great stuff, thanks @addaleax! Happy to contribute!

Moves inflateSetDictionary right after inflateInit2 when mode is INFLATERAW, since without the wrapper in appears zlib won't return Z_NEED_DICT as it would otherwise, and will thus attempt inflating without the dictionary, leading to an error. Fixes: #8507 PR-URL: #8512 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Uses const where possible, removes inaccurate comments, prefers strictEqual where possible, ensures functions with assertions are called and enures the inflater has correct encoding set. PR-URL: #8512 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2016-10-24T16:46:22Z

backport?

addaleax · 2016-10-24T20:50:58Z

@thealphanerd If that is a yes/no question, yes. :)

@thusoy Would you be up for backporting this change to the v4.x release line? There are some conflicts in the test files that seem to need resolving.

The usual process for that would be checking out the v4.x-staging branch here and cherry-picking the commits as they landed on master (only c775514 should suffice), then resolving any conflicts and PRing with the staging branch as the base branch. I or somebody else can do that too, but I’m a bit busy at the moment and you clearly did a good job here in the first place.

thusoy · 2016-10-24T23:12:19Z

@addaleax Thanks for the heads-up on the process. I might be able to that, but will probably be a couple of days before I have time. Consider me on the case for now.

This is just roughly tested, but I just ported the change from nodejs/node#8512, or at least attempted to. Before, when trying to inflateRaw (or inflate({raw:true});) with a custom dictionary, you would get 'invalid distance too far back'. This no longer seems to happen, so I think I fixed it.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 13, 2016

addaleax reviewed Sep 13, 2016
View reviewed changes

addaleax added zlib Issues and PRs related to the zlib subsystem. lts-watch-v4.x labels Sep 13, 2016

thusoy force-pushed the issue-8507 branch from ee70d2f to 6119ff2 Compare September 14, 2016 05:38

addaleax reviewed Sep 14, 2016
View reviewed changes

thusoy force-pushed the issue-8507 branch from 6119ff2 to bd05b50 Compare September 17, 2016 20:28

addaleax approved these changes Sep 17, 2016

View reviewed changes

addaleax mentioned this pull request Sep 17, 2016

test: var variables to const in zlib #8627

Closed

2 tasks

thusoy force-pushed the issue-8507 branch from bd05b50 to f1a0bdd Compare September 19, 2016 05:22

addaleax approved these changes Sep 19, 2016

View reviewed changes

jasnell approved these changes Sep 20, 2016

View reviewed changes

thusoy force-pushed the issue-8507 branch from f1a0bdd to 464ccae Compare September 20, 2016 18:34

thusoy added 2 commits September 20, 2016 11:44

zlib: tighten up dictionary tests

06fd879

Uses const where possible, removes inaccurate comments, prefers strictEqual where possible, ensures functions with assertions are called and enures the inflater has correct encoding set.

thusoy force-pushed the issue-8507 branch from 464ccae to 06fd879 Compare September 20, 2016 18:46

addaleax closed this Sep 20, 2016

thusoy deleted the issue-8507 branch September 20, 2016 21:44

thusoy mentioned this pull request Oct 29, 2016

Backport fix for zlib inflation with custom dicts to 4.x #9358

Closed

3 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 18, 2016

easyaspi314 mentioned this pull request Nov 13, 2017

Allow raw inflation with a custom dictionary. nodeca/pako#120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: fix raw inflate with custom dictionary #8512

zlib: fix raw inflate with custom dictionary #8512

thusoy commented Sep 13, 2016

addaleax Sep 13, 2016

addaleax Sep 13, 2016

addaleax commented Sep 13, 2016

thusoy commented Sep 13, 2016

thusoy commented Sep 14, 2016

addaleax Sep 14, 2016

addaleax commented Sep 14, 2016

thusoy commented Sep 17, 2016

addaleax left a comment

addaleax Sep 17, 2016

thusoy Sep 19, 2016

thusoy commented Sep 19, 2016

addaleax left a comment

jasnell left a comment

addaleax commented Sep 20, 2016

thusoy commented Sep 20, 2016

addaleax commented Sep 20, 2016

addaleax commented Sep 20, 2016

thusoy commented Sep 20, 2016

MylesBorins commented Oct 24, 2016

addaleax commented Oct 24, 2016

thusoy commented Oct 24, 2016

zlib: fix raw inflate with custom dictionary #8512

zlib: fix raw inflate with custom dictionary #8512

Conversation

thusoy commented Sep 13, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax Sep 13, 2016

Choose a reason for hiding this comment

addaleax Sep 13, 2016

Choose a reason for hiding this comment

addaleax commented Sep 13, 2016

thusoy commented Sep 13, 2016

thusoy commented Sep 14, 2016

addaleax Sep 14, 2016

Choose a reason for hiding this comment

addaleax commented Sep 14, 2016

thusoy commented Sep 17, 2016

addaleax left a comment

Choose a reason for hiding this comment

addaleax Sep 17, 2016

Choose a reason for hiding this comment

thusoy Sep 19, 2016

Choose a reason for hiding this comment

thusoy commented Sep 19, 2016

addaleax left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

addaleax commented Sep 20, 2016

thusoy commented Sep 20, 2016

addaleax commented Sep 20, 2016

addaleax commented Sep 20, 2016

thusoy commented Sep 20, 2016

MylesBorins commented Oct 24, 2016

addaleax commented Oct 24, 2016

thusoy commented Oct 24, 2016