(core) communicate with sandbox via standard pipes

Summary:
This switches to using stdin/stdout for RPC calls to the sandbox, rather than specially allocated side channels. Plain text error information remains on stderr.

The motivation for the change is to simplify use of sandboxes, some of which support extra file descriptors and some of which don't.

The new style of communication is made the default, but I'm not committed to this, just that it be easy to switch to if needed. It is possible I'll need to switch the communication method again in the near future.

One reason not to make this default would be windows support, which is likely broken since stdin/stdout are by default in text mode.

Test Plan: existing tests pass

Reviewers: dsagal, alexmojaki

Reviewed By: dsagal, alexmojaki

Differential Revision: https://phab.getgrist.com/D2897

app/server/lib/NSandbox.ts

@@ -8,7 +8,7 @@ import * as log from 'app/server/lib/log';
 import * as sandboxUtil from 'app/server/lib/sandboxUtil';
 import * as shutdown from 'app/server/lib/shutdown';
 import {Throttle} from 'app/server/lib/Throttle';
-import {ChildProcess, spawn, SpawnOptions} from 'child_process';
+import {ChildProcess, spawn} from 'child_process';
 import * as path from 'path';
 import {Stream, Writable} from 'stream';
 import * as _ from 'lodash';
@@ -47,10 +47,23 @@ export class NSandbox implements ISandbox {
    */
   public static spawn(options: ISandboxOptions): ChildProcess {
     const {command, args: pythonArgs, unsilenceLog, env} = options;
-    const spawnOptions: SpawnOptions = {
+    const spawnOptions = {
-      stdio: ['pipe', 'pipe', 'pipe', 'pipe', 'pipe'],
+      stdio: ['pipe', 'pipe', 'pipe'] as 'pipe'[],
-      env,
+      env
     };
+    const selLdrArgs = [];
+    if (!NSandbox._useMinimalPipes(env)) {
+      // add two more pipes
+      spawnOptions.stdio.push('pipe', 'pipe');
+      // We use these options to set up communication with the sandbox:
+      // -r 3:3  to associate a file descriptor 3 on the outside of the sandbox with FD 3 on the
+      //         inside, for reading from the inside. This becomes `this._streamToSandbox`.
+      // -w 4:4  to associate FD 4 on the outside with FD 4 on the inside for writing from the inside.
+      //         This becomes `this._streamFromSandbox`
+      selLdrArgs.push('-r', '3:3', '-w', '4:4');
+    }
+    if (options.selLdrArgs) { selLdrArgs.push(...options.selLdrArgs); }
+
     if (command) {
       return spawn(command, pythonArgs,
                    {cwd: path.join(process.cwd(), 'sandbox'), ...spawnOptions});
@@ -58,12 +71,6 @@ export class NSandbox implements ISandbox {
 
     const noLog = unsilenceLog ? [] :
       (process.env.OS === 'Windows_NT' ? ['-l', 'NUL'] : ['-l', '/dev/null']);
-    // We use these options to set up communication with the sandbox:
-    // -r 3:3  to associate a file descriptor 3 on the outside of the sandbox with FD 3 on the
-    //         inside, for reading from the inside. This becomes `this._streamToSandbox`.
-    // -w 4:4  to associate FD 4 on the outside with FD 4 on the inside for writing from the inside.
-    //         This becomes `this._streamFromSandbox`
-    const selLdrArgs = ['-r', '3:3', '-w', '4:4', ...options.selLdrArgs || []];
     for (const [key, value] of _.toPairs(env)) {
       selLdrArgs.push("-E");
       selLdrArgs.push(`${key}=${value}`);
@@ -80,6 +87,15 @@ export class NSandbox implements ISandbox {
     );
   }
 
+  // Check if environment is configured for minimal pipes.
+  private static _useMinimalPipes(env: NodeJS.ProcessEnv | undefined) {
+    if (!env?.PIPE_MODE) { return false; }
+    if (env.PIPE_MODE !== 'minimal') {
+      throw new Error(`unrecognized pipe mode: ${env.PIPE_MODE}`);
+    }
+    return true;
+  }
+
   public readonly childProc: ChildProcess;
   private _logTimes: boolean;
   private _exportedFunctions: {[name: string]: SandboxMethod};
@@ -111,17 +127,22 @@ export class NSandbox implements ISandbox {
     this.childProc = NSandbox.spawn(options);
 
     this._logMeta = {sandboxPid: this.childProc.pid, ...options.logMeta};
-    log.rawDebug("Sandbox started", this._logMeta);
 
-    this._streamToSandbox = (this.childProc.stdio as Stream[])[3] as Writable;
+    if (NSandbox._useMinimalPipes(options.env)) {
-    this._streamFromSandbox = (this.childProc.stdio as Stream[])[4];
+      log.rawDebug("3-pipe Sandbox started", this._logMeta);
+      this._streamToSandbox = this.childProc.stdin;
+      this._streamFromSandbox = this.childProc.stdout;
+    } else {
+      log.rawDebug("5-pipe Sandbox started", this._logMeta);
+      this._streamToSandbox = (this.childProc.stdio as Stream[])[3] as Writable;
+      this._streamFromSandbox = (this.childProc.stdio as Stream[])[4];
+      this.childProc.stdout.on('data', sandboxUtil.makeLinePrefixer('Sandbox stdout: ', this._logMeta));
+    }
+    this.childProc.stderr.on('data', sandboxUtil.makeLinePrefixer('Sandbox stderr: ', this._logMeta));
 
     this.childProc.on('close', this._onExit.bind(this));
     this.childProc.on('error', this._onError.bind(this));
 
-    this.childProc.stdout.on('data', sandboxUtil.makeLinePrefixer('Sandbox stdout: ', this._logMeta));
-    this.childProc.stderr.on('data', sandboxUtil.makeLinePrefixer('Sandbox stderr: ', this._logMeta));
-
     this._streamFromSandbox.on('data', (data) => this._onSandboxData(data));
     this._streamFromSandbox.on('end', () => this._onSandboxClose());
     this._streamFromSandbox.on('error', (err) => {
@@ -358,6 +379,9 @@ export class NSandboxCreator implements ISandboxCreator {
 
       DOC_URL: (options.docUrl || '').replace(/[^-a-zA-Z0-9_:/?&.]/, ''),
 
+      // use stdin/stdout/stderr only.
+      PIPE_MODE: 'minimal',
+
       // Making time and randomness act deterministically for testing purposes.
       // See test/utils/recordPyCalls.ts
       ...(process.env.LIBFAKETIME_PATH ? {  // path to compiled binary

sandbox/grist/main.py

@@ -13,7 +13,7 @@ import six
 
 from acl_formula import parse_acl_formula
 import actions
-from sandbox import Sandbox
+from sandbox import get_default_sandbox
 import engine
 import migrations
 import schema
@@ -115,8 +115,7 @@ def run(sandbox):
   sandbox.run()
 
 def main():
-  sandbox = Sandbox.connected_to_js_pipes()
+  run(get_default_sandbox())
-  run(sandbox)
 
 if __name__ == "__main__":
   main()

sandbox/grist/sandbox.py

@@ -42,10 +42,26 @@ class Sandbox(object):
 
   @classmethod
   def connected_to_js_pipes(cls):
+    """
+    Send data on two specially-opened side channels.
+    """
     external_input = os.fdopen(3, "rb", 64 * 1024)
     external_output = os.fdopen(4, "wb", 64 * 1024)
     return cls(external_input, external_output)
 
+  @classmethod
+  def use_common_pipes(cls):
+    """
+    Send data via stdin/stdout, rather than specially-opened side channels.
+    Duplicate stdin/stdout, close, and reopen as binary file objects.
+    """
+    os.dup2(0, 3)
+    os.dup2(1, 4)
+    os.close(0)
+    os.close(1)
+    sys.stdout = sys.stderr
+    return Sandbox.connected_to_js_pipes()
+
   def _send_to_js(self, msgCode, msgBody):
     # (Note that marshal version 2 is the default; we specify it explicitly for clarity. The
     # difference with version 0 is that version 2 uses a faster binary format for floats.)
@@ -97,7 +113,10 @@ default_sandbox = None
 def get_default_sandbox():
   global default_sandbox
   if default_sandbox is None:
-    default_sandbox = Sandbox.connected_to_js_pipes()
+    if os.environ.get('PIPE_MODE') == 'minimal':
+      default_sandbox = Sandbox.use_common_pipes()
+    else:
+      default_sandbox = Sandbox.connected_to_js_pipes()
   return default_sandbox
 
 def call_external(name, *args):