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

Standalone extractor integration #218

Merged
merged 32 commits into from
May 15, 2019
Merged

Conversation

dorpvom
Copy link
Collaborator

@dorpvom dorpvom commented Feb 18, 2019

For extractor CI see

@codecov-io
Copy link

codecov-io commented Feb 19, 2019

Codecov Report

Merging #218 into master will decrease coverage by 0.13%.
The diff coverage is 91.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
- Coverage   95.76%   95.62%   -0.14%     
==========================================
  Files         345      272      -73     
  Lines       17787    15786    -2001     
==========================================
- Hits        17034    15096    -1938     
+ Misses        753      690      -63
Impacted Files Coverage Δ
src/helperFunctions/process.py 73.58% <ø> (-0.97%) ⬇️
src/test/unit/unpacker/test_unpacker.py 100% <100%> (ø) ⬆️
...s/analysis/qemu_exec/test/test_plugin_qemu_exec.py 100% <100%> (ø) ⬆️
...ation/scheduler/test_unpack_analyse_and_compare.py 100% <100%> (ø) ⬆️
...ion/scheduler/test_regression_virtual_file_path.py 98.55% <100%> (ø)
src/plugins/analysis/qemu_exec/code/qemu_exec.py 100% <100%> (ø) ⬆️
src/unpacker/tar_repack.py 100% <100%> (ø) ⬆️
src/test/unit/helperFunctions/test_file_system.py 100% <100%> (ø) ⬆️
src/test/unit/web_interface/test_plugin_routes.py 100% <100%> (ø) ⬆️
src/web_interface/components/plugin_routes.py 100% <100%> (ø) ⬆️
... and 7 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 3f09408...7e827eb. Read the comment docs.

Copy link
Contributor

@weidenba weidenba left a comment

Choose a reason for hiding this comment

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

fact_extractor issue must be fixed before merge: fkie-cad/fact_extractor#5

It also adds a useless "files" folder, that confuses me

@@ -83,11 +83,11 @@ def file_is_empty(file_path):
Returns False otherwise
'''
try:
if os.path.getsize(file_path) == 0:
if os.path.getsize(str(file_path)) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not an issue but more a general discussion. Should we patch all our helper functions to supoort Paht objects?
I would say yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. But I'd like to do this in a separate branch to not overcrowd this already substantial one

def _repack_extracted_files(extraction_dir, out_file_path):
with Popen('tar -C {} -cvzf {} .'.format(extraction_dir.name, out_file_path), shell=True, stdout=PIPE) as process:
def _repack_extracted_files(extraction_dir: Path, out_file_path: str) -> bytes:
with Popen('tar -C {} -cvzf {} .'.format(extraction_dir, out_file_path), shell=True, stdout=PIPE) as process:
Copy link
Contributor

Choose a reason for hiding this comment

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

we might better use the execute shell command helper function herre

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but, but, that's not really code I worked on 😢

@@ -1,6 +1,5 @@
import logging
Copy link
Contributor

Choose a reason for hiding this comment

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

get_faf_bin_dir() is no longer needed

Copy link
Collaborator Author

@dorpvom dorpvom Mar 25, 2019

Choose a reason for hiding this comment

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

has been removed

@dorpvom dorpvom requested a review from weidenba March 25, 2019 15:14
Copy link
Contributor

@weidenba weidenba left a comment

Choose a reason for hiding this comment

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

can be merged after 2.6 release.
Please update the Development Docs.

@dorpvom dorpvom added the review requested Review from core dev required / requested label Apr 24, 2019
@dorpvom
Copy link
Collaborator Author

dorpvom commented Apr 26, 2019

Will look to add a memory config option for extraction containers

@weidenba weidenba self-requested a review May 2, 2019 06:03
if return_code != 0:
error = 'Failed to execute docker extractor with code {}:\n{}'.format(return_code, output)
logging.error(error)
raise RuntimeError(error)

self.change_owner_back_to_me(tmp_dir)
all_items = list(Path(tmp_dir, 'files').glob('**/*'))
return [item for item in all_items if not item.is_dir()]
return [item for item in safe_rglob(Path(tmp_dir, 'files')) if not item.is_dir()]

def change_owner_back_to_me(self, directory: str = None, permissions='u+r'):
with Popen('sudo chown -R {}:{} {}'.format(getuid(), getgid(), directory), shell=True, stdout=PIPE, stderr=PIPE) as pl:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to use execute_shell_command helper function. This command does not work correctly, since any output on stderr overwrites stdout.

@weidenba weidenba added under review Core dev is looking into code and removed review requested Review from core dev required / requested labels May 15, 2019
@weidenba weidenba removed the under review Core dev is looking into code label May 15, 2019
@weidenba weidenba merged commit 322abfb into master May 15, 2019
@weidenba weidenba deleted the standalone-extractor-integration branch May 15, 2019 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants