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

[TypeError: Invalid non-string/buffer chunk] occure when http request will be made after NodeVM run #22

Closed
pvomhoff opened this issue Mar 11, 2016 · 11 comments

Comments

@pvomhoff
Copy link

Hi,

when I do a HTTP Request after a run of the NodeVM the following Error appears.

TypeError: Invalid non-string/buffer chunk
    at chunkInvalid (_stream_readable.js:372:10)
    at readableAddChunk (_stream_readable.js:124:12)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at TCP.onread (net.js:523:20)

Here is my code:

var NodeVM = require('vm2').NodeVM;
var request = require('request');

var options = {
    console: 'inherit',
    require: true,
    requireExternal: true,
    requireNative: [],
    requireRoot : "./"
};

var vm = new NodeVM(options);
var functionInSandbox = vm.run("module.exports = function(){  console.log('Output from function in sandbox'); }", __filename);
vm.call(functionInSandbox);


request('http://google.com', function (err, response, body) {
    if (err) {
        console.error(err.stack);
        return;
    }
    console.log(body);
});

My node version is 4.3.0

When I remove the 'vm.run' and the 'vm.call' the request works.
I already look into the code but i cant find the reason why this happens but i cant find any reason why :(

@Enteee
Copy link

Enteee commented Mar 15, 2016

having similar issues here, minimal code to reproduce:

const vm = new (require('vm2').NodeVM);
vm.run('1+1');
process.stdin.pipe(process.stdout);

but this does not happen when:

const vm = new (require('vm2').NodeVM);
//vm.run('1+1');
process.stdin.pipe(process.stdout);

using VM instead of NodeVM seems to be save as well:

const vm = new (require('vm2').VM);
vm.run('1+1');
process.stdin.pipe(process.stdout);

So I do assume something in the NodeVM.run code seems to be responsible for this.

@Enteee
Copy link

Enteee commented Mar 15, 2016

tribex's fork seems to have a workaround for this. At least compiling his fork does not raise the exception. But as mentioned in his commit this is only a:

Hacky disable of Buffer support to allow this module to function on Node

so how can we fix this properly?..

@patriksimek
Copy link
Owner

Partially fixed in 2.0.

Buffer class is no longer globally available by default in NodeVM. More info in readme.

The problem is that internal buffers are no longer instnaceof Buffer after loading buffer in VM. Still trying to figure it out why.

@Enteee
Copy link

Enteee commented Mar 17, 2016

Thank you. But when I set require: true this bug still triggers. This does not happen when I use the tribex-fork.

Minimal script to reproduce:

const vm = new (require('vm2').NodeVM)({
    require: true,
});
vm.run('1+1');
process.stdin.pipe(process.stdout);

So I'd suggest re-opening this issue.

@patriksimek
Copy link
Owner

By default, all native modules are whitelisted. Use this:

const vm = new (require('vm2').NodeVM)({
    require: true,
    requireNative: []
});
vm.run('1+1');
process.stdin.pipe(process.stdout);

@minecrawler
Copy link

@patriksimek Buffers are no longer instanceof Buffer because you change context. It's the same for everything else as well, like instanceof Function. When working with multiple contexts, do not use instanceof but rather typeof. You can observe this very behavior in the browser as well: iframes! It just works as designed :)

@patriksimek
Copy link
Owner

@minecrawler You're absolutely right, but that's not what we're facing here. The problem is that buffers from the same context are not instanceof Buffer. It only happens when I load buffer inside VM... even if I don't use it at all. Seems that loading a buffer module in VM breaks Buffers in original context somehow.

@ttshaw
Copy link

ttshaw commented Mar 18, 2016

The Buffer itself is used globally:

lib/buffer.js:

const binding = process.binding('buffer');
const bindingObj = {};
binding.setupBufferJS(Buffer.prototype, bindingObj);

src/node_buffer.cc:

void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(args);

  CHECK(args[0]->IsObject());
  Local<Object> proto = args[0].As<Object>();
  env->set_buffer_prototype_object(proto);

evn->set_buffer_prototype_object(proto) is all about, looks likely. Can't setup twice.

@Enteee
Copy link

Enteee commented Mar 26, 2016

The hotfix not to use Buffer renders this module almost useless for me because what I'd like to do does have an internal dependency on buffer. As I don't understand the root of the problem yet, its a bit hard for me to help. So @patriksimek could you please explain what you think is going wrong?

@patriksimek
Copy link
Owner

In short, loading a buffer in VM breaks buffer outside VM. It's an internal problem of Nodejs so I'm not able to fix that atm. I will ask Node devs if there are any plans about fixing this.

@patriksimek
Copy link
Owner

Fixed in 3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants