reswitched/pegaswitch

[Feature Request] Promise driven PegaSwitch

JoelLarson opened this issue · 4 comments

As discussed in Discord earlier, it would be really nice to see a cleaner API for evaled files.

sc.getService("set:sys", (setsys) => {
	sc.getService("set:fd", (setfd) => {
		[...]
	});
});

In the SetupNew.js file, callbacks can become very nested. Adding promises / async code would help alleviate this problem.

sc.getService("set:sys")
  .then((setsys) => sc.getService("set:fd"), rejected)
  .then((setfd) => { [...] }, rejected);
async function example() {
  try {
  const setsys = await sc.getService("set:sys");
  const setfd = await sc.getService("set:fd");
  } catch (e) {
    //rejected
  }

  [..] // has access to setsys and setfd
}

The advantage is that no matter how many services you call, they are all processed sequentially and the code stays in a direction that moves your eyes down, has less brackets, and overall leads to less bugs.

Here is an initial implementation using it on just the start.js file. I plan to continue developing it but just got a small start on it for an example:
https://github.com/JoelLarson/pegaswitch/tree/feature/promise-driven

The callbacks don't exist because it's asynchronous- they exist because they represent resources that need to be closed and we don't have RAII in JavaScript. Getting services and such is an entirely synchronous process. AsyncCaller is pretty much the only asynchronous thing in PegaSwitch and it already uses Promises. The callbacks are a convenient way to make sure that we don't leak any resources. Since they're not asynchronous, I don't think it makes sense to use Promises.

That being said, not only do I agree that they get quite nested, but your cleaned up code for the node.js side of things looks pretty nice.

If you want to reduce nesting levels (totally a fan of this), here are my two ideas (not mutually exclusive):

Option 1: sc.getServices

sc.getServices([array of service names], ([array of service handles]) => {
  ...
});

Option 2: Handle Scopes
This one is a little more interesting. If you've ever worked with v8, it uses "handles" to represent objects controlled by the garbage collector. You need to always access them through the handle not only so they don't get GC'd, but also because the v8 GC can move things around. The interesting part though is how they manage handles. C++ code can create a HandleScope. Any Local<> handles will keep track of the HandleScope on the top of the HandleScope stack, and when that HandleScope gets destructed, all of those handles will get destructed. See https://github.com/v8/v8/wiki/Embedder%27s-Guide#handles-and-garbage-collection.

I suggest we implement something like this in PegaSwitch, again using callbacks, but removing the need to nest them.

utils.handleScope((scope) => {
  var setSys = sc.getService("set:sys");
  var setFd = sc.getService("set:fd");
});

sc.getService(name) would need to create a JavaScript object representing the native handle, because we need to be able to adjust the lifetime if the HandleScope on top of the stack doesn't represent the lifetime we want.
When the Handle object is created, it should automatically add itself to the HandleScope on top of the HandleScope stack as a sane default. The Handle object should have a .global() method, which will make its lifetime global. Maybe also another method to reassign which HandleScope it should belong to. The Handle object needs to keep track of one last thing: which process it belongs to. The current design of PegaSwitch is supposed to make our lives easy when we get control of multiple processes (when we finish the sdb exploit). That's why you'll see that we use sploitMixin and svcMixin. The Handle object should be able to keep track of which core it belongs to (sploitcore, sdbcore, etc.), and each core should include the svcMixin, meaning we can use svcMixin.svcCloseHandle.
This will improve handle management all around, since it will become automatic. Ignoring an IPC message's returned handles won't leak them anymore, because they will be collected at the end of the HandleScope.
Of course, we can't use destructors in JavaScript, which is why utils.handleScope needs to take a callback. It can have a try-finally block which destructs the HandleScope in the finally block, just like how our existing handle management functions work.

Also, I haven't been keeping track of #general, so apologies if any of this has already come up.

I'm inclined to agree with @misson20000, it doesn't really make sense to switch everything to promises. Being able to use async/await would be nice, but not worth losing this functionality.

After chatting with @misson20000 on Discord about solutions, we have decided that his Option 1 made the most sense for the context of this problem.

sc.getServices(['setsys', 'setfd'], (setsys, setfd) => {
  // implementation
});

This behavior makes the most sense without having to modify existing code and it is clear what is happening. The normal usecase for this function is normally not more than one or two arguments so code clarity / verboseness isn't a problem.


I have also proposed a solution (psuedocode this is not ES5) for handling needing more than 4-5+ parameters for code clarity that can be adapted or implemented in the future if needed:

sploitMixin.handleServiceCleanup = function (callback) {
    var self = this,
          handles = [];

    try {
        callback({
            getService: function(name) {
                return self.handles[] = self.sc.getService('name');
            }
        });
    } finally {
        for (var i = 0; i < this.handles.length; i++) {
            self.sc.svcCloseHandle(self.handles[i]);
        }
    }
};

sc.handleServiceCleanup(function (serviceProxy) {
    const setsys = serviceProxy.get('set:sys');
    const setfd = serviceProxy.get('set:fd');

    // implementation should cleanup variables above after final call.
})