Capture runtime exceptions in the jailed code

[z4m0]

Hi,

How can the parent process can get notified if the jailed code throws an exception or fails executing its code? Something like:

function task(){
  foo()//foo is not defined
}

or

function task(){
  throw 'failed'
}

It happens to me that the process doesn't end if there isn't a timeout.

Thank you.

[asvd]

I would catch the exception on the jailed side and notify the application then. Would that work?

[z4m0]

Thank you for answering.

I think it would not work because I'm running untrusted code from other users and they can do wrong/buggy code.

Could it be possible to add in _pluginNode.js something like:

process.on('uncaughtException', function(e){
  printError(e)
  process.send({type: 'executeFailure'});
})

and remove the try-catch in line 40? Or there is an easier way to do that without changing the user's code? (I can inject code before and after the user's source code too.)

Thank you.

[asvd]

It seems like you provide user's code directly into the sandbox. What I suggest, is that you create a sandboxed code yourself, create a try-catch block in there, and then send user's code as a string and evaluate it.

[z4m0]

That works well if I'm not executing async code: I've tried:

function task(){
  try{
   eval(userCode)
  }catch(e){
    application.remote.logError(e)
  }
}

where userCode is a string with the code:

application.remote.asyncFunction(function(data){
  foo()//this function is not defined
})

Where asyncFunction gets data from the DB or Internet.

Then I can't caputre the exception.

[asvd]

Ok, now I see. From my perspective the sandbox is created to protect the application. Maybe Jailed should restrict anything asynchronous then..

In your case: what if you have all the asynchronous stuff on your side? Like an exported method on the application site which is provided to the sandbox. Then you can also control the asynchronous callback. Or you create an API on the side of the sandbox. Would that work for you?

You are right about the issue - currently the main application is not notified if an unhandled exception is thrown inside a sandbox, I will need to investigate on this. (Let me know if you have some suggestions)

[z4m0]

In my case I let the user access a list of external services (DB, webservices, ...) and process the data returned. I don't know apriori what will they do. Also I need to notify them if their code fails (they will have a log).

I would suggest adding the line:

process.send({type: 'executeFailure'});

in the file _pluginNode.js

process.on('message', function(m) {
    switch(m.type){
    case 'import':
        importScript(m.url);
        break;
    case 'importJailed':
        importScriptJailed(m.url);
        break;
    case 'execute':
        execute(m.code);
        break;
    case 'message':
        // unhandled exception would break the IPC channel
        try {
            conn._messageHandler(m.data);
        } catch(e) {
           printError(e.stack);
           process.send({type: 'executeFailure'});
       }
        break;
    }
});

Then the callback plugin.whenFailed is executed and the parent process can handle the error. Also it would be cool to be able to know what has failed:

        try {
            conn._messageHandler(m.data);
        } catch(e) {
           printError(e.stack);
           process.send({type: 'executeFailure', error: e});
       }

And then from the callback in the parent process:

plugin.whenFailed(function(error){
   console.error('plugin failed', error)
})

Thank you for the interest.

[asvd]

In your example:

    ...
 case 'message':
    ...

This happens when the application sends a message to the sanbox, not when there's an asynchronous function invoked inside the sandbox. Right?

[z4m0]

When an undefined function is tried to execute I get this in the stderr:

ReferenceError: foo is not defined
    at DYNAMIC PLUGIN:8:5
    at DYNAMIC PLUGIN:115:9
    at JailedSite._processMessage (/xxx/node_modules/jailed/_JailedSite.js:126:21)
    at Object._messageHandler (/xxx/node_modules/jailed/_JailedSite.js:29:32)
    at process.<anonymous> (/xxx/node_modules/jailed/_pluginNode.js:41:18)
    at process.emit (events.js:110:17)
    at handleMessage (child_process.js:324:10)
    at Pipe.channel.onread (child_process.js:352:11)

In node_modules/jailed/_pluginNode.js:41:18 there is a try-catch and I've seen that if the line process.send({type: 'executeFailure'}); is added the failure is reported to the parent process.

I don't know if this will catch all the exceptions and it may be better solutions.

[asvd]

ok, will investigate on this on the weekend. thanks for pointing out the issue :-)

[meaku]

Any progress on this issue @asvd?

[asvd]

@meaku sorry, not yet. currently preparing a new release providing the browser-compatibility, all new features will go on to the next release I guess.

[meaku]

Thanks for the quick reply :)

[taikuukaits]

I just wanted to say that I agree with this issue and that there should also be an option to disable automatically printing errors.

I hacked together a workable solution but it would be great if someone knowledgeable on the project could tackle it.

Here is what I did:

I had to modify Whenable to take a data parameter:

Whenable.prototype.emit = function(data){
        if (!this._emitted) {
            this._emitted = true;

            var handler;
            while(handler = this._handlers.pop()) {
                setTimeout(handler,0, data);
            }
        }
    }

I had to modify executeJailed to include the error.

} catch (e) {
        printError(e.stack);
        fCb(e.stack);
    }

I had to add the callback in execute to send the failure:

var fCb = function(e) {
        process.send({type: 'executeFailure', error: e});
    }

And I added it to the DynamicPlugin connect function (I am only using Dynamic)

this._fCb = function(e){
            me._fail.emit(e);
            me.disconnect();
        }

[gpascualg]

I've extended what @taikuukaits said in https://github.com/asvd/jailed/issues/8#issuecomment-196088692 here: https://github.com/Push-EDX/jailed/commit/8664d322dcc663286e687753c1a8608af303100b

It now allows you to do so:

plugin.whenFailed(function(e) {
        console.error(e.stack);
    })

Basically, it is exported stack, name and message from the javascript error. It is only tested on browser (chrome) but should work elsewhere.

---

@asvd I can open a PR if you want too. I'd do it already, but I want your opinion on the other editions (blanks and so on) it was the editor

[asvd]

@gpascualg thank you for the effort!

In this issue we have the two problems:

1. Error details are not reported to the parent process for synchronous calls inside a jailed plugin;
2. Asynchoronous errors are not handled at all.

Your proposed changeset only covers the first item. Second is a more complex problem which should probably be solved with something like suggested in https://github.com/asvd/jailed/issues/8#issuecomment-111101741

This issue keeps open for the latter problem. The proposed event, which will fire even in case of asynchronous exception on a Jailed site can probably include the error details.

Until it is done, the first problem can be worked around by wrapping the code in a try-catch block (on a plugin site), and reporting the error details in through a callback (basically what you did, but on top of Jailed). For this reason I would not merge it into Jailed.

Minor whitespace fixes are just fine, I would just submit those as a separate commit / PR.

[gpascualg]

@asvd Oups, didn't test that part, I supposed it had been fixed too. Indeed, this would report any syntax error, but probably not a runtime exception on an async task.

I'll see into it, but it will be a few days until I have time to really do any work. I'll keep you updated.

[anderson-]

Hi, I think I've got a hack that solves both problems listed by @asvd: I'm using a older version of the library so I just created a new repository and added the changes in the following commit:

https://github.com/anderson-/jailedWithExceptions/commit/9e19c4b70b3df30952165cb1e0ea080ac5314ef7

I'm not an expert in this library, so I don't really know if this is the right way to go... Hope this is useful for someone :)

[asvd]

@anderson- I can only see that you wrapped the method invocation. I think it will not handle the asynchronous exceptions scheduled by the method.

[gpascualg]

@anderson- I had problems with , error: e, as both NodeJS and Chrome refused to copy the whole object. That is why in my branch I only copy stack, name and message.

The error was something along the lines of "could not clone object".