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

[STINNER Victor]

STINNER Victor added the comment:

No problemo.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

Sorry about impersonating your name as committer Victor. I have been fixing 
this problem in recent patches, but because I imported your patch a while ago I 
forgot about it.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Roundup Robot added the comment:

New changeset a09ae70f3489 by Victor Stinner in branch '2.7':
Issue #22636: Avoid using a shell in the ctypes.util module
https://hg.python.org/cpython/rev/a09ae70f3489

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

ctypes_util_popen-6.py2.patch LGTM.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

Updated Py 2 patch to handle OSError when shell=True is not used

--
Added file: http://bugs.python.org/file43387/ctypes_util_popen-6.py2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Roundup Robot added the comment:

New changeset 96d297e9a8a8 by Martin Panter in branch '3.5':
Issue #22636: Handle OSError from subprocess, e.g. if command not found
https://hg.python.org/cpython/rev/96d297e9a8a8

New changeset a6a36bb6ee50 by Martin Panter in branch 'default':
Issue #22636: Merge ctypes.util from 3.5
https://hg.python.org/cpython/rev/a6a36bb6ee50

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

An Open Indiana buildbot failed. The old code let the shell print any errors 
about missing programs to /dev/null, so I will change the subprocess calls to 
handle OSError.

==
ERROR: setUpModule (ctypes.test.test_loading)
--
Traceback (most recent call last):
  File 
"/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/ctypes/test/test_loading.py",
 line 19, in setUpModule
libc_name = find_library("c")
  File 
"/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/ctypes/util.py", 
line 238, in find_library
return _get_soname(_findLib_crle(name, is64) or _findLib_gcc(name))
  File 
"/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/ctypes/util.py", 
line 145, in _get_soname
stderr=subprocess.DEVNULL)
  File 
"/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/subprocess.py", 
line 947, in __init__
restore_signals, start_new_session)
  File 
"/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/subprocess.py", 
line 1551, in _execute_child
raise child_exception_type(errno_num, err_msg)
FileNotFoundError: [Errno 2] No such file or directory: '/usr/ccs/bin/dump'

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

Updated Py 2 patch to v5 with the added GCC comment

--
Added file: http://bugs.python.org/file43383/ctypes_util_popen-5.py2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Roundup Robot]

Roundup Robot added the comment:

New changeset 0715d403cae2 by Martin Panter in branch '3.5':
Issue #22636: avoid using a shell in the ctypes.util module
https://hg.python.org/cpython/rev/0715d403cae2

New changeset 60613ecad578 by Martin Panter in branch 'default':
Issue #22636: Merge ctypes.util shell injection fixes from 3.5
https://hg.python.org/cpython/rev/60613ecad578

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

ctypes_util_popen-5.py3.patch LGTM.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

Yes a comment sounds like a good idea. Here is a new Py 3 patch.

--
Added file: http://bugs.python.org/file43329/ctypes_util_popen-5.py3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor added the comment:

Maybe the failure should be explained in a comment? (Sorry I din't read the
patch.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

Yes it is okay. The code is compiling a dummy file without main(), just to see 
what libraries GCC tries to link with it. It is only interested in extracting 
the line matching *libc.so.*, which in your case should be

/lib/i386-linux-gnu/libc.so.6

So you should find that ctypes.util._findLib_gcc("c") still returns this path, 
even though the compile command technically fails.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

It looks to me that the command used in _findLib_gcc always fails.

$ LANG=C LC_ALL=C gcc -Wl,-t -o ttt -lc
/usr/bin/ld: mode elf_i386
/usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crt1.o
/usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crti.o
/usr/lib/gcc/i686-linux-gnu/5/crtbegin.o
/lib/i386-linux-gnu/libc.so.6
(/usr/lib/i386-linux-gnu/libc_nonshared.a)elf-init.oS
/lib/i386-linux-gnu/ld-linux.so.2
/lib/i386-linux-gnu/ld-linux.so.2
-lgcc_s (/usr/lib/gcc/i686-linux-gnu/5/libgcc_s.so)
/lib/i386-linux-gnu/libc.so.6
/lib/i386-linux-gnu/ld-linux.so.2
-lgcc_s (/usr/lib/gcc/i686-linux-gnu/5/libgcc_s.so)
/usr/lib/gcc/i686-linux-gnu/5/crtend.o
/usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crtn.o
/usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crt1.o: In function 
`_start':
(.text+0x18): undefined reference to `main'
/usr/bin/ld: link errors found, deleting executable `ttt'
collect2: error: ld returned 1 exit status

Is it OK?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

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 (Issue 26439).

--
Added file: http://bugs.python.org/file43326/ctypes_util_popen-4.py2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

FTR the Python 2.3 compatibility restriction was lifted; see 
.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Changes by Martin Panter :


Added file: http://bugs.python.org/file42800/crle

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Changes by Martin Panter :


Added file: http://bugs.python.org/file42799/ldconfig

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

Uploading the fake commands I used for testing.

--
Added file: http://bugs.python.org/file42798/dump

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

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 (Issue 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.

--
Added file: http://bugs.python.org/file42797/ctypes_util_popen-3.py2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

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 Issue 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.

--
stage: needs patch -> patch review
versions:  -Python 3.4
Added file: http://bugs.python.org/file42796/ctypes_util_popen-3.py3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

There are a few review comments that probably need addressing.

--
stage: patch review -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

Marking for bug fix in 2.7, requested in Issue 25751.

--
type: enhancement -> behavior
versions: +Python 2.7, Python 3.4, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Changes by Martin Panter :


--
components: +ctypes

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Martin Panter]

Martin Panter added the comment:

See Issue 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.

--
nosy: +martin.panter
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Barry A. Warsaw]

Changes by Barry A. Warsaw :


--
nosy: +barry

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

Changes by STINNER Victor :


--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
nosy: +koobs

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor added the comment:

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?

--
Added file: http://bugs.python.org/file36946/ctypes_util_popen-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis :


--
nosy: +Arfrever

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

New submission from STINNER Victor:

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.

--
files: ctypes_util_popen.patch
keywords: patch
messages: 229363
nosy: haypo
priority: normal
severity: normal
status: open
title: avoid using a shell in ctypes.util: replace os.popen with subprocess
type: enhancement
versions: Python 3.5
Added file: http://bugs.python.org/file36923/ctypes_util_popen.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com