From 138bcd9d9945653fb437bb572a767f2e1b4946e4 Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 17 May 2019 14:36:33 -0700 Subject: [PATCH] [#close 242] Threadsafe cache loader I'm honestly still not sure how to trigger this error, however the root cause looks like two threads trying to mutate the same values from the cache. https://github.com/rails/rails/issues/24627 The issue should be mitigated by copying the set to a local copy via `dup` and then mutating that. To accomplish this, two helper methods have been added. --- CHANGELOG.md | 2 + lib/sprockets/loader.rb | 99 ++++++++++++++++++++++------------------- 2 files changed, 54 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4ddf3db6..e70fe98dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Get upgrade notes from Sprockets 3.x to 4.x at https://github.com/rails/sprocket ## Master +- Fix threading bug introduced in Sprockets 4 [#603] + ## 4.0.0.beta8 - Security release for [CVE-2018-3760](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-3760) diff --git a/lib/sprockets/loader.rb b/lib/sprockets/loader.rb index 3680bbaa7..d3835e358 100644 --- a/lib/sprockets/loader.rb +++ b/lib/sprockets/loader.rb @@ -64,6 +64,43 @@ def load(uri) end private + def compress_key_from_hash(hash, key) + return unless hash.key?(key) + value = hash[key].dup + return if !value + + if block_given? + value.map! do |x| + if yield x + compress_from_root(x) + else + x + end + end + else + value.map! { |x| compress_from_root(x) } + end + hash[key] = value + end + + + def expand_key_from_hash(hash, key) + return unless hash.key?(key) + value = hash[key].dup + return if !value + if block_given? + value.map! do |x| + if yield x + expand_from_root(x) + else + x + end + end + else + value.map! { |x| expand_from_root(x) } + end + hash[key] = value + end # Internal: Load asset hash from cache # @@ -77,17 +114,16 @@ def asset_from_cache(key) asset[:uri] = expand_from_root(asset[:uri]) asset[:load_path] = expand_from_root(asset[:load_path]) asset[:filename] = expand_from_root(asset[:filename]) - asset[:metadata][:included].map! { |uri| expand_from_root(uri) } if asset[:metadata][:included] - asset[:metadata][:links].map! { |uri| expand_from_root(uri) } if asset[:metadata][:links] - asset[:metadata][:stubbed].map! { |uri| expand_from_root(uri) } if asset[:metadata][:stubbed] - asset[:metadata][:required].map! { |uri| expand_from_root(uri) } if asset[:metadata][:required] - asset[:metadata][:to_load].map! { |uri| expand_from_root(uri) } if asset[:metadata][:to_load] - asset[:metadata][:to_link].map! { |uri| expand_from_root(uri) } if asset[:metadata][:to_link] - asset[:metadata][:dependencies].map! { |uri| uri.start_with?("file-digest://") ? expand_from_root(uri) : uri } if asset[:metadata][:dependencies] + expand_key_from_hash(asset[:metadata], :included) + expand_key_from_hash(asset[:metadata], :stubbed) + expand_key_from_hash(asset[:metadata], :required) + expand_key_from_hash(asset[:metadata], :to_load) + expand_key_from_hash(asset[:metadata], :to_link) + expand_key_from_hash(asset[:metadata], :dependencies) { |uri| uri.start_with?("file-digest://") } asset[:metadata].each_key do |k| next unless k =~ /_dependencies\z/ - asset[:metadata][k].map! { |uri| expand_from_root(uri) } + expand_key_from_hash(asset[:metadata], k) end end asset @@ -205,48 +241,17 @@ def store_asset(asset, unloaded) if cached_asset[:metadata] # Deep dup to avoid modifying `asset` cached_asset[:metadata] = cached_asset[:metadata].dup - if cached_asset[:metadata][:included] && !cached_asset[:metadata][:included].empty? - cached_asset[:metadata][:included] = cached_asset[:metadata][:included].dup - cached_asset[:metadata][:included].map! { |uri| compress_from_root(uri) } - end - - if cached_asset[:metadata][:links] && !cached_asset[:metadata][:links].empty? - cached_asset[:metadata][:links] = cached_asset[:metadata][:links].dup - cached_asset[:metadata][:links].map! { |uri| compress_from_root(uri) } - end - - if cached_asset[:metadata][:stubbed] && !cached_asset[:metadata][:stubbed].empty? - cached_asset[:metadata][:stubbed] = cached_asset[:metadata][:stubbed].dup - cached_asset[:metadata][:stubbed].map! { |uri| compress_from_root(uri) } - end - - if cached_asset[:metadata][:required] && !cached_asset[:metadata][:required].empty? - cached_asset[:metadata][:required] = cached_asset[:metadata][:required].dup - cached_asset[:metadata][:required].map! { |uri| compress_from_root(uri) } - end - - if cached_asset[:metadata][:to_load] && !cached_asset[:metadata][:to_load].empty? - cached_asset[:metadata][:to_load] = cached_asset[:metadata][:to_load].dup - cached_asset[:metadata][:to_load].map! { |uri| compress_from_root(uri) } - end - - if cached_asset[:metadata][:to_link] && !cached_asset[:metadata][:to_link].empty? - cached_asset[:metadata][:to_link] = cached_asset[:metadata][:to_link].dup - cached_asset[:metadata][:to_link].map! { |uri| compress_from_root(uri) } - end - - if cached_asset[:metadata][:dependencies] && !cached_asset[:metadata][:dependencies].empty? - cached_asset[:metadata][:dependencies] = cached_asset[:metadata][:dependencies].dup - cached_asset[:metadata][:dependencies].map! do |uri| - uri.start_with?("file-digest://".freeze) ? compress_from_root(uri) : uri - end - end + compress_key_from_hash(cached_asset[:metadata], :included) + compress_key_from_hash(cached_asset[:metadata], :links) + compress_key_from_hash(cached_asset[:metadata], :stubbed) + compress_key_from_hash(cached_asset[:metadata], :required) + compress_key_from_hash(cached_asset[:metadata], :to_load) + compress_key_from_hash(cached_asset[:metadata], :to_link) + compress_key_from_hash(cached_asset[:metadata], :dependencies) { |uri| uri.start_with?("file-digest://") } - # compress all _dependencies in metadata like `sass_dependencies` cached_asset[:metadata].each do |key, value| next unless key =~ /_dependencies\z/ - cached_asset[:metadata][key] = value.dup - cached_asset[:metadata][key].map! {|uri| compress_from_root(uri) } + compress_key_from_hash(cached_asset[:metadata], key) end end