Race condition in Nokogiri::CSS::Parser

[ioquatix]

#!/usr/bin/env ruby

require 'nokogiri'

parser = Nokogiri::CSS::Parser.new

puts Nokogiri::CSS::Parser.cache_on?

threads = []

threads << Thread.new do
	Nokogiri::CSS::Parser.without_cache do
		sleep 1
	end
end

threads << Thread.new do
	sleep 0.1
	
	Nokogiri::CSS::Parser.without_cache do
		sleep 2
	end
end

threads.each(&:join)

puts Nokogiri::CSS::Parser.cache_on?

# Prints false, but should be true.

[flavorjones]

Thanks for reporting, I'll take a look as soon as I can.

[ioquatix]

The reason why it happens is because there is no locking around @cache_on which is shared global state:

nokogiri/lib/nokogiri/css/parser_extras.rb

Lines 34 to 39 in 24a2b5e

	def without_cache &block
	tmp = @cache_on
	@cache_on = false
	block.call
	@cache_on = tmp
	end

[ioquatix]

The solution is to use Thread.current[:nokogiri_cache_on] and assign to it directly rather than trying to read/write shared mutable state.

[flavorjones]

@ioquatix Can I ask how you found this bug? The #without_cache method was added in 8b9daef, and is only used in the Slop node decorator ... are you using #without_cache for something? I'm just curious.

You're right, of course, and we'll fix it.

[ioquatix]

I found it by manual analysis supported by some tools like grep and custom scripts to narrow down on potential issues.