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

Add proper exception handling in file descriptor enumeration #1514

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

atcuno
Copy link
Contributor

@atcuno atcuno commented Jan 3, 2025

The lack of exception handling caused backtraces in multiple plugins over 50+ samples. These cleans up those missing checks.

@atcuno atcuno requested a review from ikelos January 3, 2025 18:50
Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

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

There only needs to be a single try/except. Pull them together and this is fine...

@@ -307,15 +307,23 @@ def files_descriptors_for_process(
symbol_table: str,
task: interfaces.objects.ObjectInterface,
):
# task.files can be null
if not (task.files and task.files.is_readable()):
try:
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need the exception handler setting up twice, all the calls can happen with the try/except and any failure within that block will result in a return None.

@atcuno atcuno force-pushed the file_descriptors_exception_handling branch from b6f6c72 to e6f9ddb Compare January 31, 2025 17:40
@atcuno
Copy link
Contributor Author

atcuno commented Jan 31, 2025

feedback addressed now @ikelos

@ikelos
Copy link
Member

ikelos commented Jan 31, 2025

I meant more that the single exception handler should span to encompass both methods (and anything in between). Do we know that the second files call won't throw an exception?

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.

2 participants