From af7ba739a6d34b001e7bdc04d2e388639e2d3bac Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Thu, 11 Feb 2021 16:07:23 -0800 Subject: [PATCH 1/3] Parse files in parallel --- gem_common.rb | 1 + lib/brakeman/file_parser.rb | 15 +++++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) 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..fb61a93902 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,11 +15,16 @@ 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) + @file_list = Parallel.map(list) do |file_name| + file_path = @app_tree.file_path(file_name) + contents = file_path.read + + if ast = parse_ruby(contents, file_path.relative) + ASTFile.new(file_name, ast) + else + nil end - end + end.compact end def read_files list From b9e95363a3fb1df1d9e34e6610a0672fc5a0e6b7 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Wed, 28 Apr 2021 22:13:22 -0700 Subject: [PATCH 2/3] Fix up error reporting during parallel file parse --- lib/brakeman/file_parser.rb | 43 ++++++++++++++++++++++++++----------- test/tests/file_parser.rb | 29 +++++++++++++++++++++---- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/lib/brakeman/file_parser.rb b/lib/brakeman/file_parser.rb index fb61a93902..d7e6edc9a0 100644 --- a/lib/brakeman/file_parser.rb +++ b/lib/brakeman/file_parser.rb @@ -15,16 +15,35 @@ def initialize app_tree, timeout end def parse_files list - @file_list = Parallel.map(list) do |file_name| + # 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 - if ast = parse_ruby(contents, file_path.relative) - ASTFile.new(file_name, ast) + result = parse_ruby(contents, file_path.relative) + + case result + when Exception + result + when Sexp + ASTFile.new(file_name, result) else nil end - end.compact + end.compact.partition do |result| + result.is_a? ASTFile + end + + errors.concat new_errors end def read_files list @@ -33,8 +52,11 @@ def read_files list result = yield file, file.read - if result + case result + when ASTFile @file_list << result + when Exception + @errors << result end end end @@ -49,17 +71,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}") + 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") + 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}") + e.exception(e.message + "\nWhile processing #{path}") end end - - def error exception - @errors << exception - nil - end 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" From 7ccba40bfe19987a6eaf685c644055da541c5da8 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Wed, 28 Apr 2021 23:47:03 -0700 Subject: [PATCH 3/3] More parsing exception handling --- lib/brakeman/file_parser.rb | 34 ++++++++++++++++------------------ lib/brakeman/scanner.rb | 3 +++ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/brakeman/file_parser.rb b/lib/brakeman/file_parser.rb index d7e6edc9a0..a9d2e7ab36 100644 --- a/lib/brakeman/file_parser.rb +++ b/lib/brakeman/file_parser.rb @@ -29,15 +29,12 @@ def parse_files list file_path = @app_tree.file_path(file_name) contents = file_path.read - result = parse_ruby(contents, file_path.relative) - - case result - when Exception - result - when Sexp - ASTFile.new(file_name, result) - else - nil + 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 @@ -50,13 +47,14 @@ 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 - case result - when ASTFile - @file_list << result - when Exception - @errors << result + if result + @file_list << result + end + rescue Exception => e + @errors << e end end end @@ -71,11 +69,11 @@ def parse_ruby input, path Brakeman.debug "Parsing #{path}" RubyParser.new.parse input, path, @timeout rescue Racc::ParseError => e - e.exception(e.message + "\nCould not parse #{path}") + raise e.exception(e.message + "\nCould not parse #{path}") rescue Timeout::Error => e - 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 - e.exception(e.message + "\nWhile processing #{path}") + raise e.exception(e.message + "\nWhile processing #{path}") 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