From 5deda5ef3ef8763dfe936a313e0dddaa279a1b04 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
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 | 5 +++--
lib/discourse.rb | 8 +++++++-
lib/theme_store/git_importer.rb | 2 +-
lib/upload_creator.rb | 22 ++++++++++++++++++----
spec/components/discourse_spec.rb | 6 ++++++
6 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb
index e32f15a703c..228b3dc54be 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 bb6ba200174..0f1902c21be 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'
@@ -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
diff --git a/lib/discourse.rb b/lib/discourse.rb
index 5224b2aac2f..12f89a9c010 100644
--- a/lib/discourse.rb
+++ b/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)
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 bd9cadcee92..63364e7b4f1 100644
--- a/lib/upload_creator.rb
+++ b/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
diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb
index fe8db27fa19..6f1b294733c 100644
--- a/spec/components/discourse_spec.rb
+++ b/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)