Skip to content

Fix TypeError when using subprocess shell=True under strict Java Secu… #419

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.

Sign up for GitHub

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

Open
akspi wants to merge 1 commit into
base: master
Choose a base branch
from
+41 −1
Open

Fix TypeError when using subprocess shell=True under strict Java Secu… #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,8 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
args = _escape_args(args)

if shell:
if _shell_command is None:
raise OSError("Unable to locate or access a valid system shell to execute shell=True commands.")
args = _shell_command + args

if executable is not None:
Expand Down Expand Up @@ -1936,6 +1938,8 @@ def _os_system(command):
Execute the command (a string) in a subshell."""
args = _cmdline2listimpl(command)
args = _escape_args(args)
if _shell_command is None:
raise OSError("Unable to locate or access a valid system shell to execute system() command.")
args = _shell_command + args
cwd = os.getcwdu()

Expand Down
38 changes: 37 additions & 1 deletion Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,10 +724,46 @@ def test_invalid_args(self):
"-c", 1])


class SecurityManagerTestCase(unittest.TestCase):
Copy link
Copy Markdown
Member

@jeff5 jeff5 Apr 3, 2026

Choose a reason for hiding this comment

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

Or on second thoughts, is this maybe happier in test_subprocess_jy.py, where everything is Jython-specific?

def test_subprocess_shell_with_security_manager(self):
if not jython:
Copy link
Copy Markdown
Member

@jeff5 jeff5 Apr 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

A nicer way to do this is the decorator @unittest.skipIf(not jython, .... There's a @unittest.skipIf(jython somewhere in the file. It could be applied to the class.

Really appreciate you adding a test.

return

import java.lang.SecurityManager as SecurityManager
Copy link
Copy Markdown
Member

@Stewori Stewori Apr 1, 2026

Choose a reason for hiding this comment

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

Just a note: SecurityManager is deprecated for removal. Perhaps, it's premature to address this here, before it's actually removed. Once it's removed and the definitive JDK version of the removal is known, we can add a conditional skip.

import java.lang.System as System
import java.security.AccessControlException as AccessControlException

class MySecurityManager(SecurityManager):
def checkRead(self, file, context=None):
if file == "/bin/sh" or file == "cmd.exe" or file.endswith("sh"):
Copy link
Copy Markdown
Member

@jeff5 jeff5 Apr 3, 2026

Choose a reason for hiding this comment

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

This needs a little more thought. Ok, it's only for the test and we're not proposing a real security manager. But it fails on my Windows machine because file contains "C:\Windows\System32\cmd.exe". And what is the intent of endswith("sh") anyway? So, probably endswith("cmd.exe").

It passes on the Windows CI because we do not enable the subprocess resource. (Seems like an omission right now.)

Fixed like that, and with checkRead printing what file it has be asked about, I find that subprocess._setup_platform() involves looking for an awful lot of files.

raise AccessControlException("Access denied: " + file)
def checkPermission(self, perm, context=None):
pass

original_sm = System.getSecurityManager()
System.setSecurityManager(MySecurityManager())
try:
# We must force re-initialization of subprocess _shell_command
# because it was already initialized when the module was loaded.
saved_shell_command = subprocess._shell_command
subprocess._shell_command = None
subprocess._setup_platform()

try:
subprocess.Popen(["whoami"], shell=True)
self.fail("Expected OSError")
except OSError:
pass
except TypeError as e:
self.fail("Caught TypeError instead of OSError: " + str(e))
finally:
System.setSecurityManager(original_sm)
subprocess._shell_command = saved_shell_command

def test_main():
# Spawning many new jython processes takes a long time
test_support.requires('subprocess')
test_support.run_unittest(ProcessTestCase)
test_support.run_unittest(ProcessTestCase, SecurityManagerTestCase)
if hasattr(test_support, "reap_children"):
test_support.reap_children()

Expand Down
Loading