From eb40dbbe964e64064ff58796bb3f9bab7d1f3610 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 10 Feb 2015 13:19:04 +0000 Subject: [PATCH 01/11] Release version 0.1.2 --- services/filestore/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/package.json b/services/filestore/package.json index bb3cae64e9..900f7fa6fc 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -1,6 +1,6 @@ { "name": "filestore-sharelatex", - "version": "0.1.0", + "version": "0.1.2", "description": "An API for CRUD operations on binary files stored in S3", "repository": { "type": "git", From 45689fd2b8f632f4a7353078663df26538333d4f Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 26 Feb 2015 11:32:05 +0000 Subject: [PATCH 02/11] Only call getFileStream callback once --- services/filestore/app/coffee/FSPersistorManager.coffee | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/services/filestore/app/coffee/FSPersistorManager.coffee b/services/filestore/app/coffee/FSPersistorManager.coffee index 4d6d79b1a7..c2f4564987 100644 --- a/services/filestore/app/coffee/FSPersistorManager.coffee +++ b/services/filestore/app/coffee/FSPersistorManager.coffee @@ -27,7 +27,10 @@ module.exports = return callback err @sendFile location, target, fsPath, callback - getFileStream: (location, name, callback = (err, res)->)-> + getFileStream: (location, name, _callback = (err, res)->) -> + callback = (args...) -> + _callback(args...) + _callback = () -> filteredName = filterName name logger.log location:location, name:filteredName, "getting file" sourceStream = fs.createReadStream "#{location}/#{filteredName}" @@ -38,6 +41,8 @@ module.exports = else callback err sourceStream.on 'readable', () -> + # This can be called multiple times, but the callback wrapper + # ensures the callback is only called once callback null, sourceStream From eb89337ada3568a5d559d9714abc4f18858d774a Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 26 Feb 2015 11:32:21 +0000 Subject: [PATCH 03/11] Release version 0.1.3 --- services/filestore/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/package.json b/services/filestore/package.json index 900f7fa6fc..e8f0bffe52 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -1,6 +1,6 @@ { "name": "filestore-sharelatex", - "version": "0.1.2", + "version": "0.1.3", "description": "An API for CRUD operations on binary files stored in S3", "repository": { "type": "git", From 33d8974d02598612638b1556e464e3764afb2dc3 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 12 Mar 2015 17:09:27 +0000 Subject: [PATCH 04/11] kill process group on timeout of convert commands --- .../filestore/app/coffee/FileConverter.coffee | 9 ++--- services/filestore/app/coffee/SafeExec.coffee | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) create mode 100644 services/filestore/app/coffee/SafeExec.coffee diff --git a/services/filestore/app/coffee/FileConverter.coffee b/services/filestore/app/coffee/FileConverter.coffee index 0d6eb0d9f3..d791e4c2d0 100644 --- a/services/filestore/app/coffee/FileConverter.coffee +++ b/services/filestore/app/coffee/FileConverter.coffee @@ -1,7 +1,7 @@ _ = require("underscore") metrics = require("metrics-sharelatex") logger = require("logger-sharelatex") -exec = require('child_process').exec +safe_exec = require('./SafeExec') approvedFormats = ["png"] fourtySeconds = 40 * 1000 @@ -23,8 +23,7 @@ module.exports = return callback err width = "600x" args = "nice convert -define pdf:fit-page=#{width} -flatten -density 300 #{sourcePath} #{destPath}" - console.log args - exec args, childProcessOpts, (err, stdout, stderr)-> + safe_exec args, childProcessOpts, (err, stdout, stderr)-> timer.done() if err? logger.err err:err, stderr:stderr, sourcePath:sourcePath, requestedFormat:requestedFormat, destPath:destPath, "something went wrong converting file" @@ -38,7 +37,7 @@ module.exports = sourcePath = "#{sourcePath}[0]" width = "260x" args = "nice convert -flatten -background white -density 300 -define pdf:fit-page=#{width} #{sourcePath} -resize #{width} #{destPath}" - exec args, childProcessOpts, (err, stdout, stderr)-> + safe_exec args, childProcessOpts, (err, stdout, stderr)-> if err? logger.err err:err, stderr:stderr, sourcePath:sourcePath, "something went wrong converting file to preview" else @@ -51,7 +50,7 @@ module.exports = sourcePath = "#{sourcePath}[0]" width = "548x" args = "nice convert -flatten -background white -density 300 -define pdf:fit-page=#{width} #{sourcePath} -resize #{width} #{destPath}" - exec args, childProcessOpts, (err, stdout, stderr)-> + safe_exec args, childProcessOpts, (err, stdout, stderr)-> if err? logger.err err:err, stderr:stderr, sourcePath:sourcePath, destPath:destPath, "something went wrong converting file to preview" else diff --git a/services/filestore/app/coffee/SafeExec.coffee b/services/filestore/app/coffee/SafeExec.coffee new file mode 100644 index 0000000000..65da163538 --- /dev/null +++ b/services/filestore/app/coffee/SafeExec.coffee @@ -0,0 +1,40 @@ +_ = require("underscore") +logger = require("logger-sharelatex") +child_process = require('child_process') + +# execute a command in the same way as 'exec' but with a timeout that +# kills all child processes +# +# we spawn the command with 'detached:true' to make a new process +# group, then we can kill everything in that process group. + +module.exports = (command, options, callback = (err, stdout, stderr) ->) -> + [cmd, args...] = command.split(' ') + + child = child_process.spawn cmd, args, {detached:true} + stdout = "" + stderr = "" + + cleanup = _.once (err) -> + clearTimeout killTimer if killTimer? + callback err, stdout, stderr + + if options.timeout? + killTimer = setTimeout () -> + try + process.kill -child.pid, options.killSignal || "SIGTERM" + catch error + logger.log process: child.pid, kill_error: error, "error killing process" + , options.timeout + + child.on 'exit', (code, signal) -> + cleanup signal + + child.on 'error', (err) -> + cleanup err + + child.stdout.on 'data', (chunk) -> + stdout += chunk + + child.stderr.on 'data', (chunk) -> + stderr += chunk From 0e5abe1ff8d72d2b9482953d34650cdbe58bf42a Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 13 Mar 2015 09:31:43 +0000 Subject: [PATCH 05/11] update tests to use safe_exec --- .../unit/coffee/FileConverterTests.coffee | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/services/filestore/test/unit/coffee/FileConverterTests.coffee b/services/filestore/test/unit/coffee/FileConverterTests.coffee index a1305684d1..f8a8add22f 100644 --- a/services/filestore/test/unit/coffee/FileConverterTests.coffee +++ b/services/filestore/test/unit/coffee/FileConverterTests.coffee @@ -10,10 +10,9 @@ describe "FileConverter", -> beforeEach -> - @child_process = - exec : sinon.stub() + @safe_exec = sinon.stub() @converter = SandboxedModule.require modulePath, requires: - 'child_process': @child_process + "./SafeExec": @safe_exec "logger-sharelatex": log:-> err:-> @@ -25,43 +24,43 @@ describe "FileConverter", -> describe "convert", -> it "should convert the source to the requested format", (done)-> - @child_process.exec.callsArgWith(2) + @safe_exec.callsArgWith(2) @converter.convert @sourcePath, @format, (err)=> - args = @child_process.exec.args[0][0] + args = @safe_exec.args[0][0] args.indexOf(@sourcePath).should.not.equal -1 args.indexOf(@format).should.not.equal -1 done() it "should return the dest path", (done)-> - @child_process.exec.callsArgWith(2) + @safe_exec.callsArgWith(2) @converter.convert @sourcePath, @format, (err, destPath)=> destPath.should.equal "#{@sourcePath}.#{@format}" done() it "should return the error from convert", (done)-> - @child_process.exec.callsArgWith(2, @error) + @safe_exec.callsArgWith(2, @error) @converter.convert @sourcePath, @format, (err)=> err.should.equal @error done() it "should not accapt an non aproved format", (done)-> - @child_process.exec.callsArgWith(2) + @safe_exec.callsArgWith(2) @converter.convert @sourcePath, "ahhhhh", (err)=> expect(err).to.exist done() describe "thumbnail", -> it "should call converter resize with args", (done)-> - @child_process.exec.callsArgWith(2) + @safe_exec.callsArgWith(2) @converter.thumbnail @sourcePath, (err)=> - args = @child_process.exec.args[0][0] + args = @safe_exec.args[0][0] args.indexOf(@sourcePath).should.not.equal -1 done() describe "preview", -> it "should call converter resize with args", (done)-> - @child_process.exec.callsArgWith(2) + @safe_exec.callsArgWith(2) @converter.preview @sourcePath, (err)=> - args = @child_process.exec.args[0][0] + args = @safe_exec.args[0][0] args.indexOf(@sourcePath).should.not.equal -1 done() From 63ee4d1e7d73d2e6713c9f9a1ba80bb3620420b8 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 13 Mar 2015 10:10:21 +0000 Subject: [PATCH 06/11] use close event instead of exit to capture stdout/stderr correctly --- services/filestore/app/coffee/SafeExec.coffee | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/filestore/app/coffee/SafeExec.coffee b/services/filestore/app/coffee/SafeExec.coffee index 65da163538..0244f14783 100644 --- a/services/filestore/app/coffee/SafeExec.coffee +++ b/services/filestore/app/coffee/SafeExec.coffee @@ -27,8 +27,9 @@ module.exports = (command, options, callback = (err, stdout, stderr) ->) -> logger.log process: child.pid, kill_error: error, "error killing process" , options.timeout - child.on 'exit', (code, signal) -> - cleanup signal + child.on 'close', (code, signal) -> + err = if code then new Error("exit status #{code}") else signal + cleanup err child.on 'error', (err) -> cleanup err From 9aaef729ad774faf273c07009a87fb7741f651f4 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 13 Mar 2015 10:10:51 +0000 Subject: [PATCH 07/11] cleanup and comments --- services/filestore/app/coffee/FileConverter.coffee | 2 +- services/filestore/app/coffee/SafeExec.coffee | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/services/filestore/app/coffee/FileConverter.coffee b/services/filestore/app/coffee/FileConverter.coffee index d791e4c2d0..bf38833017 100644 --- a/services/filestore/app/coffee/FileConverter.coffee +++ b/services/filestore/app/coffee/FileConverter.coffee @@ -1,7 +1,7 @@ _ = require("underscore") metrics = require("metrics-sharelatex") logger = require("logger-sharelatex") -safe_exec = require('./SafeExec') +safe_exec = require("./SafeExec") approvedFormats = ["png"] fourtySeconds = 40 * 1000 diff --git a/services/filestore/app/coffee/SafeExec.coffee b/services/filestore/app/coffee/SafeExec.coffee index 0244f14783..217aab4748 100644 --- a/services/filestore/app/coffee/SafeExec.coffee +++ b/services/filestore/app/coffee/SafeExec.coffee @@ -9,6 +9,7 @@ child_process = require('child_process') # group, then we can kill everything in that process group. module.exports = (command, options, callback = (err, stdout, stderr) ->) -> + # options are {timeout: number-of-milliseconds, killSignal: signal-name} [cmd, args...] = command.split(' ') child = child_process.spawn cmd, args, {detached:true} @@ -22,6 +23,7 @@ module.exports = (command, options, callback = (err, stdout, stderr) ->) -> if options.timeout? killTimer = setTimeout () -> try + # use negative process id to kill process group process.kill -child.pid, options.killSignal || "SIGTERM" catch error logger.log process: child.pid, kill_error: error, "error killing process" From a7b9376919af9b943ebd7c22048bdc13b3959a9e Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 13 Mar 2015 10:15:37 +0000 Subject: [PATCH 08/11] use SIGTERM instead of SIGKILL to allow process to shut down cleanly --- services/filestore/app/coffee/FileConverter.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/app/coffee/FileConverter.coffee b/services/filestore/app/coffee/FileConverter.coffee index bf38833017..c142d3a5e0 100644 --- a/services/filestore/app/coffee/FileConverter.coffee +++ b/services/filestore/app/coffee/FileConverter.coffee @@ -7,7 +7,7 @@ approvedFormats = ["png"] fourtySeconds = 40 * 1000 childProcessOpts = - killSignal: "SIGKILL" + killSignal: "SIGTERM" timeout: fourtySeconds From 143d44e54bfb49a7b034c033e24c21596b70291c Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 13 Mar 2015 10:15:53 +0000 Subject: [PATCH 09/11] add tests for SafeExec module --- .../test/unit/coffee/SafeExec.coffee | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 services/filestore/test/unit/coffee/SafeExec.coffee diff --git a/services/filestore/test/unit/coffee/SafeExec.coffee b/services/filestore/test/unit/coffee/SafeExec.coffee new file mode 100644 index 0000000000..bdfcec3a70 --- /dev/null +++ b/services/filestore/test/unit/coffee/SafeExec.coffee @@ -0,0 +1,42 @@ +assert = require("chai").assert +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = "../../../app/js/SafeExec.js" +SandboxedModule = require('sandboxed-module') + +describe "SafeExec", -> + + beforeEach -> + + @safe_exec = SandboxedModule.require modulePath, requires: + "logger-sharelatex": + log:-> + err:-> + @options = {timeout: 10*1000, killSignal: "SIGTERM" } + + describe "safe_exec", -> + + it "should execute a valid command", (done) -> + @safe_exec "/bin/echo hello", @options, (err, stdout, stderr) => + stdout.should.equal "hello\n" + should.not.exist(err) + done() + + it "should execute a command with non-zero exit status", (done) -> + @safe_exec "/bin/false", @options, (err, stdout, stderr) => + stdout.should.equal "" + stderr.should.equal "" + err.message.should.equal "exit status 1" + done() + + it "should handle an invalid command", (done) -> + @safe_exec "/bin/foobar", @options, (err, stdout, stderr) => + err.code.should.equal "ENOENT" + done() + + it "should handle a command that runs too long", (done) -> + @safe_exec "/bin/sleep 10", {timeout: 500, killSignal: "SIGTERM"}, (err, stdout, stderr) => + err.should.equal "SIGTERM" + done() From a370739722b7129b26f2c58d430c14217b3b6c8f Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 20 Mar 2015 14:20:13 +0000 Subject: [PATCH 10/11] Release version 0.1.4 --- services/filestore/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/package.json b/services/filestore/package.json index e8f0bffe52..c2ba0e4952 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -1,6 +1,6 @@ { "name": "filestore-sharelatex", - "version": "0.1.3", + "version": "0.1.4", "description": "An API for CRUD operations on binary files stored in S3", "repository": { "type": "git", From 801b5653e442c0abf63cf8d51bbd02b103c3aa73 Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 20 Mar 2015 14:27:25 +0000 Subject: [PATCH 11/11] Fix unit test when false is not at /bin/false --- services/filestore/test/unit/coffee/SafeExec.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/test/unit/coffee/SafeExec.coffee b/services/filestore/test/unit/coffee/SafeExec.coffee index bdfcec3a70..b63851aa57 100644 --- a/services/filestore/test/unit/coffee/SafeExec.coffee +++ b/services/filestore/test/unit/coffee/SafeExec.coffee @@ -25,7 +25,7 @@ describe "SafeExec", -> done() it "should execute a command with non-zero exit status", (done) -> - @safe_exec "/bin/false", @options, (err, stdout, stderr) => + @safe_exec "/usr/bin/env false", @options, (err, stdout, stderr) => stdout.should.equal "" stderr.should.equal "" err.message.should.equal "exit status 1"