avoid using a shell in ctypes.util: replace os.popen with subprocess

[vstinner]

BPO	22636
Nosy	@warsaw, @vstinner, @vadmium, @serhiy-storchaka, @koobs
Files	ctypes_util_popen.patch ctypes_util_popen-2.patch ctypes_util_popen-3.py3.patch ctypes_util_popen-3.py2.patch dump: /usr/ccs/bin/dump ldconfig: /sbin/ldconfig -r (BSD) crle: /usr/bin/crle ctypes_util_popen-4.py2.patch ctypes_util_popen-5.py3.patch ctypes_util_popen-5.py2.patch ctypes_util_popen-6.py2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-06-18.03:19:15.452>
created_at = <Date 2014-10-14.22:32:55.866>
labels = ['ctypes', 'type-bug']
title = 'avoid using a shell in ctypes.util: replace os.popen with subprocess'
updated_at = <Date 2016-06-18.06:05:44.654>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2016-06-18.06:05:44.654>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2016-06-18.03:19:15.452>
closer = 'martin.panter'
components = ['ctypes']
creation = <Date 2014-10-14.22:32:55.866>
creator = 'vstinner'
dependencies = []
files = ['36923', '36946', '42796', '42797', '42798', '42799', '42800', '43326', '43329', '43383', '43387']
hgrepos = []
issue_num = 22636
keywords = ['patch']
message_count = 25.0
messages = ['229363', '229520', '255522', '255552', '255635', '265111', '265242', '265243', '265244', '266051', '268081', '268085', '268087', '268092', '268094', '268098', '268505', '268508', '268510', '268516', '268548', '268668', '268754', '268755', '268766']
nosy_count = 7.0
nosy_names = ['barry', 'vstinner', 'Arfrever', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'koobs']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22636'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[vstinner]

Attached patch modifies the ctypes.util module to not use a shell: it replaces os.open() with subprocess.Popen on Linux.

Running a shell is slower and is more vulnerable to code injection.

I only modified code path on Linux right now. They are still calls to os.popen() on sunos5, freebsd, openbsd and dragonfly.

[vstinner]

Updated patch which address also BSD and Solaris systems.

I also changed the behaviour when a required command is missing: return None instead of raising an OSError.

In the current code, when a command is missing, the shell scripts return the exit code 10. The Python codes checks for the exit code 10, but in fact os.popen() returns a status, not directly the exit code. So the OSError was never raised.

I don't know if it's better to return None instead of raising an error? It changes the behaviour, can it break backward compatibility?

[vadmium]

See bpo-25751 for some demo exploits on Linux, if anyone wants inspiration for test cases. Maybe this should be applied as a bug fix. I haven’t looked at the patch, other than confirming it removes all five os.popen() calls.

[vadmium]

I think it is better to return None without an exception, to keep the current behaviour, and because that’s what the documentation implies.

[vadmium]

Marking for bug fix in 2.7, requested in bpo-25751.

[vadmium]

There are a few review comments that probably need addressing.

[vadmium]

I merged Victor’s patch with the current code and addressed most of the comments:

• restore re.escape()
• single "-l" + name argument
• copy with dict(os.environ)
• redirect GCC stderr=STDOUT
• changed tempfile cleanup to try / finally

I also added a test case.

I kept Victor’s behaviour of not raising OSError when the command is missing. I think this should be considered separately, and only changed for 3.6+, if at all. The buggy code was added in bpo-4861.

I only have Linux and GCC, but I briefly tested each platform-specific branch by hacking the “if” statements and creating mock crle, ldconfig, etc commands, so I am somewhat confident that everything is still working.

[vadmium]

Here is a possible patch for Python 2. One snag is that ctypes is currently supposed to be compatible with Python 2.3, but subprocess was added in 2.4. The patch assumes it is okay to lift that compatibility restriction.

The main differences are:

• shutil.which() does not exist in Python 2. In _findLib_gcc() and the Gnu version of _get_soname(), I restored the mini shell script that runs “type” to check if commands are available. However I pass the library and file names as proper arguments, rather than inserting them into the shell syntax.

• subprocess.DEVNULL does not exist. Manually opened os.devnull where necessary.

• There was an extra Popen conversion for the Gnu “ldconfig -p” call. In Python 3, this was already converted thanks to revision 19d9f0a177de (bpo-11258).

• No context manager support for Popen objects. Instead, use communicate() where appropriate, or manually close and wait.

Again, I tested the Python 2 patch on Linux, but with mock platform-specific commands to exercise each new Popen() call.

[vadmium]

Uploading the fake commands I used for testing.

[vadmium]

FTR the Python 2.3 compatibility restriction was lifted; see <https://mail.python.org/pipermail/python-dev/2016-May/144502.html\>.

[vadmium]

Updated Python 2 patch merged with recent changes.

I will commit at least the Python 3 version soon, because the existing code sets a bad example for potential additions (bpo-26439).