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

zlib: fix assert fail for bad write in object mode #16960

Closed
wants to merge 1 commit into from
Closed

zlib: fix assert fail for bad write in object mode #16960

wants to merge 1 commit into from

Conversation

kevinoid
Copy link
Contributor

add4b0a introduced a regression from Node 8 to Node 9 by removing the ArrayBuffer.isView(chunk) check in Zlib.prototype._transform without properly forcing opts.objectMode to false in the constructor because the change to opts occurs after opts has been passed to the Transform constructor. This commit fixes the issue by moving the call to Transform after the changes to opts.

The regression can be demonstrated by running

node -e 'require("zlib").Gunzip({objectMode: true}).write({})'

On Node 8 this correctly throws a TypeError:

events.js:183
      throw er; // Unhandled 'error' event
      ^

TypeError: invalid input
    at Gunzip._transform (zlib.js:367:15)
    at Gunzip.Transform._read (_stream_transform.js:186:10)
    at Gunzip.Transform._write (_stream_transform.js:174:12)
    at doWrite (_stream_writable.js:387:12)
    at writeOrBuffer (_stream_writable.js:373:5)
    at Gunzip.Writable.write (_stream_writable.js:290:11)
    at [eval]:1:44
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at Object.runInThisContext (vm.js:139:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)

On Node 9 this causes a crash due to an assertion failure:

node[21732]: ../src/node_zlib.cc:179:static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = true]: Assertion `Buffer::HasInstance(args[1])' failed.
 1: node::Abort() [node]
 2: node::Assert(char const* const (*) [4]) [node]
 3: 0x1250916 [node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
 5: 0xb7547c [node]
 6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 7: 0x20c44b8842fd
Aborted
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

@kevinoid
zlib: fix assert fail for bad write in object mode
9b927b0 
add4b0a introduced a regression from Node 8 to Node 9 by removing the
`ArrayBuffer.isView(chunk)` check in `Zlib.prototype._transform` without
properly forcing `opts.objectMode` to `false` in the constructor because
the change to `opts` occurs after `opts` has been passed to the
`Transform` constructor.  This commit fixes the issue by moving the call
to `Transform` after the changes to `opts`.

The regression can be demonstrated by running

    node -e 'require("zlib").Gunzip({objectMode: true}).write({})'

On Node 8 this correctly throws a `TypeError`:

    events.js:183
          throw er; // Unhandled 'error' event
          ^

    TypeError: invalid input
        at Gunzip._transform (zlib.js:367:15)
        at Gunzip.Transform._read (_stream_transform.js:186:10)
        at Gunzip.Transform._write (_stream_transform.js:174:12)
        at doWrite (_stream_writable.js:387:12)
        at writeOrBuffer (_stream_writable.js:373:5)
        at Gunzip.Writable.write (_stream_writable.js:290:11)
        at [eval]:1:44
        at ContextifyScript.Script.runInThisContext (vm.js:50:33)
        at Object.runInThisContext (vm.js:139:38)
        at Object.<anonymous> ([eval]-wrapper:6:22)

On Node 9 this causes an assertion failure:

    node[21732]: ../src/node_zlib.cc:179:static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = true]: Assertion `Buffer::HasInstance(args[1])' failed.
     1: node::Abort() [node]
     2: node::Assert(char const* const (*) [4]) [node]
     3: 0x1250916 [node]
     4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
     5: 0xb7547c [node]
     6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
     7: 0x20c44b8842fd
    Aborted

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Nov 12, 2017
@addaleax
Copy link
Member

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

@addaleax
Copy link
Member

Landed in 662d803, thanks for the PR! 🎉

@addaleax addaleax closed this Nov 18, 2017
addaleax pushed a commit that referenced this pull request Nov 18, 2017
@kevinoid @addaleax
zlib: fix assert fail for bad write in object mode
662d803 
add4b0a introduced a regression from Node 8 to Node 9 by removing the
`ArrayBuffer.isView(chunk)` check in `Zlib.prototype._transform` without
properly forcing `opts.objectMode` to `false` in the constructor because
the change to `opts` occurs after `opts` has been passed to the
`Transform` constructor.  This commit fixes the issue by moving the call
to `Transform` after the changes to `opts`.

The regression can be demonstrated by running

    node -e 'require("zlib").Gunzip({objectMode: true}).write({})'

On Node 8 this correctly throws a `TypeError`:

    events.js:183
          throw er; // Unhandled 'error' event
          ^

    TypeError: invalid input
        at Gunzip._transform (zlib.js:367:15)
        at Gunzip.Transform._read (_stream_transform.js:186:10)
        at Gunzip.Transform._write (_stream_transform.js:174:12)
        at doWrite (_stream_writable.js:387:12)
        at writeOrBuffer (_stream_writable.js:373:5)
        at Gunzip.Writable.write (_stream_writable.js:290:11)
        at [eval]:1:44
        at ContextifyScript.Script.runInThisContext (vm.js:50:33)
        at Object.runInThisContext (vm.js:139:38)
        at Object.<anonymous> ([eval]-wrapper:6:22)

On Node 9 this causes an assertion failure:

    node[21732]: ../src/node_zlib.cc:179:static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = true]: Assertion `Buffer::HasInstance(args[1])' failed.
     1: node::Abort() [node]
     2: node::Assert(char const* const (*) [4]) [node]
     3: 0x1250916 [node]
     4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
     5: 0xb7547c [node]
     6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
     7: 0x20c44b8842fd
    Aborted

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
PR-URL: #16960
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@kevinoid
Copy link
Contributor Author

Great. Thank you for reviewing and landing it!

kevinoid added a commit to kevinoid/inflate-auto that referenced this pull request Nov 19, 2017
@kevinoid
Skip invalid type write test on Node.js v9
50b5491 
Calling .write with an invalid type in objectMode causes an assertion
failure on Node.js v9 due to nodejs/node#16960.  Skip this test on
Node.js v9.  (Note: May be able to selectively re-enable if the fix gets
backported.)

Fixes: #19

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@kevinoid @MylesBorins
zlib: fix assert fail for bad write in object mode
45ca714 
add4b0a introduced a regression from Node 8 to Node 9 by removing the
`ArrayBuffer.isView(chunk)` check in `Zlib.prototype._transform` without
properly forcing `opts.objectMode` to `false` in the constructor because
the change to `opts` occurs after `opts` has been passed to the
`Transform` constructor.  This commit fixes the issue by moving the call
to `Transform` after the changes to `opts`.

The regression can be demonstrated by running

    node -e 'require("zlib").Gunzip({objectMode: true}).write({})'

On Node 8 this correctly throws a `TypeError`:

    events.js:183
          throw er; // Unhandled 'error' event
          ^

    TypeError: invalid input
        at Gunzip._transform (zlib.js:367:15)
        at Gunzip.Transform._read (_stream_transform.js:186:10)
        at Gunzip.Transform._write (_stream_transform.js:174:12)
        at doWrite (_stream_writable.js:387:12)
        at writeOrBuffer (_stream_writable.js:373:5)
        at Gunzip.Writable.write (_stream_writable.js:290:11)
        at [eval]:1:44
        at ContextifyScript.Script.runInThisContext (vm.js:50:33)
        at Object.runInThisContext (vm.js:139:38)
        at Object.<anonymous> ([eval]-wrapper:6:22)

On Node 9 this causes an assertion failure:

    node[21732]: ../src/node_zlib.cc:179:static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = true]: Assertion `Buffer::HasInstance(args[1])' failed.
     1: node::Abort() [node]
     2: node::Assert(char const* const (*) [4]) [node]
     3: 0x1250916 [node]
     4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
     5: 0xb7547c [node]
     6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
     7: 0x20c44b8842fd
    Aborted

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
PR-URL: #16960
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
@kissofire kissofire mentioned this pull request Mar 7, 2021
Open
@meghasfdc meghasfdc mentioned this pull request Mar 8, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kevinoid @addaleax @jasnell @lpinca @cjihrig @nodejs-github-bot