diff --git a/gem_common.rb b/gem_common.rb index 529cd1398c..4fdfed704d 100644 --- a/gem_common.rb +++ b/gem_common.rb @@ -8,6 +8,7 @@ def self.dev_dependencies spec end def self.base_dependencies spec + spec.add_dependency "parallel", "~>1.20" spec.add_dependency "ruby_parser", "~>3.13" spec.add_dependency "ruby_parser-legacy", "~>1.0" spec.add_dependency "sexp_processor", "~> 4.7" diff --git a/lib/brakeman/file_parser.rb b/lib/brakeman/file_parser.rb index 3087b92113..a9d2e7ab36 100644 --- a/lib/brakeman/file_parser.rb +++ b/lib/brakeman/file_parser.rb @@ -1,3 +1,5 @@ +require 'parallel' + module Brakeman ASTFile = Struct.new(:path, :ast) @@ -13,21 +15,46 @@ def initialize app_tree, timeout end def parse_files list - read_files list do |path, contents| - if ast = parse_ruby(contents, path.relative) - ASTFile.new(path, ast) + # Parse the files in parallel. + # By default, the parsing will be in separate processes. + # So we map the result to ASTFiles and/or Exceptions + # then partition them into ASTFiles and Exceptions + # and add the Exceptions to @errors + # + # Basically just a funky way to deal with two possible + # return types that are returned from isolated processes. + # + # Note this method no longer uses read_files + @file_list, new_errors = Parallel.map(list) do |file_name| + file_path = @app_tree.file_path(file_name) + contents = file_path.read + + begin + if ast = parse_ruby(contents, file_path.relative) + ASTFile.new(file_name, ast) + end + rescue Exception => e + e end + end.compact.partition do |result| + result.is_a? ASTFile end + + errors.concat new_errors end def read_files list list.each do |path| file = @app_tree.file_path(path) - result = yield file, file.read + begin + result = yield file, file.read - if result - @file_list << result + if result + @file_list << result + end + rescue Exception => e + @errors << e end end end @@ -42,17 +69,12 @@ def parse_ruby input, path Brakeman.debug "Parsing #{path}" RubyParser.new.parse input, path, @timeout rescue Racc::ParseError => e - error e.exception(e.message + "\nCould not parse #{path}") + raise e.exception(e.message + "\nCould not parse #{path}") rescue Timeout::Error => e - error Exception.new("Parsing #{path} took too long (> #{@timeout} seconds). Try increasing the limit with --parser-timeout") + raise Exception.new("Parsing #{path} took too long (> #{@timeout} seconds). Try increasing the limit with --parser-timeout") rescue => e - error e.exception(e.message + "\nWhile processing #{path}") + raise e.exception(e.message + "\nWhile processing #{path}") end end - - def error exception - @errors << exception - nil - end end end diff --git a/lib/brakeman/scanner.rb b/lib/brakeman/scanner.rb index ac8d98d167..9ef8b95039 100644 --- a/lib/brakeman/scanner.rb +++ b/lib/brakeman/scanner.rb @@ -353,6 +353,9 @@ def index_call_sites def parse_ruby_file file fp = Brakeman::FileParser.new(tracker.app_tree, tracker.options[:parser_timeout]) fp.parse_ruby(file.read, file) + rescue Exception => e + tracker.error(e) + nil end end diff --git a/test/tests/file_parser.rb b/test/tests/file_parser.rb index 9828614fb1..e9357e5d3c 100644 --- a/test/tests/file_parser.rb +++ b/test/tests/file_parser.rb @@ -1,4 +1,5 @@ require_relative '../test' +require 'tempfile' class FileParserTests < Minitest::Test def setup @@ -8,21 +9,27 @@ def setup end def test_parse_error - @file_parser.parse_ruby <<-RUBY, "/tmp/BRAKEMAN_FAKE_PATH/test.rb" - x = - RUBY + tempfile = Tempfile.new + tempfile.write("x = ") + tempfile.close + @file_parser.parse_files([tempfile.path]) @tracker.add_errors(@file_parser.errors) assert_equal 1, @tracker.errors.length + ensure + tempfile.unlink end def test_parse_error_shows_newer_failure - @file_parser.parse_ruby <<-RUBY, "/tmp/BRAKEMAN_FAKE_PATH/test.rb" + tempfile = Tempfile.new + tempfile.write <<-RUBY blah(x: 1) thing do RUBY + tempfile.close + @file_parser.parse_files([tempfile.path]) @tracker.add_errors(@file_parser.errors) assert_equal 1, @tracker.errors.length @@ -34,6 +41,20 @@ def test_parse_error_shows_newer_failure end end + def test_read_files_reports_error + tempfile = Tempfile.new + tempfile.write("x = ") + tempfile.close + + @file_parser.read_files([tempfile.path]) do |path, contents| + @file_parser.parse_ruby contents, path + end + + @tracker.add_errors(@file_parser.errors) + + assert_equal 1, @tracker.errors.length + end + def test_parse_ruby_accepts_file_path file_path = Brakeman::FilePath.from_app_tree @tracker.app_tree, "config/test.rb"