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

spawnSync's SyncProcessRunner::CopyJsStringArray segfaults with bad getter #9821

Closed
deian opened this issue Nov 28, 2016 · 9 comments
Closed
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@deian
Copy link
Member

deian commented Nov 28, 2016

  • Version:
  • Platform:
  • Subsystem:

Similar to #9820, the underlying binding code that is used by spawnSync can
segfault when called with objects/array that have "evil" getters/setters. The
following code shows an example of this:

const spawn_sync = process.binding('spawn_sync');

// compute envPairs as done by child_process
let envPairs = [];
for (var key in process.env) {
  envPairs.push(key + '=' + process.env[key]);
}

// mess with args
const args = [ '-a' ];

Object.defineProperty(args, 1, {
  get: () => { 
    return 3; // causes StringBytes::Write in spawn_sync.cc:986 to segfault since it's not a string
  },
  set: () => {
    // override so Set after Clone will do nothing because of this
  },
  enumerable: true
});


const options = {
  file: 'ls',
  args: args,
  envPairs: envPairs,
  stdio: [
    { type: 'pipe', readable: true, writable: false },
    { type: 'pipe', readable: false, writable: true },
    { type: 'pipe', readable: false, writable: true } 
  ]

};
spawn_sync.spawn(options);

May be worth again ensuring that all arguments are strings before calling into
the binding code.

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Nov 28, 2016
@mscdex
Copy link
Contributor

mscdex commented Nov 28, 2016

Does this still apply when using require('child_process').spawnSync instead of process.binding()?

@deian
Copy link
Member Author

deian commented Nov 28, 2016

Thanks for filtering through these @mscdex. I am not completely sure. It may be; our process has been to find the low-level bugs and see if we can trigger them from the top-level lib vs. process.binding. This has been sitting in my pile for a bit, will take another look.

@mscdex
Copy link
Contributor

mscdex commented Nov 28, 2016

The reason I ask is because process.binding() is generally not meant for public consumption, so direct use of it is more "use at your own risk."

@Fishrock123
Copy link
Member

I feel like this is probably not binding('spawn_sync')'s job, please try the regular node api. :)

@deian
Copy link
Member Author

deian commented Nov 28, 2016

here you go:

const spawn = require('child_process').spawnSync;
const args = [ '-a' ];

let doit = false;
Object.defineProperty(args, 1, {
  get: () => { 
    if (doit) {
      return 3
    }
    return '3';
  },
  set: () => {
    doit = true;
  },
  enumerable: true
});
args.slice = () => {
  return args;
};

spawn('ls', args);

@deian
Copy link
Member Author

deian commented Nov 28, 2016

@mscdex I completely agree. I guess I'm arguing for a move towards slightly more defensive coding in the binding layer. (But I obviously understand that there are many other efforts and not enough time. I do appreciate the work you all are doing!)

@sam-github
Copy link
Contributor

I think there has been a general trend towards catching bad inputs early, in the js layer. Arguably, the c++ layer should still never fault, but its hard to maintain code paths that aren't reachable. I think the availability of the bindings is itself something node is trying to get rid of (making it only available, like the internal js modules, with a command line flag). I think doing this is blocked by deps in npm, maybe its time to start adding deprecation notices to use of process.binding().

@Trott
Copy link
Member

Trott commented Jul 15, 2017

Is anyone working on this? Would it make sense to add a help wanted label to try to attract a little more attention to it?

@apapirovski apapirovski added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Aug 16, 2018
@apapirovski
Copy link
Member

This seems to abort on an assertion rather than segfault so we'll close it out.

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.

6 participants