Merge ~mdeslaur/ubuntu-cve-tracker:perf-part2 into ubuntu-cve-tracker:master

[Marc Deslauriers]

commit b1839ea0984addc4f1c18d64600bb36e62d5250e
Author: Marc Deslauriers <email address hidden>
Date: Fri Mar 1 08:53:41 2024 -0500

    check-syntax: improve performance by eliminating subprojects check

    check_cve() was attempting to determine if a specified CVE path
    was in a subproject by going through all the subprojects directories,
    but just to end up using find_cve(). We should just unconditionally
    use find_cve() here to find the base CVE file, unless the CVE
    specified a path to the boilerplates directory.

commit fb769299890f579b2ac5983b0a6ff1b14999820a
Author: Marc Deslauriers <email address hidden>
Date: Fri Mar 1 08:50:47 2024 -0500

    Improve find_external_subproject_cves() performance

    In find_external_subproject_cves(), only look inside subdirectories
    if the CVE wasn't found in the top-level. Also, only call realpath
    if the file exists. Add option to disable realpath lookups.

BEFORE:

$ time ./scripts/check-syntax -j8

real 2m25.430s
user 15m53.184s
sys 2m57.945s

AFTER:

$ time ./scripts/check-syntax -j8

real 1m26.673s
user 9m49.054s
sys 1m16.596s

[Seth Arnold (seth-arnold)]

Nice!

I'm not sure why we have realpath() scattered all over the tools. I didn't see anything very compelling in git history to explain why we have them.

If none of us can recall why we've got them, at some point we ought to flip that switch and find out what breaks. It might take a while.

Thanks

6a7dbc9... by Marc Deslauriers on 2024-03-03

Remove find_external_subproject_cves() use to improve performance

To check if the main CVE file incorrectly contains a subproject
entry, find_external_subproject_cves() was used but that is expensive,
so instead, just check if the entry was found in the main CVE file
instead.

c6e5588... by Marc Deslauriers on 2024-03-03

Disable use of realpath() in find_external_subproject_cves()

The only remaining use of find_external_subproject_cves() is when
load_cve() loads subproject files. I can't think of a good
reason why we would need to use realpath() here, so turn it
off by default. Also make code a little less redundant.

[Marc Deslauriers (mdeslaur)]

I have added two more commits to this merge proposal to clean up find_external_subproject_cves() further and to completely remove its use in check-syntax.

[Marc Deslauriers (mdeslaur)]

The two new commits shaved off another few seconds

$ time ./scripts/check-syntax -j8

real 1m18.166s
user 8m54.048s
sys 1m4.496s

5a276aa... by Marc Deslauriers on 2024-03-03

check-syntax: remove disk access from is_* functions

is_active(), is_embargoed(), and is_retired() would all check if
the file existed on disk, which is expensive. Instead, just check
the path for these locations. Also, replace manual checks for the
boilerplate directory with a is_boilerplate() function.

[Marc Deslauriers (mdeslaur)]

The removed file checks in the last commit reduced the runtime pretty good:

$ time ./scripts/check-syntax -j8

real 0m54.352s
user 6m27.479s
sys 0m12.230s

f81d2da... by Marc Deslauriers on 2024-03-03

cve_lib.py: if we're loading boilerplates, don't try in subprojects

e429156... by Marc Deslauriers on 2024-03-03

Build up a cache of subproject CVEs and directories to improve performance

aca4f51... by Marc Deslauriers on 2024-03-03

cve_lib.py: use itertools cache on get_subproject_details()

[Marc Deslauriers (mdeslaur)]

I've added some caching, and now get:

$ time ./scripts/check-syntax -j8

real 0m49.048s
user 5m40.278s
sys 0m6.770s

fc0b575... by Marc Deslauriers on 2024-03-03

cve_lib.py: use lru_cache so it works with focal's python

[Alex Murray (alexmurray)]

Thanks for working on this Marc - I'll give it a run locally.

@seth-arnold - regarding realpath - I added thus to support reading a CVE from stdin for check-syntax - ie:

./scripts/check-syntax --stdin < active/CVE-YYYY-NNNNN

(since this then allows to support integrating check-syntax with an external editor for on-the-fly syntax checking).

But performance improvements are more important than having this work at the moment so I am more than happy to forego it whilst we get performance improved, then I can look to add it back if it is still needed (and try and retain the performance gains in the process).

[Alex Murray (alexmurray)]

LGTM (a small fixup is needed to get --stdin working again with this but that can land after):

diff --git a/scripts/check-syntax b/scripts/check-syntax
index ba2645516a4..1dd37810acd 100755
--- a/scripts/check-syntax
+++ b/scripts/check-syntax
@@ -538,14 +538,18 @@ def check_cve(cve):
     cve_okay = True
     srcmap = dict()
     try:
- # Try to locate the base CVE and load that, but not if this is a
- # boilerplate path
- if is_boilerplate(cve):
+ if opt.stdin:
+ # fixup cve below once we have loaded it
             cvepath = cve
- cve = os.path.basename(cve)
         else:
- cve = os.path.basename(cve)
- cvepath = cve_lib.find_cve(cve)
+ # Try to locate the base CVE and load that, but not if this is a
+ # boilerplate path
+ if is_boilerplate(cve):
+ cvepath = cve
+ cve = os.path.basename(cve)
+ else:
+ cve = os.path.basename(cve)
+ cvepath = cve_lib.find_cve(cve)
         data = cve_lib.load_cve(cvepath, opt.strict, srcmap=srcmap)
     except ValueError as e:
         print(e, file=sys.stderr)

review: Approve

diff --git a/scripts/check-syntax b/scripts/check-syntax
index b740331..ba26455 100755
--- a/scripts/check-syntax
+++ b/scripts/check-syntax
@@ -107,18 +107,21 @@ def ever_existed(pkg):
     return False
 
 
-def is_active(cve):
-    return os.path.exists(os.path.join(cve_lib.active_dir, cve))
+def is_active(cvepath):
+    return cvepath.startswith(cve_lib.active_dir)
 
 
-def is_embargoed(cve):
-    return os.path.exists(os.path.join(cve_lib.embargoed_dir, cve))
+def is_embargoed(cvepath):
+    return cvepath.startswith(cve_lib.embargoed_dir)
 
 
-def is_retired(cve):
-    return os.path.exists(os.path.join(cve_lib.retired_dir, cve))
+def is_retired(cvepath):
+    return cvepath.startswith(cve_lib.retired_dir)
 
 
+def is_boilerplate(cvepath):
+    return cvepath.startswith(cve_lib.boilerplates_dir)
+
 def is_unpublished_kernel(kernel, release=None, debug=False):
     for u in unpublished_kernels:
         unpub = u.split("/")
@@ -352,7 +355,7 @@ elif len(args) == 0:
                 if filename.startswith("%s/CVE-" % dir):
                     args += [filename]
                     continue
-            if cve_lib.boilerplates_dir in filename:
+            if is_boilerplate(filename):
                 args += [filename]
 
     else:
@@ -380,6 +383,9 @@ ignored = cve_lib.parse_CVEs_from_uri("%s/not-for-us.txt" % cve_lib.ignored_dir)
 
 debug("ignored %s" % ignored)
 
+# Build up the cache of subproject CVEs
+cve_lib.build_subproject_dir_cache()
+
 cna_cves_set = CVEs_from_CNA()
 # Just run this if we're not specifying specific CVEs
 if len(cna_cves_set) > 0 and all_files:
@@ -529,21 +535,18 @@ def get_cve_path(cve, rel):
     return cvepath
 
 def check_cve(cve):
-    if re.match(r"EMB-", cve):
-        cvepath = os.path.join(cve_lib.embargoed_dir, cve)
-    elif re.match(r"CVE-", cve):
-        cvepath = os.path.join(cve_lib.active_dir, cve)
-    else:
-        cvepath = cve
-    cve = os.path.basename(cvepath)
-
     cve_okay = True
     srcmap = dict()
     try:
-        if cvepath in cve_lib.find_external_subproject_cves(cve):
-            data = cve_lib.load_cve(cve_lib.find_cve(cve), opt.strict, srcmap=srcmap)
+        # Try to locate the base CVE and load that, but not if this is a
+        # boilerplate path
+        if is_boilerplate(cve):
+            cvepath = cve
+            cve = os.path.basename(cve)
         else:
-            data = cve_lib.load_cve(cvepath, opt.strict, srcmap=srcmap)
+            cve = os.path.basename(cve)
+            cvepath = cve_lib.find_cve(cve)
+        data = cve_lib.load_cve(cvepath, opt.strict, srcmap=srcmap)
     except ValueError as e:
         print(e, file=sys.stderr)
         return False
@@ -580,7 +583,7 @@ def check_cve(cve):
             cve_okay = False
 
     # verify candidate field matches the CVE file name but ignore if we don't have one
-    if "stdin" not in cve and cve_lib.boilerplates_dir not in cvepath and not data["Candidate"] == cve:
+    if "stdin" not in cve and not is_boilerplate(cvepath) and not data["Candidate"] == cve:
         filename = srcmap["Candidate"][0]
         linenum = srcmap["Candidate"][1]
         print(
@@ -606,7 +609,7 @@ def check_cve(cve):
         for rel in all_required_releases:
             if rel in cve_lib.external_releases:
                 # for external releases skip boilerplates and embargoed CVEs
-                if 'boilerplates' in cvepath or is_embargoed(cve):
+                if is_boilerplate(cvepath) or is_embargoed(cvepath):
                     continue
 
             if rel in source:
@@ -615,7 +618,7 @@ def check_cve(cve):
                     _, series = cve_lib.product_series(rel)
                     # If the series is not listed in the original CVE,
                     # we won't create the CVE in the subproject.
-                    if not is_active(cve) and series not in listed_series:
+                    if not is_active(cvepath) and series not in listed_series:
                         skip_aliases = True
 
                     # We are not adding CVEs in ignored.
@@ -655,7 +658,7 @@ def check_cve(cve):
         missing_releases = all_required_releases - listed_releases
         for rel in missing_releases:
             # only warn on active CVEs
-            if is_active(cve) and \
+            if is_active(cvepath) and \
                rel in source and pkg in source[rel]:
                 
                 # If the subproject doesn't require cve triage,
@@ -689,7 +692,7 @@ def check_cve(cve):
             linenum = srcmap["pkgs"][pkg][release][1]
             # warn if the cve file contains external subproject releases
             # and it is not located in the subprojects folder
-            if rel in cve_lib.external_releases and filename not in cve_lib.find_external_subproject_cves(cve):
+            if rel in cve_lib.external_releases and filename == cvepath:
                 print(
                     "%s: %d: external release '%s' listed in internal CVE file"
                     % (filename, linenum, rel),
@@ -800,7 +803,7 @@ def check_cve(cve):
 
             # Check that package exists in a given release
             if not ever_existed(pkg):
-                if is_active(cve) and not cve_lib.is_active_esm_release(release):
+                if is_active(cvepath) and not cve_lib.is_active_esm_release(release):
                     # forcibly skip linux-lts-backport packages and #
                     # other derived kernels since want to track them
                     # before they end up fully in the archive;
@@ -825,7 +828,7 @@ def check_cve(cve):
             else:
                 if rel in source:
                     if pkg not in source[rel]:
-                        if is_active(cve):
+                        if is_active(cvepath):
                             if is_unpublished_kernel(pkg, rel):
                                 if opt.debug:
                                     print(
@@ -903,7 +906,7 @@ def check_cve(cve):
                 # looking for kernels that were in the
                 # unpublished_kernels list that have subsequently been
                 # published and should be checked for existence.
-                if is_active(cve) and is_unpublished_kernel(pkg, rel):
+                if is_active(cvepath) and is_unpublished_kernel(pkg, rel):
                     # we'll only issue a warning the first time we come
                     # across a kernel that's been published
                     if pkg not in warned_kernels:
@@ -937,7 +940,7 @@ def check_cve(cve):
     # Verify priority for any CVE with a supported package
     if (
         len(supported)
-        and (is_active(cve) or is_embargoed(cve))
+        and (is_active(cvepath) or is_embargoed(cvepath))
         and ("Priority" not in data or data["Priority"][0] not in cve_lib.priorities)
     ):
         filename = srcmap["Priority"][0] if "Priority" in srcmap else cvepath
@@ -1005,7 +1008,7 @@ def check_cve(cve):
     # check to see if the description has been changed to rejected by
     # MITRE, we should move CVE to ignored state.
     if (
-        is_active(cve)
+        is_active(cvepath)
         and "Description" in data
         and data["Description"].lstrip().startswith("** REJECT **")
     ):
@@ -1065,7 +1068,7 @@ def check_cve(cve):
                     cve_okay = False
 
     # Either PublicDate or CRD must be set to something
-    if (cve_lib.boilerplates_dir not in cvepath and
+    if (not is_boilerplate(cvepath) and
             ("PublicDate" not in data or data["PublicDate"] == "") and
             ("CRD" not in data or data["CRD"] == "")
     ):
@@ -1082,11 +1085,11 @@ def check_cve(cve):
         )
 
     for d in ["PublicDate", "PublicDateAtUSN", "CRD"]:
-        if cve_lib.boilerplates_dir in cvepath:
+        if is_boilerplate(cvepath):
             continue
         passes = []
         # Embargoed CVEs can have an empty or unknown PublicDate
-        if d == "PublicDate" and is_embargoed(cve):
+        if d == "PublicDate" and is_embargoed(cvepath):
             passes = ["", "unknown"]
         if d == "PublicDateAtUSN":
             # PublicDateAtUSN can be empty
diff --git a/scripts/cve_lib.py b/scripts/cve_lib.py
index 2bd1d31..ccd89e7 100755
--- a/scripts/cve_lib.py
+++ b/scripts/cve_lib.py
@@ -26,6 +26,7 @@ import urllib.error
 import urllib.request
 
 from functools import reduce
+from functools import lru_cache
 
 def set_cve_dir(path):
     '''Return a path with CVEs in it. Specifically:
@@ -744,6 +745,7 @@ def product_series(rel):
     return product, series
 
 # get the subproject details for rel along with it's canonical name, product and series
+@lru_cache
 def get_subproject_details(rel):
     """Return the canonical name,product,series,details tuple for rel."""
     canon, product, series, details, release = None, None, None, None, None
@@ -974,17 +976,34 @@ def find_files_recursive(path, name):
                 matches.append(filepath)
     return matches
 
-def find_external_subproject_cves(cve):
+def find_external_subproject_cves(cve, realpath=False):
     """Return the list of external subproject CVE snippets for the given CVE."""
     cves = []
-    for rel in external_releases:
-        # fallback to the series specific subdir rather than just the
-        # top-level project directory even though this is preferred
-        for d in [get_external_subproject_cve_dir(rel),
-                  get_external_subproject_dir(rel)]:
-            path = os.path.realpath(os.path.join(d, cve))
-            if os.path.exists(path) and path not in cves:
-                cves.append(path)
+    # Use the cache if it's not empty
+    if subproject_dir_cache_cves:
+        if cve not in subproject_dir_cache_cves:
+            return cves
+        for entry in subproject_dir_cache_dirs:
+            path = os.path.join(entry, cve)
+            if os.path.exists(path):
+                if realpath:
+                    path = os.path.realpath(path)
+                if path not in cves:
+                    cves.append(path)
+    else:
+        for rel in external_releases:
+            # fallback to the series specific subdir rather than just the
+            # top-level project directory even though this is preferred
+            for path in [get_external_subproject_dir(rel),
+                         get_external_subproject_cve_dir(rel)]:
+                path = os.path.join(path, cve)
+                if os.path.exists(path):
+                    if realpath:
+                        path = os.path.realpath(path)
+                    if path not in cves:
+                        cves.append(path)
+                    break
+
     return cves
 
 # Keys in config.yml for a external subproject
@@ -1620,8 +1639,21 @@ if os.path.islink(embargoed_dir):
 EXIT_FAIL = 1
 EXIT_OKAY = 0
 
+subproject_dir_cache_cves = set()
+subproject_dir_cache_dirs = set()
+
 config = {}
 
+def build_subproject_dir_cache():
+    """Build a pre-cached list of the subproject files"""
+    for rel in external_releases:
+        # fallback to the series specific subdir rather than just the
+        # top-level project directory even though this is preferred
+        for path in [get_external_subproject_dir(rel),
+                     get_external_subproject_cve_dir(rel)]:
+            subproject_dir_cache_dirs.update([path])
+            cves = glob.glob("CVE-*", root_dir=path)
+            subproject_dir_cache_cves.update(cves)
 
 def parse_CVEs_from_uri(url):
     """Return a list of all CVE numbers mentioned in the given URL."""
@@ -2529,7 +2561,8 @@ def load_cve(cvefile, strict=False, srcmap=None):
 
     data['pkgs'] = affected
 
-    code, msg = load_external_subproject_cve_data(cvefile, data, srcmap, code, msg)
+    if not "boilerplate" in cvefile:
+        code, msg = load_external_subproject_cve_data(cvefile, data, srcmap, code, msg)
 
     if code != EXIT_OKAY:
         raise ValueError(msg.strip())