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

Removed shell=True from subprocess commands that require user inputs #7875

Merged
merged 6 commits into from
May 19, 2022

Conversation

JWLee89
Copy link
Contributor

@JWLee89 JWLee89 commented May 18, 2022

This PR involves the following changes

  • Removal of unused arguments in functions
  • Removed shell=True from subprocess calls where user-defined inputs are provided (to prevent possible unwanted injection)

I ran the scripts after modification inside of the yolo docker container and checked manually to see if it works.
If needed I can also post serialized outputs and computational graph generated by netron.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Enhancements to model export functionality in 'ultralytics/yolov5.'

📊 Key Changes

  • Simplified function signatures by removing unnecessary model and im parameters from OpenVINO, TensorFlow GraphDef (pb), Edge TPU, and TensorFlow.js export functions.
  • Replaced subprocess.check_output and subprocess.run with shell=True to use .split() method, enhancing security and preventing shell injection vulnerabilities.

🎯 Purpose & Impact

  • 🛠 Code Simplicity: Removing unused parameters makes the codebase cleaner and easier to understand.
  • 🔒 Security Improvement: Moving away from shell=True prevents execution of arbitrary code, making exports more secure.
  • 👩‍💻 Developer Experience: These changes could improve the maintainability and contribute to the robustness of the export functionality for developers.
  • 🏃‍♂️ Operational Efficiency: Users can export models more safely and potentially with fewer errors or complications.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

👋 Hello @JWLee89, thank you for submitting a YOLOv5 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify your PR is up-to-date with upstream/master. If your PR is behind upstream/master an automatic GitHub Actions merge may be attempted by writing /rebase in a new comment, or by running the following code, replacing 'feature' with the name of your local branch:
git remote add upstream https://github.com/ultralytics/yolov5.git
git fetch upstream
# git checkout feature  # <--- replace 'feature' with local branch name
git merge upstream/master
git push -u origin -f
  • ✅ Verify all Continuous Integration (CI) checks are passing.
  • ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential. The closer to the source, the less wastage there is." -Bruce Lee

@JWLee89 JWLee89 mentioned this pull request May 18, 2022
1 task
@JWLee89 JWLee89 changed the title Removed shell=True from subprocess commands that require user inputs [WIP] Removed shell=True from subprocess commands that require user inputs May 18, 2022
@JWLee89 JWLee89 changed the title [WIP] Removed shell=True from subprocess commands that require user inputs Removed shell=True from subprocess commands that require user inputs May 18, 2022
@glenn-jocher glenn-jocher merged commit fe1b503 into ultralytics:master May 19, 2022
@glenn-jocher
Copy link
Member

@JWLee89 PR is merged. Thank you for your contributions to YOLOv5 🚀 and Vision AI ⭐

@JWLee89
Copy link
Contributor Author

JWLee89 commented May 19, 2022

@glenn-jocher Thank you for reviewing my PR.

Looking forward to working on yolov5 during my downtime!

tdhooghe pushed a commit to tdhooghe/yolov5 that referenced this pull request Jun 10, 2022
…ltralytics#7875)

* Removed shell=True from subprocess commands that require user inputs. Also removed unused arguments

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Added check=True

* Revert line add

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Glenn Jocher <[email protected]>
ctjanuhowski pushed a commit to ctjanuhowski/yolov5 that referenced this pull request Sep 8, 2022
…ltralytics#7875)

* Removed shell=True from subprocess commands that require user inputs. Also removed unused arguments

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Added check=True

* Revert line add

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Glenn Jocher <[email protected]>
@glenn-jocher
Copy link
Member

@JWLee89 you're welcome! Our team is grateful for your valuable contributions to YOLOv5. Let us know if you have any questions or need assistance during your work on the project. Happy coding!

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.

2 participants