Move timeout subtraction outside Engine.fuzz. (#2108)

Currently it's not obvious at all to users of Engine.fuzz how long
fuzzing will go for when a max_time is passed.

Add Engine.fuzz_additional_processing_timeout so that the caller can
explicitly subtract the necessary processing time themselves if needed.

Remove engine_common.POSTPROCESSING_TIME. This was a holdout from the
launcher days. This is moved to the AFL launcher and can be removed
there as well once that's migrated to the Engine interface.

Author: oliverchang

src/python/bot/fuzzers/afl/launcher.py

@@ -64,6 +64,10 @@
 # .options file option for the number of persistent executions.
 PERSISTENT_EXECUTIONS_OPTION = 'n'
 
+# Grace period for the launcher to complete any processing before it's killed.
+# This will no longer be needed when we migrate to the engine interface.
+POSTPROCESSING_TIMEOUT = 30
+
 
 class AflOptionType(object):
   ARG = 0
@@ -1440,7 +1444,7 @@ def get_first_stacktrace(stderr_data):
 
 def get_fuzz_timeout(is_mutations_run):
   """Get the maximum amount of time that should be spent fuzzing."""
-  hard_timeout = engine_common.get_hard_timeout()
+  hard_timeout = engine_common.get_hard_timeout() - POSTPROCESSING_TIMEOUT
   merge_timeout = engine_common.get_merge_timeout(DEFAULT_MERGE_TIMEOUT)
   fuzz_timeout = hard_timeout - merge_timeout
   mutations_timeout = engine_common.get_new_testcase_mutations_timeout()

src/python/bot/fuzzers/engine_common.py

@@ -52,9 +52,6 @@
 # file extension.
 SEED_CORPUS_ARCHIVE_SUFFIX = '_seed_corpus'
 
-# Number of seconds to allow for extra processing.
-POSTPROCESSING_TIME = 30.0
-
 # Maximum number of files in the corpus for which we will unpack the seed
 # corpus.
 MAX_FILES_FOR_UNPACK = 5
@@ -351,9 +348,7 @@ def get_hard_timeout(total_timeout=None):
   if total_timeout is None:
     total_timeout = environment.get_value('FUZZ_TEST_TIMEOUT')
 
-  # Give a small window of time to process (upload) the fuzzer output.
-  hard_timeout = total_timeout - POSTPROCESSING_TIME
-  return get_overridable_timeout(hard_timeout, 'HARD_TIMEOUT_OVERRIDE')
+  return get_overridable_timeout(total_timeout, 'HARD_TIMEOUT_OVERRIDE')
 
 
 def get_merge_timeout(default_merge_timeout):

src/python/bot/fuzzers/libFuzzer/engine.py

@@ -86,6 +86,21 @@ class LibFuzzerEngine(engine.Engine):
   def name(self):
     return 'libFuzzer'
 
+  def fuzz_additional_processing_timeout(self, options):
+    """Return the maximum additional timeout in seconds for additional
+    operations in fuzz() (e.g. merging back new items).
+
+    Args:
+      options: A FuzzOptions object.
+
+    Returns:
+      An int representing the number of seconds required.
+    """
+    fuzz_timeout = libfuzzer.get_fuzz_timeout(
+        options.is_mutations_run, total_timeout=0)
+    # get_fuzz_timeout returns a negative value.
+    return -fuzz_timeout
+
   def prepare(self, corpus_dir, target_path, build_dir):
     """Prepare for a fuzzing session, by generating options. Returns a
     FuzzOptions object.
@@ -245,11 +260,9 @@ def fuzz(self, target_path, options, reproducers_dir, max_time):
     new_corpus_dir = self._create_temp_corpus_dir('new')
 
     corpus_directories = [new_corpus_dir] + options.fuzz_corpus_dirs
-    fuzz_timeout = libfuzzer.get_fuzz_timeout(
-        options.is_mutations_run, total_timeout=max_time)
     fuzz_result = runner.fuzz(
         corpus_directories,
-        fuzz_timeout=fuzz_timeout,
+        fuzz_timeout=max_time,
         additional_args=options.arguments,
         artifact_prefix=reproducers_dir,
         extra_env=options.extra_env)
@@ -299,7 +312,7 @@ def fuzz(self, target_path, options, reproducers_dir, max_time):
     timeout_limit = fuzzer_utils.extract_argument(
         options.arguments, constants.TIMEOUT_FLAG, remove=False)
 
-    expected_duration = runner.get_max_total_time(fuzz_timeout)
+    expected_duration = runner.get_max_total_time(max_time)
     actual_duration = int(fuzz_result.time_executed)
     fuzzing_time_percent = 100 * actual_duration / float(expected_duration)
     parsed_stats.update({

src/python/bot/tasks/fuzz_task.py

@@ -1301,6 +1301,14 @@ def run_engine_fuzzer(engine_impl, target_name, sync_corpus_directory,
   options = engine_impl.prepare(sync_corpus_directory, target_path, build_dir)
 
   fuzz_test_timeout = environment.get_value('FUZZ_TEST_TIMEOUT')
+  additional_processing_time = engine_impl.fuzz_additional_processing_timeout(
+      options)
+  fuzz_test_timeout -= additional_processing_time
+  if fuzz_test_timeout <= 0:
+    raise FuzzTaskException(
+        f'Invalid engine timeout: '
+        f'{fuzz_test_timeout} - {additional_processing_time}')
+
   result = engine_impl.fuzz(target_path, options, testcase_directory,
                             fuzz_test_timeout)
 

src/python/lib/clusterfuzz/fuzz/engine.py

@@ -70,6 +70,19 @@ def name(self):
     """Get the name of the engine."""
     raise NotImplementedError
 
+  def fuzz_additional_processing_timeout(self, options):
+    """Return the maximum additional timeout in seconds for additional
+    operations in fuzz() (e.g. merging back new items).
+
+    Args:
+      options: A FuzzOptions object.
+
+    Returns:
+      An int representing the number of seconds required.
+    """
+    del options
+    return 0
+
   def prepare(self, corpus_dir, target_path, build_dir):
     """Prepare for a fuzzing session, by generating options. Returns a
     FuzzOptions object.

src/python/tests/core/bot/fuzzers/afl/afl_launcher_test.py

@@ -193,7 +193,7 @@ class AflFuzzInputDirectoryTest(LauncherTestBase):
 
   def setUp(self):
     self.temp_input_dir = os.path.join(self.TEMP_DIR, 'afl_input_dir')
-    super(AflFuzzInputDirectoryTest, self).setUp()
+    super().setUp()
     test_helpers.patch(self, ['bot.fuzzers.engine_common.is_lpm_fuzz_target'])
     self.mock.is_lpm_fuzz_target.return_value = True
     self.strategies = launcher.FuzzingStrategies(None)
@@ -315,7 +315,7 @@ class AflFuzzOutputDirectoryTest(LauncherTestBase):
   QUEUE_TESTCASE_INO = 2
 
   def setUp(self):
-    super(AflFuzzOutputDirectoryTest, self).setUp()
+    super().setUp()
     # Creates self.OUTPUT_DIR.
     self.afl_output = launcher.AflFuzzOutputDirectory()
     os.mkdir(self.CRASHES_DIR)
@@ -462,7 +462,7 @@ def assert_sanitizer_opts_set(self, sanitizer_options_variable,
       self.assertIn(opt, sanitizer_options_value)
 
   def setUp(self):
-    super(SetSanitizerOptionsTest, self).setUp()
+    super().setUp()
     test_helpers.patch_environ(self)
 
   def test_left_unset(self):
@@ -515,7 +515,7 @@ class AflRunnerTest(LauncherTestBase):
   BAD_FILE_CONTENTS = 'bad'
 
   def setUp(self):
-    super(AflRunnerTest, self).setUp()
+    super().setUp()
     test_helpers.patch_environ(self)
     test_helpers.patch(self, ['bot.fuzzers.engine_common.is_lpm_fuzz_target'])
     self.mock.is_lpm_fuzz_target.return_value = True
@@ -831,7 +831,7 @@ def test_validation(self):
   def test_correctness(self):
     """Test that get_fuzz_timeout returns what we expect."""
     expected_fuzz_timeout = (
-        self.valid_hard_timeout - engine_common.POSTPROCESSING_TIME -
+        self.valid_hard_timeout - launcher.POSTPROCESSING_TIMEOUT -
         self.valid_merge_timeout)
 
     self.call_helper(
@@ -845,7 +845,7 @@ class AflConfigTest(LauncherTestBase):
   """Test AflConfig."""
 
   def setUp(self):
-    super(AflConfigTest, self).setUp()
+    super().setUp()
     self.target_path = '/build_dir/target'
     self.options_path = self.target_path + '.options'
 
@@ -927,7 +927,7 @@ class ListFullFilePathsTest(LauncherTestBase):
   DUMMY_3_FILENAME = 'dummyfile3'
 
   def setUp(self):
-    super(ListFullFilePathsTest, self).setUp()
+    super().setUp()
     self.dummy_file_path = os.path.join(self.INPUT_DIR, fuzzer.AFL_DUMMY_INPUT)
     self.dummy_3_file_path, _ = self._create_file(self.DUMMY_3_FILENAME)
 

src/python/tests/core/bot/fuzzers/engine_common_test.py

@@ -107,7 +107,6 @@ def function():
   def setUp(self):
     test_helpers.patch_environ(self)
     self.valid_hard_timeout = 500
-    self.invalid_hard_timeout = engine_common.POSTPROCESSING_TIME - 1
     self.valid_merge_timeout = 15
 
   def _set_environment_values(self, environment_variable_values):
@@ -151,28 +150,19 @@ def test_hard_timeout_override_validation(self):
         'HARD_TIMEOUT_OVERRIDE': -1
     })
 
-  def test_fuzz_test_timeout_validation(self):
-    """Test that get_hard_timeout rejects invalid values of
-    FUZZ_TEST_TIMEOUT."""
-    self.validation_helper({
-        'FUZZ_TEST_TIMEOUT': engine_common.POSTPROCESSING_TIME - 1
-    })
-
   def test_hard_timeout_override_correctness(self):
     """Test that get_hard_timeout returns what we expect when we set
     HARD_TIMEOUT_OVERRIDE."""
-    self.call_helper(
-        self.valid_hard_timeout, {
-            'HARD_TIMEOUT_OVERRIDE': self.valid_hard_timeout,
-            'FUZZ_TEST_TIMEOUT': self.invalid_hard_timeout
-        })
+    self.call_helper(self.valid_hard_timeout, {
+        'HARD_TIMEOUT_OVERRIDE': self.valid_hard_timeout,
+        'FUZZ_TEST_TIMEOUT': -1
+    })
 
   def test_fuzz_test_timeout_correctness(self):
     """Test that get_hard_timeout returns what we expect when we set
     FUZZ_TEST_TIMEOUT."""
-    self.call_helper(
-        self.valid_hard_timeout - engine_common.POSTPROCESSING_TIME,
-        {'FUZZ_TEST_TIMEOUT': self.valid_hard_timeout})
+    self.call_helper(self.valid_hard_timeout,
+                     {'FUZZ_TEST_TIMEOUT': self.valid_hard_timeout})
 
 
 class GetMergeTimeoutTest(GetTimeoutTestBase):