Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

canonical_convert still assumes it will find something, resulting in AttributeError: 'NoneType' object has no attribute 'group' #1639

Closed
wyattearp opened this issue Apr 14, 2022 · 4 comments · Fixed by #1643

Comments

@wyattearp
Copy link
Contributor

I was testing out the latest code and ran into the same issue on the 3.0 release as the current head ca1de05. The code assumes that the regex match will return something correctly, which isn't always the case.

Line will still fail like issue #1300 and #1519 are intended to fix:

parsed_version = parse_version(pv.group(0))

I patched it (poorly) in my code to do the following:

try:
    parsed_version = parse_version(pv.group(0))
except AttributeError:
    parsed_version = parse_version("99.99.99")
    self.logger.warn(
        f"error parsing {product_info.vendor}.{product_info.product} v{product_info.version} - manual inspection required"
    )

Likely there should be a configuration option on what happens if there's an error matching regular expressions, something like a "fail safe" and set the version to lowest possible value, which likely will create a lot of false positives, or "fail scary" which just drops a warning and tells the user to do manual inspection like I have above.

I'm happy to create a PR, but I'm not sure what the best approach should really be for this - thanks!

@terriko
Copy link
Contributor

terriko commented Apr 14, 2022

In other places, we return version UNKNOWN and print a warning, similar to your "fail scary" option, so I think the code above is probably in line with what we're doing now. I'd prefer one clear error message over a long list of false positives, and I think most users would like the same. Please go ahead and make a PR -- this is at least a decent partial solution for now.

I feel like we should probably make sure that this information is also presented in the report formats other than console so that it doesn't get missed. Could it be translated into an UNKNOWN entry with a comment explaining what went wrong? Or should we consider adding an error/warning section to the report that contains this type of information?

@terriko
Copy link
Contributor

terriko commented Apr 14, 2022

Another option: also return an error code if such a result is found (this would be helpful for CI systems and quiet mode to trigger further review)

@anthonyharrison
Copy link
Contributor

@terriko I have been looking at the packaging documentaion and I think if the version string is not PEP440 compliant it will raise an InvalidVersion exception (need to check this though) which we could then translate into an Unknown version.

However it does raise the question of why we are getting invalid versions as I think the values are coming from the NVD database. Should we be validating the version when we load the data into the database?

@wyattearp
Copy link
Contributor Author

@anthonyharrison - I'll see if I can get the exact package / binary that cause this issue, but it's on a disconnected network. Like the previous one, it was libxml2. I'll see if I can get a basic PR up sometime this week. Thanks!

wyattearp added a commit to wyattearp/cve-bin-tool that referenced this issue Apr 22, 2022
addresses intel#1639

re.search returns `None` on failure, updating to indicate the version is
`UNKNOWN` when this occurs and generating a log message
terriko pushed a commit that referenced this issue Apr 26, 2022
addresses #1639

re.search returns `None` on failure, updating to indicate the version is
`UNKNOWN` when this occurs and generating a log message
anthonyharrison pushed a commit to anthonyharrison/cve-bin-tool that referenced this issue May 2, 2022
addresses intel#1639

re.search returns `None` on failure, updating to indicate the version is
`UNKNOWN` when this occurs and generating a log message
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 a pull request may close this issue.

3 participants