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

fix: allow extractraction on all files to fail #1285

Merged
merged 11 commits into from
Aug 2, 2021

Conversation

terriko
Copy link
Contributor

@terriko terriko commented Jul 28, 2021

Changes in #1181 to check return codes made it so that cve-bin-tool failed if any file did not extract correctly.

This changes the default so that failures during extraction are logged but do not halt the scan. Error messages are not logged when the file extension is .exe because users found those confusing (cve-bin-tool tries to unzip all .exe files in case they are self-extracting zipfiles, but many are not)

Signed-off-by: Terri Oda terri.oda@intel.com

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

Merging #1285 (89cc4ed) into main (2d69f84) will decrease coverage by 0.05%.
The diff coverage is 69.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1285      +/-   ##
==========================================
- Coverage   78.96%   78.90%   -0.06%     
==========================================
  Files         262      262              
  Lines        4801     4812      +11     
  Branches      578      580       +2     
==========================================
+ Hits         3791     3797       +6     
- Misses        856      861       +5     
  Partials      154      154              
Flag Coverage Δ
longtests 78.90% <69.23%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
cve_bin_tool/extractor.py 51.57% <42.85%> (+0.27%) ⬆️
test/test_extractor.py 100.00% <100.00%> (ø)
cve_bin_tool/async_utils.py 91.66% <0.00%> (-2.09%) ⬇️

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 2d69f84...89cc4ed. Read the comment docs.

@terriko
Copy link
Contributor Author

terriko commented Jul 28, 2021

We may wan to change the default across all extractors before merging this code, will discuss during our meeting tomorrow.

@terriko
Copy link
Contributor Author

terriko commented Jul 28, 2021

Discussed in meeting:

  • may want to open an issue to have a flag to force failure (does anyone want this or do they prefer to log?)
  • should change the other extractors to also have process_can_fail=True (and make sure to pass through to the aio_ function)

@terriko
Copy link
Contributor Author

terriko commented Jul 29, 2021

I checked with the person who originally reported the issue and this PR doesn't solve it yet. I'm going to work on adding some additional tests to see if we can cover their use case better, as well as extending the allow-fail behaviour to other checkers.

Changes in intel#1181 to check return codes made it so failed extractions
caused cve-bin-tool to halt rather than log an error. This was
particularly a problem for .exe files, which we try to extract as
zipfiles in case they are self-extracting exectables.

Signed-off-by: Terri Oda <terri.oda@intel.com>
This test is expected to fail currently and will need to pass before
this PR is ready to merge.

Signed-off-by: Terri Oda <terri.oda@intel.com>
@terriko terriko changed the title fix: allow extracts on zip files to fail fix: allow extractraction on all files to fail Jul 29, 2021
@pdxjohnny
Copy link
Member

Looks great for zip, however, the PR title implys we want to do this for all file types. If that's still the case then we should make sure to hit apk, cab, deb, and rpm. That said, I think that it makes more sense to just merge this PR as is for zip files and then do the others in other PRs

)
if stderr or not stdout:
if is_exe:

Choose a reason for hiding this comment

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

return (int(not is_exe))
Take it or leave it, just saves a couple of lines, but not great readability.

@terriko
Copy link
Contributor Author

terriko commented Aug 2, 2021

Looks great for zip, however, the PR title implys we want to do this for all file types. If that's still the case then we should make sure to hit apk, cab, deb, and rpm. That said, I think that it makes more sense to just merge this PR as is for zip files and then do the others in other PRs

It's not immediately obvious because I put in explicit changes for zip and then went ahead and changed the default here:

async def aio_run_command(args, process_can_fail=True):

But because the default has changed, all file types should fail without halting now. I should update the tests to include more than just zip-related extensions to show it, though.

@terriko terriko mentioned this pull request Aug 2, 2021
@terriko
Copy link
Contributor Author

terriko commented Aug 2, 2021

Actually, since I don't see any objections right now, and I think halting on .exe files is a problem, I'm going to go ahead and merge this and open a good first issue for additional tests for additional file extensions.

@terriko terriko merged commit 2e71440 into intel:main Aug 2, 2021
terriko added a commit that referenced this pull request Aug 4, 2021
* fixes #1281
* Related to #1181 

Changes in #1181 to check return codes made it so that cve-bin-tool failed if any file did not extract correctly. 

This changes the default so that failures during extraction are logged but do not halt the scan.  Error messages are not logged when the file extension is .exe because users found those confusing (cve-bin-tool tries to unzip all .exe files in case they are self-extracting zipfiles, but many are not)

Signed-off-by: Terri Oda <terri.oda@intel.com>
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.

cve-bin-tool halting during extract fails
4 participants