chore: remove shell usage in shutdown subprocess #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jericht
approved these changes
Sep 27, 2023
|
Looks like DCO is failing, just needs a sign off on commit |
Signed-off-by: Josh Usiskin <[email protected]>
jusiskin
force-pushed
the
jusiskin/remove_shell_subprocess
branch
from
e2dc3bb to
f40eb9a
Compare
epmog
approved these changes
Sep 27, 2023
gmchale79
pushed a commit
that referenced
this pull request
Oct 31, 2023
Signed-off-by: Josh Usiskin <[email protected]> Signed-off-by: Graeme McHale <[email protected]>
gmchale79
pushed a commit
that referenced
this pull request
Nov 2, 2023
Signed-off-by: Josh Usiskin <[email protected]> Signed-off-by: Graeme McHale <[email protected]>
gahyusuh
pushed a commit
that referenced
this pull request
Nov 6, 2023
Signed-off-by: Josh Usiskin <[email protected]> Signed-off-by: Graeme McHale <[email protected]> Signed-off-by: Gahyun Suh <[email protected]>
gmchale79
pushed a commit
that referenced
this pull request
Feb 12, 2024
Signed-off-by: Josh Usiskin <[email protected]> Signed-off-by: Graeme McHale <[email protected]>
gmchale79
pushed a commit
that referenced
this pull request
Mar 11, 2024
Signed-off-by: Josh Usiskin <[email protected]> Signed-off-by: Graeme McHale <[email protected]>
jusiskin
pushed a commit
to jusiskin/deadline-cloud-worker-agent
that referenced
this pull request
Sep 4, 2024
Signed-off-by: Jericho Tolentino <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the problem/requirement? (What/Why)
When the worker is instructed in a
UpdateWorkerScheduleAPI response to shut down the worker host, it launches a subprocess using theshell=Trueargument tosubprocess.Popen. While the command is hard-coded and does not use user-supplied input, the worker agent code does not require the use of a shell and we should demonstrate best-practices and prevent any future possibility of unquoted shell command exploit.What was the solution? (How)
Switch from using a shell command string and
shell=Trueto instead provide a list of strings for the command andshell=False.What is the impact of this change?
The Worker Agent no longer uses a shell command-string when creating a subprocess for shutting down the host. This helps protect against a future potential regression where untrusted input is used to form the command. It also removes the unnecessary overhead for spawning a shell to run the desired command.
How was this change tested?
Updated the unit tests which now pass.
Was this change documented?
No
Is this a breaking change?
No