The implementation of platform.architecture shells out to the file command. It tries to escape quotes by replacing " with \", but that's not sufficient.

$ python3.2 -c 'import platform; platform.architecture("foo\\\"; echo Hi there > /tmp/Z; echo \\\"")' && cat /tmp/Z
Hi there

Here's a patch to make it use subprocess instead. I haven't tested it thoroughly building everything from trunk and running tests, but I verified it works by replacing the platform.py in my system Python install.

This is security risk, but I can't think about any "platform.architecture()" use that would use a untrusted parameter. Nevertheless "os.popen()" has been deprecated for ages. I take care of this.

New changeset c73b90b6dadd by Jesus Cea in branch '2.7':
Closes #16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/c73b90b6dadd

New changeset 6c830b657900 by Jesus Cea in branch '3.2':
Closes #16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/6c830b657900

New changeset 3112bf7e0ecb by Jesus Cea in branch '3.3':
MERGE: Closes #16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/3112bf7e0ecb

New changeset cd026866b333 by Jesus Cea in branch 'default':
MERGE: Closes #16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/cd026866b333

I have a bootstrap problem with python 2.7. Backout that patch, for now.

New changeset d6d908dc11f2 by Jesus Cea in branch '2.7':
Closes #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Solve a 2.7 bootstrap issue
http://hg.python.org/cpython/rev/d6d908dc11f2

Thanks for bringing this up and for your patch.

I did a similar commit two years ago, but I reverted it because I failed to find a solution for the bootstrap issue.

changeset:   60673:7c90ac194e40
branch:      legacy-trunk
parent:      60664:d7d5c76545fb
user:        Victor Stinner <victor.stinner@haypocalc.com>
date:        Sun Apr 18 09:07:49 2010 +0000
files:       Lib/platform.py
description:
platform: use subprocess.Popen() instead of os.popen() in _syscmd_file()

 * Popen() avoids ugly shell escape: target.replace('"', '\\"')
 * Use proc.communicate() instead of f.stdout.read()
 * Get output from stdout by splitting with ": " instead of splitting by spaces
   to support filename with spaces

See also issue #9560 and the following email from Marc-Andre Lemburg.
http://mail.python.org/pipermail/python-checkins/2010-April/092099.html

"Viktor, before making such changes to platform.py, please coordinate
with me, as I am the maintainer of that module.

The subprocess change is not OK, since platform.py has to stay
compatible with Python 2.3 which doesn't have subprocess. Please either
revert the change or make it fallback to os.popen()."

Thanks for the heads-up, Victor.

I have added Marc-Andre Lemburg to the nosy list, so he can know about this issue and can provide feedback (or request a backout for 2.7).

Marc-Andre?.

This change broke test_platform for me:

======================================================================
ERROR: test_architecture (test.test_platform.PlatformTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/cpython/32/Lib/test/test_platform.py", line 11, in test_architecture
    res = platform.architecture()
  File "/home/antoine/cpython/32/Lib/platform.py", line 1072, in architecture
    if 'executable' not in fileout:
TypeError: Type str doesn't support the buffer API

Reopen: test_platform is failing. Attached patch should fix the issue. It fixes a theoric "deadlock" issue in _syscmd_file: use proc.communicate() instead of proc.stdout.read().

Well, the theoretical deadlock's just if stdin is also a pipe, right? I think there isn't be a difference between communicate and stdout.read if only stdout is a pipe. Though it might be worth passing DEVNULL to stdin instead of inheriting, just to be tidy.

> Well, the theoretical deadlock's just if stdin is also a pipe, right?

If I remember correctly, the parent process (Python) and the child
process (file) can be blocked if the child is blocked in a blocking
write into a pipe (ex: stderr), whereas the pipe buffer is full (the
size of the buffer is usually 4096 bytes) and the parent is reading
all content of another pipe (ex: stdout). So it occurs if the process
uses more than 1 pipe for standard input/output streams. In the case
of platform, it looks like the issue cannot occur.

I always prefer communicate() because I consider it safer :-)

> Though it might be worth passing DEVNULL to stdin instead of inheriting, just to be tidy.

Yeah, maybe. As you want.

Victor: your patch doesn't apply on 3.2, but it works for me on 3.3.

Victor is right, communicate() is usually the right way to do it. No need to use unsafe idioms.

> Victor: your patch doesn't apply on 3.2, but it works for me on 3.3.

I wrote it for Python 3.4. I'm too lazy for adapt it to other branches.

Yes, communicate is needed if you have multiple pipes and need to be careful about both ends doing a blocking reads/writes on different ones. There's only one pipe here. Eh, whatever. If you guys really want to use communicate, I don't really care.

New changeset b94a9ff13199 by Jesus Cea in branch '2.7':
#16112: platform.architecture does not correctly escape argument to /usr/bin/file. Use 'communicate()'
http://hg.python.org/cpython/rev/b94a9ff13199

New changeset 04f39958aea9 by Jesus Cea in branch '3.2':
#16112: platform.architecture does not correctly escape argument to /usr/bin/file. Use 'communicate()' and decode the bytes
http://hg.python.org/cpython/rev/04f39958aea9

New changeset 19d37c8d1882 by Jesus Cea in branch '2.7':
#16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch
http://hg.python.org/cpython/rev/19d37c8d1882

New changeset 9838ae397a19 by Jesus Cea in branch '3.2':
#16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch
http://hg.python.org/cpython/rev/9838ae397a19

New changeset 64a0caf49429 by Jesus Cea in branch '3.3':
MERGE: #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch
http://hg.python.org/cpython/rev/64a0caf49429

New changeset b9ac3c44a4eb by Jesus Cea in branch 'default':
MERGE: #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch
http://hg.python.org/cpython/rev/b9ac3c44a4eb

David, after so many patches, can you confirm that we have solved the original issue? :)

Why I was not seeing "test_platform.py" failing in the buildbots? :-?

Jesús Cea Avión wrote:
> 
> Jesús Cea Avión added the comment:
> 
> Thanks for the heads-up, Victor.
> 
> I have added Marc-Andre Lemburg to the nosy list, so he can know about this issue and can provide feedback (or request a backout for 2.7).
> 
> Marc-Andre?.

The comment that Viktor posted still stands for Python 2.7.

You can use subprocess in platform for Python 2.7, but only if
it's available. Otherwise the module must fall back to the
portable popen() that comes with the platform module.

It may be worth adding that selection process to the popen()
function in platform itself.

For Python 3.x, you can use subprocess.

Thanks,
-- 
Marc-Andre Lemburg
eGenix.com

Professional Python Services directly from the Source  (#1, Oct 05 2012)
>>> Python Projects, Consulting and Support ...   http://www.egenix.com/
>>> mxODBC.Zope/Plone.Database.Adapter ...       http://zope.egenix.com/
>>> mxODBC, mxDateTime, mxTextTools ...        http://python.egenix.com/
________________________________________________________________________
2012-09-27: Released eGenix PyRun 1.1.0 ...       http://egenix.com/go35
2012-09-26: Released mxODBC.Connect 2.0.1 ...     http://egenix.com/go34
2012-09-25: Released mxODBC 3.2.1 ...             http://egenix.com/go33
2012-10-23: Python Meeting Duesseldorf ...                 18 days to go

   eGenix.com Software, Skills and Services GmbH  Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
           Registered at Amtsgericht Duesseldorf: HRB 46611
               http://www.egenix.com/company/contact/

> The implementation of platform.architecture shells out to the file command. It tries to escape quotes by replacing " with \", but that's not sufficient.
> 
> $ python3.2 -c 'import platform; platform.architecture("foo\\\"; echo Hi there > /tmp/Z; echo \\\"")' && cat /tmp/Z
> Hi there
> 
> Here's a patch to make it use subprocess instead. I haven't tested it thoroughly building everything from trunk and running tests, but I verified it works by replacing the platform.py in my system Python install.

I think a much better patch would be to test for existence of the
file in question. File names rarely use any of the mentioned quoting
and most certainly not for an executable, so if the check fails, that's
a good indication that something is not right.

Perhaps such a check could be added in addition to the other things
in the patch ?

BTW: It's probably better to discuss such patches on the tracker first,
before applying them to the code base. It becomes difficult discussing
patches that have already been partially applied to the code.

-- 
Marc-Andre Lemburg
eGenix.com

Professional Python Services directly from the Source  (#1, Oct 05 2012)
>>> Python Projects, Consulting and Support ...   http://www.egenix.com/
>>> mxODBC.Zope/Plone.Database.Adapter ...       http://zope.egenix.com/
>>> mxODBC, mxDateTime, mxTextTools ...        http://python.egenix.com/
________________________________________________________________________
2012-09-27: Released eGenix PyRun 1.1.0 ...       http://egenix.com/go35
2012-09-26: Released mxODBC.Connect 2.0.1 ...     http://egenix.com/go34
2012-09-25: Released mxODBC 3.2.1 ...             http://egenix.com/go33
2012-10-23: Python Meeting Duesseldorf ...                 18 days to go

   eGenix.com Software, Skills and Services GmbH  Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
           Registered at Amtsgericht Duesseldorf: HRB 46611
               http://www.egenix.com/company/contact/

1.7 -        with open(DEV_NULL) as dev_null:
     1.8 -            proc = subprocess.Popen(['file', '-b', '--', target],
     1.9 -                                    stdout=subprocess.PIPE,
stderr=dev_null)
     1.9 +        proc = subprocess.Popen(['file', target],
    1.10 +                stdout=subprocess.PIPE, stderr=subprocess.STDOUT)

Errors should be ignored, not written into stderr. subprocess.DEVNULL
was added to Python 3.3, in older version you have to manually call
open the DEV_NULL file.

(Oh by the way, it should be opened in write mode for stderr, not in
read mode!?)

> You can use subprocess in platform for Python 2.7, but only if
> it's available. Otherwise the module must fall back to the
> portable popen() that comes with the platform module.

Marc-Andre: I still don't understand why you want to run platform.py
of Python 2.7 on older Python version? How does it happen? I expected
Python 2.3 to use its own version of platform.py.

> BTW: It's probably better to discuss such patches on the tracker first,
> before applying them to the code base. It becomes difficult discussing
> patches that have already been partially applied to the code.

Agreed with Marc-André. Jesus, please understand that by committing bogus code, you are annoying every other core developer. Be careful and run the test suite before pushing any new code. And waiting for reviews doesn't hurt.

Antoine, I agree. I beg your pardon. This patch was suppose to be quite trivial, and "test_platform" passes just fine on my Linux and Solaris computers. And the four buildbots I was monitoring, the testsuite passed.

The suggestion of Marc-Andre of simply adding an "os.access()" to check for file existence first is quite sensible and would be enough and compatible in all platforms and python releases.

I am for backing out my patches so far and push a simple "os.access()" check. What do you think?.

Marc-Andre, I was wondering if you could elaborate a bit why 2.7 platform.py file should be able to run fine in 2.3 :-?

Jesús Cea Avión wrote:
> 
> Antoine, I agree. I beg your pardon. This patch was suppose to be quite trivial, and "test_platform" passes just fine on my Linux and Solaris computers. And the four buildbots I was monitoring, the testsuite passed.
> 
> The suggestion of Marc-Andre of simply adding an "os.access()" to check for file existence first is quite sensible and would be enough and compatible in all platforms and python releases.
> 
> I am for backing out my patches so far and push a simple "os.access()" check. What do you think?.

At least for Python 2.7, I think that would be a nice and simple
solution.

> Marc-Andre, I was wondering if you could elaborate a bit why 2.7 platform.py file should be able to run fine in 2.3 :-?

That only applies to the version in Python 2.x. Python 2.3 is perhaps
a bit extreme and I'd be fine with bumping it to 2.4.

The main reason for keeping the compatibility is that the module is
also being used outside the stdlib for Python versions starting from
2.4 and later. I don't want to maintain two separate versions.

-- 
Marc-Andre Lemburg
eGenix.com

Professional Python Services directly from the Source  (#1, Oct 06 2012)
>>> Python Projects, Consulting and Support ...   http://www.egenix.com/
>>> mxODBC.Zope/Plone.Database.Adapter ...       http://zope.egenix.com/
>>> mxODBC, mxDateTime, mxTextTools ...        http://python.egenix.com/
________________________________________________________________________
2012-09-27: Released eGenix PyRun 1.1.0 ...       http://egenix.com/go35
2012-09-26: Released mxODBC.Connect 2.0.1 ...     http://egenix.com/go34
2012-09-25: Released mxODBC 3.2.1 ...             http://egenix.com/go33
2012-10-23: Python Meeting Duesseldorf ...                 17 days to go

   eGenix.com Software, Skills and Services GmbH  Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
           Registered at Amtsgericht Duesseldorf: HRB 46611
               http://www.egenix.com/company/contact/

> The main reason for keeping the compatibility is that the module is
> also being used outside the stdlib for Python versions starting from
> 2.4 and later. I don't want to maintain two separate versions.

Which projects use their own copy of platform.py? Where does this file come from?

Using subprocess sounds safer than adding a (theorical) TOCTTOU issue:
http://en.wikipedia.org/wiki/Time_of_check_to_time_of_use

If you are concerned about compatibility with Python 2.4, you should maintain platform outside the stdlib. The stdlib is regulary updated to the most recent syntax/modules. For example, "yield from" is now used in the stdlib of Python 3.4.

STINNER Victor wrote:
> 
> STINNER Victor added the comment:
> 
>> The main reason for keeping the compatibility is that the module is
>> also being used outside the stdlib for Python versions starting from
>> 2.4 and later. I don't want to maintain two separate versions.
> 
> Which projects use their own copy of platform.py? Where does this file come from?

It's originally from the mxCGIPython project I ran a few years ago
and which now lives on in form of the eGenix PyRun project. Since
the main purpose of platform.py is to identify the underlying Python
runtime and OS platform across multiple Python runtimes and OS platforms,
it's meant to always be used in latest version, even against older Python
versions.

> Using subprocess sounds safer than adding a (theorical) TOCTTOU issue:
> http://en.wikipedia.org/wiki/Time_of_check_to_time_of_use
> 
> If you are concerned about compatibility with Python 2.4, you should maintain platform outside the stdlib. The stdlib is regulary updated to the most recent syntax/modules. For example, "yield from" is now used in the stdlib of Python 3.4.

The Python 2.4 compatibility requirement only applies to Python 2.x,
not Python 3.x.

Since Python 2.4 does ship with subprocess, I guess we could use
subprocess even in Python 2.7, but I'm not sure whether it's a good
idea to change the code in such a major way for a patch level release.

-- 
Marc-Andre Lemburg
eGenix.com

Professional Python Services directly from the Source  (#1, Oct 18 2012)
>>> Python Projects, Consulting and Support ...   http://www.egenix.com/
>>> mxODBC.Zope/Plone.Database.Adapter ...       http://zope.egenix.com/
>>> mxODBC, mxDateTime, mxTextTools ...        http://python.egenix.com/
________________________________________________________________________
2012-09-27: Released eGenix PyRun 1.1.0 ...       http://egenix.com/go35
2012-09-26: Released mxODBC.Connect 2.0.1 ...     http://egenix.com/go34
2012-09-25: Released mxODBC 3.2.1 ...             http://egenix.com/go33
2012-10-23: Python Meeting Duesseldorf ...                  5 days to go

   eGenix.com Software, Skills and Services GmbH  Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
           Registered at Amtsgericht Duesseldorf: HRB 46611
               http://www.egenix.com/company/contact/

I don't see why backwards compatibility with ancient, unsupported Python versions should be more important than security. Let's use the subprocess module here.

MAL, if you need platform.py for some old projects, please vendor it and use a copy.

Christian: would you mind to review my old patch (and rebase it if needed)?

Shouldn't this ticket be closed ?

platform is using subprocess on both the 2.7 and trunk, so the issue should be fixed - and indeed I cannot reproduce it anymore.

Oh right, I didn't read the history of the issue. _syscmd_file() doesn't use popen() anymore, but subprocess, so the issue can now be closed.

Note: Other platform functions still use os.popen(), you might open new issues.

_syscmd_ver:

    for cmd in ('ver', 'command /c ver', 'cmd /c ver'):
        try:
            pipe = os.popen(cmd)
    ...

def _syscmd_uname(option, default=''):
    ...
        f = os.popen('uname %s 2> %s' % (option, DEV_NULL))

Note: there is also platform.popen() which is deprecated since Python 3.3, we might also remove it from Python 3.7 as well (I think that it's now too late for Python 3.6).