Merge pull request #62 from overleaf/spd-decaf-cleanup-1a

Decaf cleanup of SafeExec
This commit is contained in:
Simon Detheridge 2020-01-07 15:41:42 +00:00 committed by GitHub
commit 9b64aadcfc
6 changed files with 245 additions and 171 deletions

View file

@ -9,7 +9,7 @@
"prettier/standard"
],
"parserOptions": {
"ecmaVersion": 6
"ecmaVersion": 2017
},
"plugins": [
"mocha",

View file

@ -1,16 +1,38 @@
/* eslint-disable
no-proto,
no-unused-vars,
*/
// TODO: This file was created by bulk-decaffeinate.
// Fix any style issues and re-enable lint.
let Errors
var NotFoundError = function(message) {
const error = new Error(message)
error.name = 'NotFoundError'
error.__proto__ = NotFoundError.prototype
return error
}
NotFoundError.prototype.__proto__ = Error.prototype
const OError = require('@overleaf/o-error')
module.exports = Errors = { NotFoundError }
// Error class for legacy errors so they inherit OError while staying
// backward-compatible (can be instantiated with string as argument instead
// of object)
class BackwardCompatibleError extends OError {
constructor(messageOrOptions) {
let options
if (typeof messageOrOptions === 'string') {
options = { message: messageOrOptions }
} else if (!messageOrOptions) {
options = {}
} else {
options = messageOrOptions
}
super(options)
}
}
class NotFoundError extends BackwardCompatibleError {}
class ConversionsDisabledError extends BackwardCompatibleError {}
class FailedCommandError extends OError {
constructor(command, code, stdout, stderr) {
super({
message: 'command failed with error exit code',
info: {
command,
code
}
})
this.stdout = stdout
this.stderr = stderr
this.code = code
}
}
module.exports = { NotFoundError, FailedCommandError, ConversionsDisabledError }

View file

@ -1,21 +1,8 @@
/* eslint-disable
camelcase,
handle-callback-err,
no-return-assign,
*/
// TODO: This file was created by bulk-decaffeinate.
// Fix any style issues and re-enable lint.
/*
* decaffeinate suggestions:
* DS101: Remove unnecessary use of Array.from
* DS102: Remove unnecessary code created because of implicit returns
* DS207: Consider shorter variations of null checks
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
*/
const _ = require('underscore')
const logger = require('logger-sharelatex')
const child_process = require('child_process')
const childProcess = require('child_process')
const Settings = require('settings-sharelatex')
const { ConversionsDisabledError, FailedCommandError } = require('./Errors')
// execute a command in the same way as 'exec' but with a timeout that
// kills all child processes
@ -23,36 +10,32 @@ const Settings = require('settings-sharelatex')
// we spawn the command with 'detached:true' to make a new process
// group, then we can kill everything in that process group.
module.exports = function(command, options, callback) {
if (callback == null) {
callback = function(err, stdout, stderr) {}
}
module.exports = safeExec
module.exports.promises = safeExecPromise
// options are {timeout: number-of-milliseconds, killSignal: signal-name}
function safeExec(command, options, callback) {
if (!Settings.enableConversions) {
const error = new Error('Image conversions are disabled')
return callback(error)
return callback(
new ConversionsDisabledError('image conversions are disabled')
)
}
// options are {timeout: number-of-milliseconds, killSignal: signal-name}
const [cmd, ...args] = Array.from(command)
const [cmd, ...args] = command
const child = child_process.spawn(cmd, args, { detached: true })
const child = childProcess.spawn(cmd, args, { detached: true })
let stdout = ''
let stderr = ''
const cleanup = _.once(function(err) {
if (killTimer != null) {
clearTimeout(killTimer)
}
return callback(err, stdout, stderr)
})
let killTimer
if (options.timeout != null) {
var killTimer = setTimeout(function() {
if (options.timeout) {
killTimer = setTimeout(function() {
try {
// use negative process id to kill process group
return process.kill(-child.pid, options.killSignal || 'SIGTERM')
process.kill(-child.pid, options.killSignal || 'SIGTERM')
} catch (error) {
return logger.log(
logger.log(
{ process: child.pid, kill_error: error },
'error killing process'
)
@ -60,14 +43,41 @@ module.exports = function(command, options, callback) {
}, options.timeout)
}
child.on('close', function(code, signal) {
const err = code ? new Error(`exit status ${code}`) : signal
return cleanup(err)
const cleanup = _.once(function(err) {
if (killTimer) {
clearTimeout(killTimer)
}
callback(err, stdout, stderr)
})
child.on('error', err => cleanup(err))
child.on('close', function(code, signal) {
if (code || signal) {
return cleanup(
new FailedCommandError(command, code || signal, stdout, stderr)
)
}
child.stdout.on('data', chunk => (stdout += chunk))
cleanup()
})
return child.stderr.on('data', chunk => (stderr += chunk))
child.on('error', err => {
cleanup(err)
})
child.stdout.on('data', chunk => {
stdout += chunk
})
child.stderr.on('data', chunk => {
stderr += chunk
})
}
function safeExecPromise(command, options) {
return new Promise((resolve, reject) => {
safeExec(command, options, (err, stdout, stderr) => {
if (err) {
reject(err)
}
resolve({ stdout, stderr })
})
})
}

View file

@ -368,6 +368,11 @@
}
}
},
"@overleaf/o-error": {
"version": "2.1.0",
"resolved": "https://registry.npmjs.org/@overleaf/o-error/-/o-error-2.1.0.tgz",
"integrity": "sha512-Zd9sks9LrLw8ErHt/cXeWIkyxWAqNAvNGn7wIjLQJH6TTEEW835PWOhpch+hQwwWsTxWIx/JDj+IpZ3ouw925g=="
},
"@protobufjs/aspromise": {
"version": "1.1.2",
"resolved": "https://registry.npmjs.org/@protobufjs/aspromise/-/aspromise-1.1.2.tgz",
@ -1450,47 +1455,48 @@
"integrity": "sha1-G2HAViGQqN/2rjuyzwIAyhMLhtQ="
},
"eslint": {
"version": "5.16.0",
"resolved": "https://registry.npmjs.org/eslint/-/eslint-5.16.0.tgz",
"integrity": "sha512-S3Rz11i7c8AA5JPv7xAH+dOyq/Cu/VXHiHXBPOU1k/JAM5dXqQPt3qcrhpHSorXmrpu2g0gkIBVXAqCpzfoZIg==",
"version": "6.4.0",
"resolved": "https://registry.npmjs.org/eslint/-/eslint-6.4.0.tgz",
"integrity": "sha512-WTVEzK3lSFoXUovDHEbkJqCVPEPwbhCq4trDktNI6ygs7aO41d4cDT0JFAT5MivzZeVLWlg7vHL+bgrQv/t3vA==",
"dev": true,
"requires": {
"@babel/code-frame": "^7.0.0",
"ajv": "^6.9.1",
"ajv": "^6.10.0",
"chalk": "^2.1.0",
"cross-spawn": "^6.0.5",
"debug": "^4.0.1",
"doctrine": "^3.0.0",
"eslint-scope": "^4.0.3",
"eslint-utils": "^1.3.1",
"eslint-visitor-keys": "^1.0.0",
"espree": "^5.0.1",
"eslint-scope": "^5.0.0",
"eslint-utils": "^1.4.2",
"eslint-visitor-keys": "^1.1.0",
"espree": "^6.1.1",
"esquery": "^1.0.1",
"esutils": "^2.0.2",
"file-entry-cache": "^5.0.1",
"functional-red-black-tree": "^1.0.1",
"glob": "^7.1.2",
"glob-parent": "^5.0.0",
"globals": "^11.7.0",
"ignore": "^4.0.6",
"import-fresh": "^3.0.0",
"imurmurhash": "^0.1.4",
"inquirer": "^6.2.2",
"js-yaml": "^3.13.0",
"inquirer": "^6.4.1",
"is-glob": "^4.0.0",
"js-yaml": "^3.13.1",
"json-stable-stringify-without-jsonify": "^1.0.1",
"levn": "^0.3.0",
"lodash": "^4.17.11",
"lodash": "^4.17.14",
"minimatch": "^3.0.4",
"mkdirp": "^0.5.1",
"natural-compare": "^1.4.0",
"optionator": "^0.8.2",
"path-is-inside": "^1.0.2",
"progress": "^2.0.0",
"regexpp": "^2.0.1",
"semver": "^5.5.1",
"strip-ansi": "^4.0.0",
"strip-json-comments": "^2.0.1",
"semver": "^6.1.2",
"strip-ansi": "^5.2.0",
"strip-json-comments": "^3.0.1",
"table": "^5.2.3",
"text-table": "^0.2.0"
"text-table": "^0.2.0",
"v8-compile-cache": "^2.0.3"
},
"dependencies": {
"debug": {
@ -1502,19 +1508,11 @@
"ms": "^2.1.1"
}
},
"glob": {
"version": "7.1.6",
"resolved": "https://registry.npmjs.org/glob/-/glob-7.1.6.tgz",
"integrity": "sha512-LwaxwyZ72Lk7vZINtNNrywX0ZuLyStrdDtabefZKAY5ZGJhVtgdznluResxNmPitE0SAO+O26sWTHeKSI2wMBA==",
"dev": true,
"requires": {
"fs.realpath": "^1.0.0",
"inflight": "^1.0.4",
"inherits": "2",
"minimatch": "^3.0.4",
"once": "^1.3.0",
"path-is-absolute": "^1.0.0"
}
"lodash": {
"version": "4.17.15",
"resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.15.tgz",
"integrity": "sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A==",
"dev": true
},
"ms": {
"version": "2.1.2",
@ -1523,19 +1521,10 @@
"dev": true
},
"semver": {
"version": "5.7.1",
"resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz",
"integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==",
"version": "6.3.0",
"resolved": "https://registry.npmjs.org/semver/-/semver-6.3.0.tgz",
"integrity": "sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw==",
"dev": true
},
"strip-ansi": {
"version": "4.0.0",
"resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-4.0.0.tgz",
"integrity": "sha1-qEeQIusaw2iocTibY1JixQXuNo8=",
"dev": true,
"requires": {
"ansi-regex": "^3.0.0"
}
}
}
},
@ -1698,9 +1687,9 @@
"dev": true
},
"eslint-scope": {
"version": "4.0.3",
"resolved": "https://registry.npmjs.org/eslint-scope/-/eslint-scope-4.0.3.tgz",
"integrity": "sha512-p7VutNr1O/QrxysMo3E45FjYDTeXBy0iTltPFNSqKAIfjDSXC+4dj+qfyuD8bfAXrW/y6lW3O76VaYNPKfpKrg==",
"version": "5.0.0",
"resolved": "https://registry.npmjs.org/eslint-scope/-/eslint-scope-5.0.0.tgz",
"integrity": "sha512-oYrhJW7S0bxAFDvWqzvMPRm6pcgcnWc4QnofCAqRTRfQC0JcwenzGglTtsLyIuuWFfkqDG9vz67cnttSd53djw==",
"dev": true,
"requires": {
"esrecurse": "^4.1.0",
@ -1723,14 +1712,22 @@
"dev": true
},
"espree": {
"version": "5.0.1",
"resolved": "https://registry.npmjs.org/espree/-/espree-5.0.1.tgz",
"integrity": "sha512-qWAZcWh4XE/RwzLJejfcofscgMc9CamR6Tn1+XRXNzrvUSSbiAjGOI/fggztjIi7y9VLPqnICMIPiGyr8JaZ0A==",
"version": "6.1.2",
"resolved": "https://registry.npmjs.org/espree/-/espree-6.1.2.tgz",
"integrity": "sha512-2iUPuuPP+yW1PZaMSDM9eyVf8D5P0Hi8h83YtZ5bPc/zHYjII5khoixIUTMO794NOY8F/ThF1Bo8ncZILarUTA==",
"dev": true,
"requires": {
"acorn": "^6.0.7",
"acorn-jsx": "^5.0.0",
"eslint-visitor-keys": "^1.0.0"
"acorn": "^7.1.0",
"acorn-jsx": "^5.1.0",
"eslint-visitor-keys": "^1.1.0"
},
"dependencies": {
"acorn": {
"version": "7.1.0",
"resolved": "https://registry.npmjs.org/acorn/-/acorn-7.1.0.tgz",
"integrity": "sha512-kL5CuoXA/dgxlBbVrflsflzQ3PAas7RYZB52NOm/6839iVYJgKMJ3cQJD+t2i5+qFa8h3MDpEOJiS64E8JLnSQ==",
"dev": true
}
}
},
"esprima": {
@ -2189,6 +2186,15 @@
"path-is-absolute": "^1.0.0"
}
},
"glob-parent": {
"version": "5.1.0",
"resolved": "https://registry.npmjs.org/glob-parent/-/glob-parent-5.1.0.tgz",
"integrity": "sha512-qjtRgnIVmOfnKUE3NJAQEdk+lKrxfw8t5ke7SXtfMTHcjsBfOfWXCQfdb30zfDoZQ2IRSIiidmjtbHZPZ++Ihw==",
"dev": true,
"requires": {
"is-glob": "^4.0.1"
}
},
"globals": {
"version": "11.12.0",
"resolved": "https://registry.npmjs.org/globals/-/globals-11.12.0.tgz",
@ -2501,12 +2507,27 @@
"integrity": "sha1-mqIOtq7rv/d/vTPnTKAbM1gdOhY=",
"dev": true
},
"is-extglob": {
"version": "2.1.1",
"resolved": "https://registry.npmjs.org/is-extglob/-/is-extglob-2.1.1.tgz",
"integrity": "sha1-qIwCU1eR8C7TfHahueqXc8gz+MI=",
"dev": true
},
"is-fullwidth-code-point": {
"version": "2.0.0",
"resolved": "https://registry.npmjs.org/is-fullwidth-code-point/-/is-fullwidth-code-point-2.0.0.tgz",
"integrity": "sha1-o7MKXE8ZkYMWeqq5O+764937ZU8=",
"dev": true
},
"is-glob": {
"version": "4.0.1",
"resolved": "https://registry.npmjs.org/is-glob/-/is-glob-4.0.1.tgz",
"integrity": "sha512-5G0tKtBTFImOqDnLB2hG6Bp2qcKEFduo4tZu9MT/H6NQv/ghhy30o55ufafxJ/LdH79LLs2Kfrn85TLKyA7BUg==",
"dev": true,
"requires": {
"is-extglob": "^2.1.1"
}
},
"is-promise": {
"version": "2.1.0",
"resolved": "https://registry.npmjs.org/is-promise/-/is-promise-2.1.0.tgz",
@ -4969,9 +4990,9 @@
"dev": true
},
"strip-json-comments": {
"version": "2.0.1",
"resolved": "https://registry.npmjs.org/strip-json-comments/-/strip-json-comments-2.0.1.tgz",
"integrity": "sha1-PFMZQukIwml8DsNEhYwobHygpgo=",
"version": "3.0.1",
"resolved": "https://registry.npmjs.org/strip-json-comments/-/strip-json-comments-3.0.1.tgz",
"integrity": "sha512-VTyMAUfdm047mwKl+u79WIdrZxtFtn+nBxHeb844XBQ9uMNTuTHdx2hc5RiAJYqwTj3wc/xe5HLSdJSkJ+WfZw==",
"dev": true
},
"supports-color": {
@ -5228,6 +5249,12 @@
"resolved": "https://registry.npmjs.org/utils-merge/-/utils-merge-1.0.1.tgz",
"integrity": "sha1-n5VxD1CiZ5R7LMwSR0HBAoQn5xM="
},
"v8-compile-cache": {
"version": "2.1.0",
"resolved": "https://registry.npmjs.org/v8-compile-cache/-/v8-compile-cache-2.1.0.tgz",
"integrity": "sha512-usZBT3PW+LOjM25wbqIlZwPeJV+3OSz3M1k1Ws8snlW39dZyYL9lOGC5FgPVHfk0jKmjiDV8Z0mIbVQPiwFs7g==",
"dev": true
},
"validate-npm-package-license": {
"version": "3.0.4",
"resolved": "https://registry.npmjs.org/validate-npm-package-license/-/validate-npm-package-license-3.0.4.tgz",

View file

@ -20,6 +20,7 @@
"test:unit:_run": "mocha --recursive --reporter spec $@ test/unit/js"
},
"dependencies": {
"@overleaf/o-error": "^2.1.0",
"async": "~0.2.10",
"aws-sdk": "^2.1.39",
"body-parser": "^1.2.0",
@ -47,7 +48,7 @@
"babel-eslint": "^10.0.3",
"bunyan": "^1.3.5",
"chai": "4.2.0",
"eslint": "^5.16.0",
"eslint": "^6.4.0",
"eslint-config-prettier": "^6.7.0",
"eslint-config-standard": "^14.1.0",
"eslint-plugin-chai-expect": "^2.1.0",

View file

@ -1,95 +1,109 @@
/* eslint-disable
no-return-assign,
no-unused-vars,
*/
// TODO: This file was created by bulk-decaffeinate.
// Fix any style issues and re-enable lint.
/*
* decaffeinate suggestions:
* DS102: Remove unnecessary code created because of implicit returns
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
*/
const { assert } = require('chai')
const sinon = require('sinon')
const chai = require('chai')
const should = chai.should()
const { expect } = chai
const modulePath = '../../../app/js/SafeExec.js'
const modulePath = '../../../app/js/SafeExec'
const SandboxedModule = require('sandboxed-module')
describe('SafeExec', function() {
let settings, options, safeExec
beforeEach(function() {
this.settings = { enableConversions: true }
this.safe_exec = SandboxedModule.require(modulePath, {
settings = { enableConversions: true }
options = { timeout: 10 * 1000, killSignal: 'SIGTERM' }
safeExec = SandboxedModule.require(modulePath, {
requires: {
'logger-sharelatex': {
log() {},
err() {}
},
'settings-sharelatex': this.settings
'settings-sharelatex': settings
}
})
return (this.options = { timeout: 10 * 1000, killSignal: 'SIGTERM' })
})
return describe('safe_exec', function() {
describe('safeExec', function() {
it('should execute a valid command', function(done) {
return this.safe_exec(
['/bin/echo', 'hello'],
this.options,
(err, stdout, stderr) => {
stdout.should.equal('hello\n')
should.not.exist(err)
return done()
}
)
safeExec(['/bin/echo', 'hello'], options, (err, stdout, stderr) => {
stdout.should.equal('hello\n')
stderr.should.equal('')
should.not.exist(err)
done()
})
})
it('should error when conversions are disabled', function(done) {
this.settings.enableConversions = false
return this.safe_exec(
['/bin/echo', 'hello'],
this.options,
(err, stdout, stderr) => {
expect(err).to.exist
return done()
}
)
settings.enableConversions = false
safeExec(['/bin/echo', 'hello'], options, err => {
expect(err).to.exist
done()
})
})
it('should execute a command with non-zero exit status', function(done) {
return this.safe_exec(
['/usr/bin/env', 'false'],
this.options,
(err, stdout, stderr) => {
stdout.should.equal('')
stderr.should.equal('')
err.message.should.equal('exit status 1')
return done()
}
)
safeExec(['/usr/bin/env', 'false'], options, err => {
expect(err).to.exist
expect(err.name).to.equal('FailedCommandError')
expect(err.code).to.equal(1)
expect(err.stdout).to.equal('')
expect(err.stderr).to.equal('')
done()
})
})
it('should handle an invalid command', function(done) {
return this.safe_exec(
['/bin/foobar'],
this.options,
(err, stdout, stderr) => {
err.code.should.equal('ENOENT')
return done()
}
)
safeExec(['/bin/foobar'], options, err => {
err.code.should.equal('ENOENT')
done()
})
})
return it('should handle a command that runs too long', function(done) {
return this.safe_exec(
it('should handle a command that runs too long', function(done) {
safeExec(
['/bin/sleep', '10'],
{ timeout: 500, killSignal: 'SIGTERM' },
(err, stdout, stderr) => {
err.should.equal('SIGTERM')
return done()
err => {
expect(err).to.exist
expect(err.name).to.equal('FailedCommandError')
expect(err.code).to.equal('SIGTERM')
done()
}
)
})
})
describe('as a promise', function() {
beforeEach(function() {
safeExec = safeExec.promises
})
it('should execute a valid command', async function() {
const { stdout, stderr } = await safeExec(['/bin/echo', 'hello'], options)
stdout.should.equal('hello\n')
stderr.should.equal('')
})
it('should throw a ConversionsDisabledError when appropriate', async function() {
settings.enableConversions = false
try {
await safeExec(['/bin/echo', 'hello'], options)
} catch (err) {
expect(err.name).to.equal('ConversionsDisabledError')
return
}
expect('method did not throw an error').not.to.exist
})
it('should throw a FailedCommandError when appropriate', async function() {
try {
await safeExec(['/usr/bin/env', 'false'], options)
} catch (err) {
expect(err.name).to.equal('FailedCommandError')
expect(err.code).to.equal(1)
return
}
expect('method did not throw an error').not.to.exist
})
})
})