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

Use asyncio.create_subprocess instead of subprocess.run #235

Merged

Conversation

marczalik
Copy link
Contributor

One sentence summary of this PR (This should go in the CHANGELOG!)
Use asyncio.create_subprocess_execinstead of blocking subprocess.run calls in packers/unpackers.

Link to Related Issue(s)
#53

Please describe the changes in your request.
See above

Anyone you think should look at this, specifically?
@rbs-jacob

Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

I made a few comments, but the comments generally need to be applied across all of the places that were switched from subprocess to asyncio.

ofrak_core/ofrak/core/apk.py Outdated Show resolved Hide resolved
ofrak_core/CHANGELOG.md Outdated Show resolved Hide resolved
ofrak_core/ofrak/core/apk.py Outdated Show resolved Hide resolved
ofrak_core/ofrak/core/apk.py Outdated Show resolved Hide resolved
@rbs-jacob
Copy link
Member

rbs-jacob commented Feb 27, 2023

Here are some additional places that were missed that should probably also be converted:

jacob@jacob ofrak % rg -i subprocess | rg -v 'asyncio|test|patch_maker|build_image|ipynb|\.md|ofrak_ghidra'
./ofrak_core/ofrak/model/component_model.py:import subprocess
./ofrak_core/ofrak/model/component_model.py:            retcode = subprocess.call(
./ofrak_core/ofrak/model/component_model.py:                stdout=subprocess.DEVNULL,
./ofrak_core/ofrak/model/component_model.py:                stderr=subprocess.DEVNULL,
./ofrak_core/ofrak/component/abstract.py:from subprocess import CalledProcessError
./ofrak_core/ofrak/component/abstract.py:            raise ComponentSubprocessError(e)
./ofrak_core/ofrak/component/abstract.py:class ComponentSubprocessError(RuntimeError):
./ofrak_core/ofrak/core/apk.py:import subprocess
./ofrak_core/ofrak/core/apk.py:            retcode = subprocess.call(
./ofrak_core/ofrak/core/apk.py:                stdout=subprocess.DEVNULL,
./ofrak_core/ofrak/core/apk.py:                stderr=subprocess.DEVNULL,
./ofrak_core/ofrak/core/tar.py:import subprocess
./ofrak_core/ofrak/core/tar.py:                subprocess.run(command, check=True, capture_output=True)
./ofrak_core/ofrak/core/squashfs.py:import subprocess
./ofrak_core/ofrak/core/squashfs.py:            result = subprocess.run(
./ofrak_core/ofrak/core/squashfs.py:                stdout=subprocess.PIPE,
./ofrak_core/ofrak/core/squashfs.py:                stderr=subprocess.DEVNULL,

Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

One minor outstanding change

ofrak_core/ofrak/core/apk.py Outdated Show resolved Hide resolved
@rbs-jacob rbs-jacob merged commit 7906ec5 into redballoonsecurity:master Feb 28, 2023
Comment on lines +54 to +56
returncode = await proc.wait()
if returncode:
raise CalledProcessError(returncode=returncode, cmd=cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

@marczalik, here (and in other places in the PR) its seems that there are functional changes.

Before, it would return 0 == retcode. After this PR, any tool that does not have a return code of 0 raises a CalledProcessError. Was this change in functionality intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm asking because this changes the behavior of ComponentExternalTool, making it less usable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whyitfor In general, raising CalledProcessError for the majority of these cases is consistent with previous behavior, where the check=True in subprocess.run() does the same thing. (Here are the docs for reference)

However, you're correct in that this specific case , component_model.py, and 'squashfs.py` line 30 I should not have added these. I will create a bugfix for this.

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.

4 participants