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: ensure message sanity at send source #24787

Merged
merged 1 commit into from Mar 18, 2019

Conversation

@gireeshpunathil
Copy link
Member

commented Dec 2, 2018

Error messages coming out of de-serialization at the send target is not consumable. So detect the scenario and fix it at the send source.

Ref: #20314

existing error message (sample code in the linked issue):

SUBPROCESS error: SyntaxError: Unexpected token u in JSON at position 0
    at JSON.parse (<anonymous>)
    at Pipe.channel.onread (internal/child_process.js:492:28)

with this change:

PARENT error: TypeError [ERR_INVALID_ARG_TYPE]: The "message" argument must be of type serializable objects. Received type symbol
    at ChildProcess.target._send (internal/child_process.js:664:13)
    at ChildProcess.target.send (internal/child_process.js:641:19)
    at Object.<anonymous> (/home/gireesh/node/parent.js:6:14)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:776:12)
    at executeUserCode (internal/bootstrap/node.js:343:17)

/cc @vsemozhetbyt @joyeecheung @nodejs/child_process

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
lib/internal/child_process.js Outdated Show resolved Hide resolved
// end point; as any failure in the strinfication there
// will result in error message that is weekly consumable.
// So perform a sanity check on message prior to sending.
const ret = JSON.stringify(message);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Dec 2, 2018

Member

Calling JSON.stringify() twice is wasteful. Cache the result and use it on line 730.

This comment has been minimized.

Copy link
@gireeshpunathil

gireeshpunathil Dec 2, 2018

Author Member

unfortunately, the message undergoes change in all the possible control flows between here and line 730, so cached value is unusable.

Another alternative is to delay this sanity check to as late as line 730 where there is already a stringification, don't know whether it can have side effects or not - thoughts please?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 20, 2019

Member

This will only guard against three input types: undefined, function and symbol. All other values either throw (e.g., any input that includes a BigInt) or the non-serializable part would be omitted or replaced with the string 'null' (with the exception of the object containing an toJSON function which returns undefined).

There are lots of inputs which would also be problematic which would not be caught here. I actually doubt that we really have to guard against anything else than undefined. So let's just use a simple type check instead of using JSON.stringify:

if (typeof message !== 'string' &&
    typeof message !== 'object' &&
    typeof message !== 'number' &&
    typeof message !== 'boolean') {
  throw new ERR_INVALID_ARG_TYPE(
    'message', ['string', 'object', 'number', 'boolean'], message);
}

The reason why I would do the check like this is that in the default case the message is either a string or an object. Other type checks would not be necessary anymore. While doing it the other way around would require to always check for undefined, bigint, symbol and function. The error message is also a bit clearer that way.

This will not guard against a weird toJSON function but that should be fine.

This comment has been minimized.

Copy link
@gireeshpunathil

gireeshpunathil Jan 21, 2019

Author Member

@BridgeAR - I followed your suggestions, PTAL.

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

more than a month with no reviews and approvals; can I have one please!

// end point; as any failure in the strinfication there
// will result in error message that is weekly consumable.
// So perform a sanity check on message prior to sending.
const ret = JSON.stringify(message);

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 20, 2019

Member

This will only guard against three input types: undefined, function and symbol. All other values either throw (e.g., any input that includes a BigInt) or the non-serializable part would be omitted or replaced with the string 'null' (with the exception of the object containing an toJSON function which returns undefined).

There are lots of inputs which would also be problematic which would not be caught here. I actually doubt that we really have to guard against anything else than undefined. So let's just use a simple type check instead of using JSON.stringify:

if (typeof message !== 'string' &&
    typeof message !== 'object' &&
    typeof message !== 'number' &&
    typeof message !== 'boolean') {
  throw new ERR_INVALID_ARG_TYPE(
    'message', ['string', 'object', 'number', 'boolean'], message);
}

The reason why I would do the check like this is that in the default case the message is either a string or an object. Other type checks would not be necessary anymore. While doing it the other way around would require to always check for undefined, bigint, symbol and function. The error message is also a bit clearer that way.

This will not guard against a weird toJSON function but that should be fine.

// will result in error message that is weakly consumable.
// So perform a sanity check on message prior to sending.
const ret = JSON.stringify(message);
if (ret == null) {

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 20, 2019

Member

ret can never be null. If null is passed through as message JSON.stringify() would return the string 'null'.

JSON.stringify() either returns undefined or a string or it throws.

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

ping @BridgeAR

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

can I get a review here pls?

@lpinca
lpinca approved these changes Mar 11, 2019
@lpinca

This comment has been minimized.

@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:sub_process branch from 65ac601 to 8b981fa Mar 18, 2019

child_process: ensure message sanity at source
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: #20314

PR-URL: #24787
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:sub_process branch from 8b981fa to daa97df Mar 18, 2019

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

landed as daa97df

@gireeshpunathil gireeshpunathil merged commit daa97df into nodejs:master Mar 18, 2019

1 of 2 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details
Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 2019
child_process: ensure message sanity at source
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: nodejs#20314

PR-URL: nodejs#24787
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit to targos/node that referenced this pull request Mar 27, 2019
child_process: ensure message sanity at source
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: nodejs#20314

PR-URL: nodejs#24787
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit to targos/node that referenced this pull request Mar 27, 2019
child_process: ensure message sanity at source
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: nodejs#20314

PR-URL: nodejs#24787
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit to targos/node that referenced this pull request Mar 27, 2019
child_process: ensure message sanity at source
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: nodejs#20314

PR-URL: nodejs#24787
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit that referenced this pull request Mar 27, 2019
child_process: ensure message sanity at source
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: #20314

PR-URL: #24787
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos referenced this pull request Mar 27, 2019
BethGriggs added a commit that referenced this pull request Apr 17, 2019
child_process: ensure message sanity at source
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: #20314

PR-URL: #24787
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BethGriggs BethGriggs referenced this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.