Merge remote-tracking branch 'origin/master'

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
 
 

services/filestore/app/coffee/FileConverter.coffee

@@ -1,13 +1,13 @@
 _ = require("underscore")
 metrics = require("metrics-sharelatex")
 logger = require("logger-sharelatex")
-exec = require('child_process').exec
+safe_exec = require("./SafeExec")
 approvedFormats = ["png"]
 
 fourtySeconds = 40 * 1000
 
 childProcessOpts =
-	killSignal: "SIGKILL"
+	killSignal: "SIGTERM"
 	timeout: fourtySeconds
 
 
@@ -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

services/filestore/app/coffee/SafeExec.coffee

@@ -0,0 +1,43 @@
+_ = 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) ->) ->
+	# options are {timeout:  number-of-milliseconds, killSignal: signal-name}
+	[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
+				# 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"
+		, options.timeout
+
+	child.on 'close', (code, signal) ->
+		err = if code then new Error("exit status #{code}") else signal
+		cleanup err
+
+	child.on 'error', (err) ->
+		cleanup err
+
+	child.stdout.on 'data', (chunk) ->
+		stdout += chunk
+
+	child.stderr.on 'data', (chunk) ->
+		stderr += chunk

services/filestore/package.json

@@ -1,6 +1,6 @@
 {
   "name": "filestore-sharelatex",
-  "version": "0.1.0",
+  "version": "0.1.4",
   "description": "An API for CRUD operations on binary files stored in S3",
   "repository": {
     "type": "git",

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()

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 "/usr/bin/env 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()