Skip to content

Conversation

@Niraj-Kamdar
Copy link
Contributor

@Niraj-Kamdar Niraj-Kamdar commented Aug 8, 2020

Depends on #861
Fixes: #859

@Niraj-Kamdar Niraj-Kamdar marked this pull request as draft August 8, 2020 04:30
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2020

Codecov Report

Merging #860 into master will decrease coverage by 0.54%.
The diff coverage is 67.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
- Coverage   87.85%   87.31%   -0.55%     
==========================================
  Files         164      164              
  Lines        2710     2705       -5     
  Branches      295      295              
==========================================
- Hits         2381     2362      -19     
- Misses        258      272      +14     
  Partials       71       71              
Flag Coverage Δ
#longtests 87.31% <67.30%> (-0.55%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cve_bin_tool/output_engine/html.py 17.14% <22.22%> (+1.35%) ⬆️
cve_bin_tool/output_engine/util.py 84.00% <63.63%> (-16.00%) ⬇️
cve_bin_tool/cli.py 86.36% <71.42%> (-0.46%) ⬇️
cve_bin_tool/cve_scanner.py 78.64% <81.25%> (-2.96%) ⬇️
test/test_scanner.py 91.01% <83.33%> (+0.42%) ⬆️
cve_bin_tool/input_engine.py 98.00% <100.00%> (-0.34%) ⬇️
cve_bin_tool/output_engine/__init__.py 94.82% <100.00%> (-0.09%) ⬇️
cve_bin_tool/output_engine/console.py 100.00% <100.00%> (ø)
cve_bin_tool/version_scanner.py 86.08% <100.00%> (ø)
test/test_input_engine.py 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a4ef49...a5fe2f4. Read the comment docs.

from collections import defaultdict
from enum import Enum
from typing import NamedTuple

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put datastructures for triage and cve_scanner at one place to avoid circular import.

Comment on lines +56 to +70
class ProductInfo(NamedTuple):
vendor: str
product: str
version: str


class CVEData(defaultdict):
def __missing__(self, key):
if key == "cves":
self[key] = []
elif key == "paths":
self[key] = set()
else:
return NotImplemented
return self[key]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues with old structure:

  1. Old CVEData was NamedTuple and since newly added path attribute was mutable it can create hard to find bugs.
  2. To update path we need to scan all_cve_data to find product for which we want to append paths.
    Time Complexity: O(n**2) which can be reduced to O(n) using better structure.
  3. Throwing vendor, product, version in different function was decreasing readability. So, ProductInfo would be nice to pack this data together since we don't actually need that alone.
  4. TriageData structure wasn't syncing with old CVEData. So, csv2cve was breaking.

New structure is addressing all these issues.

product_info, file_path = scan_info
list_products.add(product_info.product)
list_versions.add(product_info.version)
assert file_path == expected_path
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for filepath :)

@Niraj-Kamdar Niraj-Kamdar marked this pull request as ready for review August 12, 2020 16:38
Copy link

@johnandersen777 johnandersen777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try fetching master and merging in latest HTML report changes. Then run the report and just double check that everything works

@johnandersen777 johnandersen777 merged commit cf7e827 into ossf:master Aug 12, 2020
@Niraj-Kamdar Niraj-Kamdar deleted the cve_scanner branch August 13, 2020 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: InputEngine is breaking after addition of file paths of CVEs.

4 participants