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.

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

child_process instance set encoding will trigger exception #18969

Closed
hstarorg opened this issue Feb 24, 2018 · 5 comments
Closed

child_process instance set encoding will trigger exception #18969

hstarorg opened this issue Feb 24, 2018 · 5 comments
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@hstarorg
Copy link

hstarorg commented Feb 24, 2018

  • Version: v9.3.0
  • Platform: Darwin 16.7.0 Darwin Kernel Version 16.7.0: Thu Jan 11 22:59:40 PST 2018; root:xnu-3789.73.8~1/RELEASE_X86_64 x86_64
  • Subsystem:

Execute code at node command line.

const cp = require('child_process');

const child = cp.exec('pwd', {encoding: 'invalid'}, (err, stdout, stderr) => {
  console.log('callback', err)
  console.log('callback', typeof stdout, stdout)
})

child.stdout.setEncoding('utf8')

child.stdout.on('data', function (v) {
  console.log(typeof v, v)
})

The output is :

buffer.js:475
      throw new errors.TypeError(
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "list" argument must be one of type Array, Buffer, or Uint8Array
    at Function.concat (buffer.js:475:13)
    at ChildProcess.exithandler (child_process.js:259:23)
    at ChildProcess.emit (events.js:159:13)
    at maybeClose (internal/child_process.js:943:16)
    at Socket.stream.socket.on (internal/child_process.js:363:11)
    at Socket.emit (events.js:159:13)
    at Pipe._handle.close [as _onclose] (net.js:568:12)

Is this a bug in nodejs?

BTW: The issue finder is @xqin

There didn't trigger the exception if use cp.exec('pwd', {encoding: 'invalid'}) without the callback.

@xqin has a new idea that whether need bind listener on stdout and stderr when user not pass the callback param. The source code at: https://github.com/nodejs/node/blob/master/lib/child_process.js#L322-L358

@xqin
Copy link

xqin commented Feb 24, 2018

$ node -v
v9.3.0
$ uname -a
Darwin 16.7.0 Darwin Kernel Version 16.7.0: Thu Jan 11 22:59:40 PST 2018; root:xnu-3789.73.8~1/RELEASE_X86_64 x86_64

code

$ cat test.js
const cp = require('child_process');

const child = cp.exec('pwd', {encoding: 'invalid'}, (err, stdout, stderr) => {
  console.log('callback', err)
  console.log('callback', typeof stdout, stdout)
})

child.stdout.setEncoding('utf8')

child.stdout.on('data', function (v) {
  console.log(typeof v, v)
})

$ node test.js
string /Users/xqin

buffer.js:475
      throw new errors.TypeError(
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "list" argument must be one of type Array, Buffer, or Uint8Array
    at Function.concat (buffer.js:475:13)
    at ChildProcess.exithandler (child_process.js:259:23)
    at ChildProcess.emit (events.js:159:13)
    at maybeClose (internal/child_process.js:943:16)
    at Socket.stream.socket.on (internal/child_process.js:363:11)
    at Socket.emit (events.js:159:13)
    at Pipe._handle.close [as _onclose] (net.js:568:12)

@targos targos added the child_process Issues and PRs related to the child_process subsystem. label Feb 24, 2018
@killagu
Copy link
Contributor

killagu commented Feb 24, 2018

It's cp bug.
cp have not checked the stdout or stderr encoding changed, cp decide by options.encoding to use stdout or Buffer.concat it.when encoding change to utf-8, the Buffer.concat will failed.

I will raise a PR to fix it.

@hstarorg
Copy link
Author

@killagu Please see this, whether need bind listener on stdout and stderr when user not pass the callback param. The source code at: https://github.com/nodejs/node/blob/master/lib/child_process.js#L322-L358

@killagu
Copy link
Contributor

killagu commented Feb 27, 2018

@hstarorg It should in another issue. One pr resolve one problem.

@hstarorg
Copy link
Author

@killagu OK, Thanks.

killagu added a commit to killagu/node that referenced this issue May 2, 2018
cp.exec decide to use `_stdout`(_stdout is string) or
`Buffer.concat(_stdout)`(_stdout is buffer array) by options.encoding.
but std(out|err) encoding can be changed. If encoding is changed to
not null, `_stdout` will become string, and `Buffer.concat(_stdout)`
will throw TypeError. This patch will fix it, use
options.encoding and `std(out|err)._readableState.encoding`.

Fixes: nodejs#18969
MylesBorins pushed a commit that referenced this issue May 22, 2018
cp.exec decide to use `_stdout`(_stdout is string) or
`Buffer.concat(_stdout)`(_stdout is buffer array) by options.encoding.
but std(out|err) encoding can be changed. If encoding is changed to
not null, `_stdout` will become string, and `Buffer.concat(_stdout)`
will throw TypeError. This patch will fix it, use
options.encoding and `std(out|err)._readableState.encoding`.

PR-URL: #18976
Fixes: #18969
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants