From 5b99cb92752928356136c66c4e150abbdea7ca65 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 12 Apr 2021 13:55:54 +1000 Subject: [PATCH] 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 | 3 ++- app/models/upload.rb | 3 ++- lib/discourse.rb | 8 +++++++- lib/theme_store/git_importer.rb | 2 +- lib/upload_creator.rb | 12 +++++++++--- spec/components/discourse_spec.rb | 6 ++++++ 6 files changed, 27 insertions(+), 7 deletions(-) diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 0c33d7d1629..f73db79eb77 100644 --- a/app/models/optimized_image.rb +++ b/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) diff --git a/app/models/upload.rb b/app/models/upload.rb index dac34616af3..7c713ffb992 100644 --- a/app/models/upload.rb +++ b/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' @@ -270,7 +271,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 diff --git a/lib/discourse.rb b/lib/discourse.rb index d9db43e4e59..ac5ae71ba4a 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -94,7 +94,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) diff --git a/lib/theme_store/git_importer.rb b/lib/theme_store/git_importer.rb index 37fb49e6cdf..553016b07f3 100644 --- a/lib/theme_store/git_importer.rb +++ b/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 diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 1b37f47e79c..31d2747a9ab 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -190,7 +190,7 @@ class UploadCreator 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 end @frame_count > 1 @@ -269,6 +269,7 @@ class UploadCreator extract_image_info! end + MAX_CONVERT_FORMAT_SECONDS = 20 def execute_convert(from, to, opts = {}) command = [ "convert", @@ -282,7 +283,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? @@ -359,13 +364,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 diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb index f74b26f3d4a..145c1b6fbc3 100644 --- a/spec/components/discourse_spec.rb +++ b/spec/components/discourse_spec.rb @@ -392,6 +392,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)