Fix MozillaSecurity#169 by adapting its fixes and review suggestions to master.

Author: nth10sd

src/funfuzz/js/compare_jit.py

@@ -61,38 +61,40 @@ def ignore_some_stderr(err_inp):
     return lines
 
 
-def compare_jit(jsEngine,  # pylint: disable=invalid-name,missing-param-doc,missing-type-doc,too-many-arguments
+def compare_jit(js_engine,  # pylint: disable=invalid-name,missing-param-doc,missing-type-doc,too-many-arguments
                 flags, infilename, logPrefix, repo, build_options_str, targetTime, options):
     """For use in loop.py
 
     Returns:
         bool: True if any kind of bug is found, otherwise False
     """
     # pylint: disable=too-many-locals
+    js_engine = Path(js_engine)
+    infilename = Path(infilename)
     # If Lithium uses this as an interestingness test, logPrefix is likely not a Path object, so make it one.
     logPrefix = Path(logPrefix)
     initialdir_name = (logPrefix.parent / (logPrefix.stem + "-initial"))
-    # pylint: disable=invalid-name
-    cl = compareLevel(jsEngine, flags, infilename, initialdir_name, options, False, True)
+    cl = compareLevel(js_engine,  # pylint: disable=invalid-name
+                      flags, infilename, initialdir_name, options, False, True)
     lev = cl[0]
 
     if lev != js_interesting.JS_FINE:
         itest = [__name__, "--flags=" + " ".join(flags),
-                 "--minlevel=" + str(lev), "--timeout=" + str(options.timeout), options.knownPath]
+                 "--minlevel=" + str(lev), "--timeout=" + str(options.timeout)]
         (lithResult, _lithDetails, autoBisectLog) = lithium_helpers.pinpoint(  # pylint: disable=invalid-name
-            itest, logPrefix, jsEngine, [], infilename, repo, build_options_str, targetTime, lev)
+            itest, logPrefix, js_engine, [], infilename, repo, build_options_str, targetTime, lev)
         if lithResult == lithium_helpers.LITH_FINISHED:
-            print("Retesting %s after running Lithium:" % infilename)
+            print("Retesting %s after running Lithium:" % str(infilename))
             finaldir_name = (logPrefix.parent / (logPrefix.stem + "-final"))
-            retest_cl = compareLevel(jsEngine, flags, infilename, finaldir_name, options, True, False)
+            retest_cl = compareLevel(js_engine, flags, infilename, finaldir_name, options, True, False)
             if retest_cl[0] != js_interesting.JS_FINE:
-                cl = retest_cl
+                cl = retest_cl  # pylint: disable=invalid-name
                 quality = 0
             else:
                 quality = 6
         else:
             quality = 10
-        print("compare_jit: Uploading %s with quality %s" % (infilename, quality))
+        print("compare_jit: Uploading %s with quality %s" % (str(infilename), quality))
 
         metadata = {}
         if autoBisectLog:
@@ -103,17 +105,19 @@ def compare_jit(jsEngine,  # pylint: disable=invalid-name,missing-param-doc,miss
     return False
 
 
-def compareLevel(jsEngine, flags, infilename, logPrefix, options, showDetailedDiffs, quickMode):
-    # pylint: disable=invalid-name,missing-docstring,missing-return-doc,missing-return-type-doc,too-complex
-    # pylint: disable=too-many-branches,too-many-arguments,too-many-locals
+def compareLevel(js_engine,  # pylint: disable=invalid-name,missing-docstring,missing-return-doc,missing-return-type-doc
+                 flags, infilename, logPrefix, options, showDetailedDiffs, quickMode):
+    # pylint: disable=too-complex,too-many-branches,too-many-arguments,too-many-locals
 
     # options dict must be one we can pass to js_interesting.ShellResult
-    # we also use it directly for knownPath, timeout, and collector
+    # we also use it directly for timeout, and collector
     # Return: (lev, crashInfo) or (js_interesting.JS_FINE, None)
 
-    assert isinstance(infilename, Path)
+    js_engine = Path(js_engine)
+    infilename = Path(infilename)
+    logPrefix = Path(logPrefix)
 
-    combos = shell_flags.basic_flag_sets(jsEngine)
+    combos = shell_flags.basic_flag_sets(str(js_engine))
 
     if quickMode:
         # Only used during initial fuzzing. Allowed to have false negatives.
@@ -122,7 +126,7 @@ def compareLevel(jsEngine, flags, infilename, logPrefix, options, showDetailedDi
     if flags:
         combos.insert(0, flags)
 
-    commands = [[jsEngine] + combo + [str(infilename)] for combo in combos]
+    commands = [[str(js_engine)] + combo + [str(infilename)] for combo in combos]
 
     for i, command in enumerate(commands):
         prefix = (logPrefix.parent / ("%s-r%s" % (logPrefix.stem, str(i))))
@@ -192,8 +196,7 @@ def optionDisabledAsmOnOneSide():  # pylint: disable=invalid-name,missing-docstr
                 rerunCommand = " ".join(quote(str(x)) for x in ["python -m funfuzz.js.compare_jit",
                                                                 "--flags=" + " ".join(flags),
                                                                 "--timeout=" + str(options.timeout),
-                                                                options.knownPath,
-                                                                jsEngine,
+                                                                str(js_engine),
                                                                 str(infilename.name)])
                 (summary, issues) = summarizeMismatch(mismatchErr, mismatchOut, prefix0, prefix)
                 summary = ("  " + " ".join(quote(str(x)) for x in commands[0]) + "\n  " +
@@ -209,7 +212,7 @@ def optionDisabledAsmOnOneSide():  # pylint: disable=invalid-name,missing-docstr
                     print(summary)
                     print()
                 # Create a crashInfo object with empty stdout, and stderr showing diffs
-                pc = ProgramConfiguration.fromBinary(jsEngine)  # pylint: disable=invalid-name
+                pc = ProgramConfiguration.fromBinary(str(js_engine))  # pylint: disable=invalid-name
                 pc.addProgramArguments(flags)
                 crashInfo = CrashInfo.CrashInfo.fromRawCrashData([], summary, pc)  # pylint: disable=invalid-name
                 return (js_interesting.JS_OVERALL_MISMATCH, crashInfo)
@@ -281,11 +284,10 @@ def parseOptions(args):  # pylint: disable=invalid-name
                       default="",
                       help="space-separated list of one set of flags")
     options, args = parser.parse_args(args)
-    if len(args) != 3:
-        raise Exception("Wrong number of positional arguments. Need 3 (knownPath, jsengine, infilename).")
-    options.knownPath = Path(args[0]).expanduser().resolve()
-    options.jsengine = Path(args[1]).expanduser().resolve()
-    options.infilename = Path(args[2]).expanduser().resolve()
+    if len(args) != 2:
+        raise Exception("Wrong number of positional arguments. Need 2 (jsengine, infilename).")
+    options.jsengine = Path(args[0]).expanduser().resolve()
+    options.infilename = Path(args[1]).expanduser().resolve()
     options.flags = options.flagsSpaceSep.split(" ") if options.flagsSpaceSep else []
     if not options.jsengine.is_file():
         raise OSError("js shell does not exist: " + options.jsengine)
@@ -298,24 +300,37 @@ def parseOptions(args):  # pylint: disable=invalid-name
     return options
 
 
-# For use by Lithium and autobisectjs. (autobisectjs calls init multiple times because it changes the js engine name)
-def init(args):
-    global gOptions  # pylint: disable=invalid-name,global-statement
-    gOptions = parseOptions(args)
+class Interesting(object):
+
+    def __init__(self, interestingness_script=False):
+        if interestingness_script:
+            global init, interesting  # pylint: disable=global-variable-undefined
+            init = self.init
+            interesting = self.interesting
+
+        self.args = None
+        self.real_level = None
+
+    # For use by Lithium and autobisectjs.
+    # (autobisectjs calls init multiple times because it changes the js engine name)
+    def init(self, args):
+        self.args = parseOptions(args)
+
+    def interesting(self, _args, cwd_prefix):  # pylint: disable=missing-docstring,missing-return-doc
+        # pylint: disable=missing-return-type-doc
+        self.real_level = compareLevel(
+            Path(self.args.jsengine), self.args.flags, Path(self.args.infilename),
+            Path(cwd_prefix), self.args, False, False)[0]
+        return self.real_level >= self.args.minimumInterestingLevel
 
 
-# FIXME: _args is unused here, we should check if it can be removed?  # pylint: disable=fixme
-def interesting(_args, cwd_prefix):
-    cwd_prefix = Path(cwd_prefix)  # Lithium uses this function and cwd_prefix from Lithium is not a Path
-    actualLevel = compareLevel(  # pylint: disable=invalid-name
-        gOptions.jsengine, gOptions.flags, gOptions.infilename, cwd_prefix, gOptions, False, False)[0]
-    return actualLevel >= gOptions.minimumInterestingLevel
+Interesting(interestingness_script=True)
 
 
 def main():
     options = parseOptions(sys.argv[1:])
     print(compareLevel(
-        options.jsengine, options.flags, options.infilename,  # pylint: disable=no-member
+        Path(options.jsengine), options.flags, Path(options.infilename),  # pylint: disable=no-member
         Path(tempfile.mkdtemp("compare_jitmain")), options, True, False)[0])
 
 

src/funfuzz/js/js_interesting.py

@@ -66,7 +66,7 @@
 
 class ShellResult(object):  # pylint: disable=missing-docstring,too-many-instance-attributes,too-few-public-methods
 
-    # options dict should include: timeout, knownPath, collector, valgrind, shellIsDeterministic
+    # options dict should include: timeout, collector, valgrind, shellIsDeterministic
     def __init__(self, options, runthis, logPrefix, in_compare_jit):  # pylint: disable=too-complex,too-many-branches
         # pylint: disable=too-many-locals,too-many-statements
 
@@ -327,8 +327,7 @@ def parseOptions(args):  # pylint: disable=invalid-name,missing-docstring,missin
     options, args = parser.parse_args(args)
     if len(args) < 2:
         raise Exception("Not enough positional arguments")
-    options.knownPath = args[0]
-    options.jsengineWithArgs = [Path(args[1]).resolve()] + args[2:-1] + [Path(args[-1]).resolve()]
+    options.jsengineWithArgs = [Path(args[0]).resolve()] + args[1:-1] + [Path(args[-1]).resolve()]
     assert options.jsengineWithArgs[0].is_file()  # js shell
     assert options.jsengineWithArgs[-1].is_file()  # testcase
     options.collector = create_collector.make_collector()
@@ -341,24 +340,30 @@ def parseOptions(args):  # pylint: disable=invalid-name,missing-docstring,missin
 # loop uses parseOptions and ShellResult [with in_compare_jit = False]
 # compare_jit uses ShellResult [with in_compare_jit = True]
 
-# For use by Lithium and autobisectjs. (autobisectjs calls init multiple times because it changes the js engine name)
-def init(args):  # pylint: disable=missing-docstring
-    global gOptions  # pylint: disable=global-statement,invalid-name
-    gOptions = parseOptions(args)
+class Interesting(object):  # pylint: disable=missing-docstring
 
+    def __init__(self, interestingness_script=False):
+        if interestingness_script:
+            global init, interesting  # pylint: disable=global-variable-undefined,invalid-name
+            init = self.init
+            interesting = self.interesting
 
-# FIXME: _args is unused here, we should check if it can be removed?  # pylint: disable=fixme
-def interesting(_args, cwd_prefix):  # pylint: disable=missing-docstring,missing-return-doc
-    # pylint: disable=missing-return-type-doc
-    cwd_prefix = Path(cwd_prefix)  # Lithium uses this function and cwd_prefix from Lithium is not a Path
-    options = gOptions
-    # options, runthis, logPrefix, in_compare_jit
-    res = ShellResult(options, options.jsengineWithArgs, cwd_prefix, False)
-    out_log = (cwd_prefix.parent / (cwd_prefix.stem + "-out")).with_suffix(".txt")
-    err_log = (cwd_prefix.parent / (cwd_prefix.stem + "-err")).with_suffix(".txt")
-    truncateFile(out_log, 1000000)
-    truncateFile(err_log, 1000000)
-    return res.lev >= gOptions.minimumInterestingLevel
+        self.args = None
+
+    def init(self, args):  # pylint: disable=missing-docstring
+        self.args = parseOptions(args)
+
+    def interesting(self, _args, cwd_prefix):  # pylint: disable=missing-docstring,missing-return-doc
+        # pylint: disable=missing-return-type-doc
+        cwd_prefix = Path(cwd_prefix)  # Lithium uses this function, and cwd_prefix from Lithium is not a Path
+        # options, runthis, logPrefix, in_compare_jit
+        res = ShellResult(self.args, self.args.jsengineWithArgs, cwd_prefix, False)
+        truncateFile((cwd_prefix.parent / (cwd_prefix.stem + "-out")).with_suffix(".txt"), 1000000)
+        truncateFile((cwd_prefix.parent / (cwd_prefix.stem + "-err")).with_suffix(".txt"), 1000000)
+        return res.lev >= self.args.minimumInterestingLevel
+
+
+Interesting(interestingness_script=True)
 
 
 # For direct, manual use

src/funfuzz/js/loop.py

@@ -75,10 +75,8 @@ def parseOpts(args):  # pylint: disable=invalid-name,missing-docstring,missing-r
     # lower = less time wasted in timeouts and in compare_jit testcases that are thrown away due to OOMs.
     options.timeout = int(args[0])
 
-    # FIXME: We can probably remove args[1]  # pylint: disable=fixme
-    options.knownPath = "mozilla-central"
-    options.jsEngine = Path(args[2])
-    options.engineFlags = args[3:]
+    options.js_engine = Path(args[1])
+    options.engineFlags = args[2:]
 
     return options
 
@@ -154,10 +152,9 @@ def many_timed_runs(targetTime, wtmpDir, args, collector):  # pylint: disable=in
         js_interesting_args.append("--timeout=" + str(options.timeout))
         if options.valgrind:
             js_interesting_args.append("--valgrind")
-        js_interesting_args.append(options.knownPath)
-        js_interesting_args.append(options.jsEngine)
+        js_interesting_args.append(str(options.js_engine))
         if options.randomFlags:
-            engineFlags = shell_flags.random_flag_set(options.jsEngine)  # pylint: disable=invalid-name
+            engineFlags = shell_flags.random_flag_set(str(options.js_engine))  # pylint: disable=invalid-name
             js_interesting_args.extend(engineFlags)
         js_interesting_args.extend(["-e", "maxRunTime=" + str(options.timeout * (1000 // 2))])
         js_interesting_args.extend(["-f", fuzzjs])
@@ -196,9 +193,8 @@ def many_timed_runs(targetTime, wtmpDir, args, collector):  # pylint: disable=in
                 itest.append("--valgrind")
             itest.append("--minlevel=" + str(res.lev))
             itest.append("--timeout=" + str(options.timeout))
-            itest.append(options.knownPath)
             (lithResult, _lithDetails, autoBisectLog) = lithium_helpers.pinpoint(  # pylint: disable=invalid-name
-                itest, logPrefix, options.jsEngine, engineFlags, reduced_log, options.repo,
+                itest, logPrefix, str(options.js_engine), engineFlags, reduced_log, options.repo,
                 options.build_options_str, targetTime, res.lev)
 
             # Upload with final output
@@ -237,7 +233,7 @@ def many_timed_runs(targetTime, wtmpDir, args, collector):  # pylint: disable=in
                 with io.open(str(jitcomparefilename), "w", encoding="utf-8", errors="replace") as f:
                     f.writelines(linesToCompare)
                 # pylint: disable=invalid-name
-                anyBug = compare_jit.compare_jit(options.jsEngine, engineFlags, jitcomparefilename,
+                anyBug = compare_jit.compare_jit(str(options.js_engine), engineFlags, jitcomparefilename,
                                                  logPrefix.parent / (logPrefix.stem + "-cj"), options.repo,
                                                  options.build_options_str, targetTime, js_interesting_options)
                 if not anyBug:

src/funfuzz/util/lithium_helpers.py

@@ -40,16 +40,16 @@
  LITH_FINISHED, LITH_RETESTED_STILL_INTERESTING, LITH_PLEASE_CONTINUE, LITH_BUSTED) = range(8)
 
 
-def pinpoint(itest, logPrefix, jsEngine, engineFlags, infilename,  # pylint: disable=invalid-name,missing-param-doc
+def pinpoint(itest, logPrefix, js_engine, engineFlags, infilename,  # pylint: disable=invalid-name,missing-param-doc
              bisectRepo, build_options_str, targetTime, suspiciousLevel):
     # pylint: disable=missing-return-doc,missing-return-type-doc,missing-type-doc,too-many-arguments,too-many-locals
     """Run Lithium and autobisectjs.
 
     itest must be an array of the form [module, ...] where module is an interestingness module.
-    The module's "interesting" function must accept [...] + [jsEngine] + engineFlags + infilename
+    The module's "interesting" function must accept [...] + [js_engine] + engineFlags + infilename
     (If it's not prepared to accept engineFlags, engineFlags must be empty.)
     """
-    lithArgs = itest + [str(jsEngine)] + engineFlags + [str(infilename)]  # pylint: disable=invalid-name
+    lithArgs = itest + [str(js_engine)] + engineFlags + [str(infilename)]  # pylint: disable=invalid-name
 
     (lithResult, lithDetails) = reduction_strat(  # pylint: disable=invalid-name
         logPrefix, infilename, lithArgs, targetTime, suspiciousLevel)
@@ -61,11 +61,11 @@ def pinpoint(itest, logPrefix, jsEngine, engineFlags, infilename,  # pylint: dis
 
     # pylint: disable=literal-comparison
     if (bisectRepo is not "none" and targetTime >= 3 * 60 * 60 and
-            build_options_str is not None and testJsShellOrXpcshell(jsEngine) != "xpcshell"):
+            build_options_str is not None and testJsShellOrXpcshell(js_engine) != "xpcshell"):
         autobisectCmd = (  # pylint: disable=invalid-name
             [sys.executable, "-u", "-m", "funfuzz.autobisectjs"] +
             ["-b", build_options_str] +
-            ["-p", " ".join(engineFlags + [infilename])] +
+            ["-p", " ".join(engineFlags + [str(infilename)])] +
             ["-i"] + itest
         )
         print(" ".join(quote(str(x)) for x in autobisectCmd))