FIX: automatically timeout long running image magick commands (#12670)

Previously certain images may lead to convert / identify to run for unreasonable
amounts of time

This adds a maximum amount of time these commands can run prior to forcing
them to stop

app/models/optimized_image.rb

@@ -306,9 +306,10 @@ class OptimizedImage < ActiveRecord::Base
   end
 
   MAX_PNGQUANT_SIZE = 500_000
+  MAX_CONVERT_SECONDS = 20
 
   def self.convert_with(instructions, to, opts = {})
-    Discourse::Utils.execute_command("nice", "-n", "10", *instructions)
+    Discourse::Utils.execute_command("nice", "-n", "10", *instructions, timeout: MAX_CONVERT_SECONDS)
 
     allow_pngquant = to.downcase.ends_with?(".png") && File.size(to) < MAX_PNGQUANT_SIZE
     FileHelper.optimize_image!(to, allow_pngquant: allow_pngquant)

app/models/upload.rb

@@ -13,6 +13,7 @@ class Upload < ActiveRecord::Base
   SHA1_LENGTH = 40
   SEEDED_ID_THRESHOLD = 0
   URL_REGEX ||= /(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/
+  MAX_IDENTIFY_SECONDS = 5
 
   belongs_to :user
   belongs_to :access_control_post, class_name: 'Post'
@@ -225,7 +226,7 @@ class Upload < ActiveRecord::Base
 
     begin
       if extension == 'svg'
-        w, h = Discourse::Utils.execute_command("identify", "-format", "%w %h", path).split(' ') rescue [0, 0]
+        w, h = Discourse::Utils.execute_command("identify", "-format", "%w %h", path, timeout: MAX_IDENTIFY_SECONDS).split(' ') rescue [0, 0]
       else
         w, h = FastImage.new(path, raise_on_failure: true).size
       end
@@ -274,7 +275,7 @@ class Upload < ActiveRecord::Base
   end
 
   def target_image_quality(local_path, test_quality)
-    @file_quality ||= Discourse::Utils.execute_command("identify", "-format", "%Q", local_path).to_i rescue 0
+    @file_quality ||= Discourse::Utils.execute_command("identify", "-format", "%Q", local_path, timeout: MAX_IDENTIFY_SECONDS).to_i rescue 0
 
     if @file_quality == 0 || @file_quality > test_quality
       test_quality

lib/discourse.rb

@@ -95,7 +95,13 @@ module Discourse
 
       private
 
-      def execute_command(*command, failure_message: "", success_status_codes: [0], chdir: ".")
+      def execute_command(*command, timeout: nil, failure_message: "", success_status_codes: [0], chdir: ".")
+        if timeout
+          # will send a TERM after timeout
+          # will send a KILL after timeout * 2
+          command = ["timeout", "-k", "#{timeout.to_f * 2}", timeout.to_s] + command
+        end
+
         stdout, stderr, status = Open3.capture3(*command, chdir: chdir)
 
         if !status.exited? || !success_status_codes.include?(status.exitstatus)

lib/theme_store/git_importer.rb

@@ -82,7 +82,7 @@ class ThemeStore::GitImporter
       else
         Discourse::Utils.execute_command("git", "clone", @url, @temp_folder)
       end
-    rescue RuntimeError => err
+    rescue RuntimeError
       raise RemoteTheme::ImportError.new(I18n.t("themes.import_error.git"))
     end
   end

lib/upload_creator.rb

@@ -125,7 +125,15 @@ class UploadCreator
 
       if is_image
         if @image_info.type.to_s == 'svg'
-          w, h = Discourse::Utils.execute_command("identify", "-format", "%w %h", @file.path).split(' ') rescue [0, 0]
+          w, h = [0, 0]
+
+          begin
+            w, h = Discourse::Utils
+              .execute_command("identify", "-format", "%w %h", @file.path, timeout: Upload::MAX_IDENTIFY_SECONDS)
+              .split(' ')
+          rescue
+            # use default 0, 0
+          end
         else
           w, h = @image_info.size
         end
@@ -264,6 +272,7 @@ class UploadCreator
     extract_image_info!
   end
 
+  MAX_CONVERT_FORMAT_SECONDS = 20
   def execute_convert(from, to, opts = {})
     command = [
       "convert",
@@ -277,7 +286,11 @@ class UploadCreator
     command << "-quality" << opts[:quality].to_s if opts[:quality]
     command << to
 
-    Discourse::Utils.execute_command(*command, failure_message: I18n.t("upload.png_to_jpg_conversion_failure_message"))
+    Discourse::Utils.execute_command(
+      *command,
+      failure_message: I18n.t("upload.png_to_jpg_conversion_failure_message"),
+      timeout: MAX_CONVERT_FORMAT_SECONDS
+    )
   end
 
   def should_alter_quality?
@@ -354,13 +367,14 @@ class UploadCreator
     @image_info.orientation.to_i > 1
   end
 
+  MAX_FIX_ORIENTATION_TIME = 5
   def fix_orientation!
     path = @file.path
 
     OptimizedImage.ensure_safe_paths!(path)
     path = OptimizedImage.prepend_decoder!(path, nil, filename: "image.#{@image_info.type}")
 
-    Discourse::Utils.execute_command('convert', path, '-auto-orient', path)
+    Discourse::Utils.execute_command('convert', path, '-auto-orient', path, timeout: MAX_FIX_ORIENTATION_TIME)
 
     extract_image_info!
   end
@@ -458,7 +472,7 @@ class UploadCreator
         OptimizedImage.ensure_safe_paths!(@file.path)
 
         command = ["identify", "-format", "%n\\n", @file.path]
-        frames = Discourse::Utils.execute_command(*command).to_i rescue 1
+        frames = Discourse::Utils.execute_command(*command, timeout: Upload::MAX_IDENTIFY_SECONDS).to_i rescue 1
 
         frames > 1
       else

spec/components/discourse_spec.rb

@@ -398,6 +398,12 @@ describe Discourse do
       expect(Discourse::Utils.execute_command("pwd", chdir: "plugins").strip).to eq("#{Rails.root.to_s}/plugins")
     end
 
+    it "supports timeouts" do
+      expect do
+        Discourse::Utils.execute_command("sleep", "999999999999", timeout: 0.001)
+      end.to raise_error(RuntimeError)
+    end
+
     it "works with a block" do
       Discourse::Utils.execute_command do |runner|
         expect(runner.exec("pwd").strip).to eq(Rails.root.to_s)