Fix TypeError when using subprocess shell=True under strict Java Secu…#419
Fix TypeError when using subprocess shell=True under strict Java Secu…#419akspi wants to merge 1 commit intojython:masterfrom
Conversation
| if not jython: | ||
| return | ||
|
|
||
| import java.lang.SecurityManager as SecurityManager |
There was a problem hiding this comment.
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.
|
|
||
| class SecurityManagerTestCase(unittest.TestCase): | ||
| def test_subprocess_shell_with_security_manager(self): | ||
| if not jython: |
There was a problem hiding this comment.
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.
jeff5
left a comment
There was a problem hiding this comment.
This seems like a valid bug and a sensible solution, thanks. I've only found points to criticise in the test. But thanks for adding one.
|
|
||
| class MySecurityManager(SecurityManager): | ||
| def checkRead(self, file, context=None): | ||
| if file == "/bin/sh" or file == "cmd.exe" or file.endswith("sh"): |
There was a problem hiding this comment.
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.
| "-c", 1]) | ||
|
|
||
|
|
||
| class SecurityManagerTestCase(unittest.TestCase): |
There was a problem hiding this comment.
Or on second thoughts, is this maybe happier in test_subprocess_jy.py, where everything is Jython-specific?
This change addresses the issue captured here: #418
Fixes #418