From 4222f1ed3261357c6cf2bfe5c61052427dba879e Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Thu, 8 Jul 2021 17:52:00 -0400 Subject: [PATCH] (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 | 56 +++++++++++++++++++++++++++----------- sandbox/grist/main.py | 5 ++-- sandbox/grist/sandbox.py | 21 +++++++++++++- 3 files changed, 62 insertions(+), 20 deletions(-) diff --git a/app/server/lib/NSandbox.ts b/app/server/lib/NSandbox.ts index 3721ee6e..255c17c1 100644 --- a/app/server/lib/NSandbox.ts +++ b/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 = { - stdio: ['pipe', 'pipe', 'pipe', 'pipe', 'pipe'], - env, + const spawnOptions = { + stdio: ['pipe', 'pipe', 'pipe'] as 'pipe'[], + 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; - this._streamFromSandbox = (this.childProc.stdio as Stream[])[4]; + if (NSandbox._useMinimalPipes(options.env)) { + 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 diff --git a/sandbox/grist/main.py b/sandbox/grist/main.py index 6d87a4c4..1c4b16ca 100644 --- a/sandbox/grist/main.py +++ b/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(sandbox) + run(get_default_sandbox()) if __name__ == "__main__": main() diff --git a/sandbox/grist/sandbox.py b/sandbox/grist/sandbox.py index f3b6b5ae..fb4c4fb6 100644 --- a/sandbox/grist/sandbox.py +++ b/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):