fix: remove unnecessary shell=True in ROCm GPU architecture detection#7915
Conversation
Replace shell pipeline commands with list-based subprocess calls and Python regex matching. This eliminates unnecessary shell=True usage consistent with the hardening done for CVE-2024-43497 in the same file. Made-with: Cursor
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 852b2a852d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
op_builder/builder.py
Outdated
| result = subprocess.check_output([str(rocm_info)], stderr=subprocess.DEVNULL) | ||
| output = result.decode('utf-8') | ||
| match = re.search(r'gfx\S+', output) | ||
| rocm_gpu_arch = match.group(0).strip() if match else "" | ||
| except (subprocess.CalledProcessError, FileNotFoundError): |
There was a problem hiding this comment.
rocminfo launch errors before falling back
Switching from a shell pipeline to check_output([str(rocm_info)]) changes the failure mode when rocminfo exists but cannot be executed (for example, ROCm copied without the execute bit or mounted from a noexec filesystem in a container). The old shell path surfaced that as a non-zero exit status, which this code treated as a harmless fallback; here it raises PermissionError/other OSErrors that are not caught, so get_rocm_gpu_arch() and the identical call in get_rocm_wavefront_size() now abort ROCm extension builds instead of returning ""/"32".
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the feedback! Fixed in the latest commit — now catching OSError (parent of PermissionError) to handle cases where rocminfo exists but is not executable.
Address review feedback: catch PermissionError and other OSError subclasses that arise when rocminfo exists but cannot be executed (e.g. missing execute bit, noexec mount in containers). Made-with: Cursor
tohtana
left a comment
tohtana
left a comment
There was a problem hiding this comment.
Looks good to me, thank you @instantraaamen!
…deepspeedai#7915) ### Summary `get_rocm_gpu_arch()` and `get_rocm_wavefront_size()` in `op_builder/builder.py` use `subprocess.check_output()` with `shell=True` to pipe `rocminfo` output through `grep`. This is unnecessary and inconsistent with the rest of the codebase, which uses list-based subprocess calls after the CVE-2024-43497 fix. While the commands currently use fixed paths (not user-controlled), removing `shell=True` eliminates a potential attack surface and aligns with the hardening approach taken for CVE-2024-43497. ### Root Cause ```python # Before (shell=True) rocm_gpu_arch_cmd = str(rocm_info) + " | grep -o -m 1 'gfx.*'" result = subprocess.check_output(rocm_gpu_arch_cmd, shell=True) rocm_wavefront_size_cmd = str(rocm_info) + " | grep -Eo -m1 'Wavefront Size:[[:space:]]+[0-9]+' | grep -Eo '[0-9]+'" result = subprocess.check_output(rocm_wavefront_size_cmd, shell=True) ``` ### Changes - `op_builder/builder.py`: Replace shell pipeline with list-based `subprocess.check_output()` and Python `re.search()` on the full output of `rocminfo` (no `shell=True`), also catch `FileNotFoundError` for robustness ### Testing - Verified `shell=True` is fully removed from `op_builder/builder.py` (0 occurrences) - Python syntax validation: PASS - Both functions use list-based `subprocess.check_output([str(rocm_info)])`: confirmed - Non-ROCm systems return expected defaults (empty string / "32") --------- Co-authored-by: Masahiro Tanaka <81312776+tohtana@users.noreply.github.com> Signed-off-by: nathon-lee <leejianwoo@gmail.com>
2 participants
Summary
get_rocm_gpu_arch()andget_rocm_wavefront_size()inop_builder/builder.pyusesubprocess.check_output()withshell=Trueto piperocminfooutput throughgrep. This is unnecessary and inconsistent with the rest of the codebase, which uses list-based subprocess calls after the CVE-2024-43497 fix.While the commands currently use fixed paths (not user-controlled), removing
shell=Trueeliminates a potential attack surface and aligns with the hardening approach taken for CVE-2024-43497.Root Cause
Changes
op_builder/builder.py: Replace shell pipeline with list-basedsubprocess.check_output()and Pythonre.search()on the full output ofrocminfo(noshell=True), also catchFileNotFoundErrorfor robustnessTesting
shell=Trueis fully removed fromop_builder/builder.py(0 occurrences)subprocess.check_output([str(rocm_info)]): confirmed