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

Handle timeout errors when calling fact_extractor, related to #820 #852

Merged
merged 2 commits into from
Sep 22, 2022

Conversation

Oren-i
Copy link

@Oren-i Oren-i commented Sep 8, 2022

Timeouts are handled by catching all requests exceptions because requests raise ConnectionError on a timeout.

…d#820

Timeouts are handled by catching all requests exceptions because
requests raise ConnectionError on a timeout.
Copy link
Collaborator

@jstucke jstucke left a comment

Choose a reason for hiding this comment

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

Looks good to me and seems to work as intended with one exception: the tags don't seem to be rendered correctly

Comment on lines 53 to 58
file_object.processed_analysis['unpacker'] = {
'plugin_used': 'None', 'number_of_unpacked_files': 0, 'plugin_version': '0.0', 'analysis_date': time(),
'info': 'Unpacking stopped because extractor raised a exception (possible timeout)',
}
tag_dict = {'unpacker': {'extractor error': {'value': 'possible extractor timeout', 'color': TagColor.ORANGE, 'propagate': False}}}
file_object.analysis_tags.update(tag_dict)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tags don't seem to be saved correctly in the database. But something like this should work:

Suggested change
file_object.processed_analysis['unpacker'] = {
'plugin_used': 'None', 'number_of_unpacked_files': 0, 'plugin_version': '0.0', 'analysis_date': time(),
'info': 'Unpacking stopped because extractor raised a exception (possible timeout)',
}
tag_dict = {'unpacker': {'extractor error': {'value': 'possible extractor timeout', 'color': TagColor.ORANGE, 'propagate': False}}}
file_object.analysis_tags.update(tag_dict)
file_object.processed_analysis['unpacker'] = {
'plugin_used': 'None', 'number_of_unpacked_files': 0, 'plugin_version': '0.0', 'analysis_date': time(),
'info': 'Unpacking stopped because extractor raised a exception (possible timeout)',
'tags': {'extractor error': {'value': 'possible extractor timeout', 'color': TagColor.ORANGE, 'propagate': False}},
}

This also applies to the _store_unpacking_depth_skip_info() method below

Copy link
Author

Choose a reason for hiding this comment

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

I pushed the changed suggested (untested).

Copy link
Author

Choose a reason for hiding this comment

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

Converting to draft as the changes are going do cause unneeded timeouts when using fact_extractor in standalone mode. I think the correct way to implement this should be by getting the timeout value from fact_core as a parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Converting to draft as the changes are going do cause unneeded timeouts when using fact_extractor in standalone mode.

I'm not sure I can follow. The changes in this PR should not affect the extractor. Did you maybe mean fkie-cad/fact_extractor#94?

I just tested the changes again and everything seems to work fine (with one exception: analysis tags are not always rendered correctly for firmware containers but that is something that is out of scope for this PR).

Copy link
Author

Choose a reason for hiding this comment

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

You are correct, I meant to mark the other pull request as draft. This pull request is fine (I can not remove the draft status myself).

@jstucke
Copy link
Collaborator

jstucke commented Sep 21, 2022

What is more, it would probably be a good idea to add , 'tags' to the list in l. 30 of src/web_interface/templates/analysis_plugins/unpacker.html so that tags are no longer shown with the rest of the "analysis" data on the analysis results page of the unpacker

@Oren-i Oren-i marked this pull request as draft September 22, 2022 00:52
Copy link
Collaborator

@jstucke jstucke left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks again for your contribution!

@jstucke jstucke marked this pull request as ready for review September 22, 2022 12:33
@jstucke jstucke merged commit ae25a87 into fkie-cad:master Sep 22, 2022
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.

None yet

2 participants