[issue17180] shutil copy* unsafe on POSIX - they preserve setuid/setgit bits

2013-02-11 Thread Milko Krachounov

New submission from Milko Krachounov:

When copying the mode of a file with copy, copy2, copymode, copystat or 
copytree, all permission bits are copied (including setuid and setgit), but the 
owner of the file is not. This can be used for privilege escalation.

An example:

-rwSr--r--  1 milko milko0 фев 11 10:53 test1

shutil.copy(test1, test2)

-rwSr--r--  1 root  root 0 фев 11 10:53 test2

If test1 contained anything malicious, now the user milko can execute his 
malicious payload as root.

Potential fixes:
- Strip setuid/setgid bits.
- Copy the owner on POSIX.
- Perform a safety check on the owner.
- Document the security risk.


The behaviour of copymode/copystat in this case is the same as `chmod 
--reference', and there can be some expectation of unsafety, but 
copy/copy2/copytree's behaviour differs from that of `cp -p', and this is a 
non-obvious difference.

--
components: Library (Lib)
messages: 181885
nosy: milko.krachounov
priority: normal
severity: normal
status: open
title: shutil copy* unsafe on POSIX - they preserve setuid/setgit bits
versions: Python 2.6, Python 2.7, Python 3.1, Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17180
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue8604] Adding an atomic FS write API

2011-05-13 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

 Something's missing in all the implementations presented:
 to make sure that the new version of the file is available afer 
 a crash, fsync must be called on the containing directory after 
 the rename.

I upgraded my proposed approach to include dir fsync, and to do a copy when 
backing up instead of rename (is that good?)

http://bazaar.launchpad.net/~exabyte/blackherd/async-refactor/view/66/blackherd/misc.py#L498

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8604
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue8604] Adding an atomic FS write API

2011-05-12 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

I have a class for overwriting files properly that I use in three of my 
projects. It aims to be atomic as possible, supports creating backups, but it 
doesn't have functions to set or retain permissions when requested (which might 
be desirable if such a function is added to stdlib). I'd give it here for 
reference and ideas.

- It's a context manager acting like a normal file object so you can use it 
with e.g. json.dump. In CM mode, if an error is caught, you end up with the old 
file automatically. If you use it as a file, the 'close' method has a 'cancel' 
argument to achieve the same.
- Normal overwrite on POSIX uses flush, fsync, rename as it should.
- Since fsync doesn't work on Mac OS X, it takes care of calling the Mac OS X 
specific F_FULLFSYNC fcntl.
- On POSIX, if a backup is requested, an attempt is made to do it with a 
hardlink, otherwise do two renames (killing the atomicity). Maybe a copy with 
shutil would be a better choice though.
- On Windows it uses two renames - the old file is backed up to a temporary 
name, and then the new file is renamed over it. If a backup wasn't requested, 
the temporary name is deleted.

I also have a simple unit test for it, but I've ran it on POSIX only.

Here's the part of the code that does the open/close part:
http://bazaar.launchpad.net/~exabyte/blackherd/async-refactor/view/61/blackherd/misc.py#L498

And the unit test:
http://bazaar.launchpad.net/~exabyte/blackherd/async-refactor/view/61/tests/test_misc.py#L473

I hope that's useful.

--
nosy: +milko.krachounov

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8604
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue8604] Adding an atomic FS write API

2011-05-12 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

Well, since I had a typo in the main method which called the Windows 
implementation on POSIX, the unit test works on the code for Windows when ran 
on POSIX. Heh, I'm sorry for the noise, but it seems that re-reading the code 
four times and running the unit tests is not enough, corrected revision:

http://bazaar.launchpad.net/~exabyte/blackherd/async-refactor/view/62/blackherd/misc.py#L498

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8604
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-12 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

 For the Python implementation, the GIL is not enough to
 ensure the atomicity of a process creation. That's why 
 _posixsubprocess was created. I suppose that other parts 
 of subprocess are not atomic and a lock is required to
 ensure that the creation of subprocess is atomic.
Both _posixsubprocess.fork_process and _posixsubprocess.cloexec_pipe (without 
HAVE_PIPE2) hold the GIL. They can't be running at the same time, so in regards 
to each other, they will be atomic. Or at least that's the idea. You don't need 
a separate lock when the GIL is already held. Neither a lock nor the GIL make 
the operation to be atomic (e.g. if someone forks from multiprocessing, the 
effect might be different, though os.fork() also holds the GIL and I think 
multiprocessing uses it).

The idea of _posixsubprocess is that fork() isn't thread-safe, because Python 
might call malloc() or free() in the child, which might be currently locked by 
another thread, which might cause the child to hang. Nothing to do with 
atomicity.

 Can you add a comment to explain why you can release the 
 GIL here? (because the operation is atomic)

I release the GIL because I copy-pasted the code from os.pipe(). It makes sense 
that pipe() and pipe2() are called in the same way. In the other implementation 
it is held to (ab)use it to avoid the race, but there's no need to hold it here 
because pipe2() *is* atomic.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-11 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

I attached unit tests that test that cloexec is properly set. I can't test my 
tests too well with the unpatched version because runtests.sh is too 
complicated to use, and doesn't print any useful output by default.

--
Added file: 
http://bugs.python.org/file20007/subprocess-cloexec-atomic-py3k-tests1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Removed file: 
http://bugs.python.org/file20007/subprocess-cloexec-atomic-py3k-tests1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: 
http://bugs.python.org/file20008/subprocess-cloexec-atomic-py3k-tests1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Removed file: 
http://bugs.python.org/file20008/subprocess-cloexec-atomic-py3k-tests1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: 
http://bugs.python.org/file20009/subprocess-cloexec-atomic-py3k-tests1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue6559] [PATCH]add pass_fds paramter to subprocess.Popen()

2010-12-11 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

The patch doesn't seem to work.

I added this before closerange in _close_all_but_a_sorted_few_fds:

print(Closing, start_fd, up to, fd, exclusive)

And used the attached script to run as a subprocess to check for open fds 
(taken from my tests patch for issue 7213).

Here's the result:
Python 3.2b1 (py3k:87158M, Dec 11 2010, 02:55:28) 
[GCC 4.4.5] on linux2
Type help, copyright, credits or license for more information.
 import sys
 import os
 import subprocess
 subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=False).wait()
0,1,2
0
 os.pipe()
(3, 4)
 os.pipe()
(5, 6)
 subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=False).wait()
0,1,2,3,4,5,6
0
 subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True).wait()
0,1,2
0
 subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, 
 pass_fds=(6,)).wait()
0,1,2,6
0
 subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, 
 pass_fds=(3,)).wait()
0,1,2
0
 subprocess._posixsubprocess = None
 subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, 
 pass_fds=(6,)).wait()
Closing 3 up to 6 exclusive
Closing 7 up to 8 exclusive
0,1,2,6
0
 subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, 
 pass_fds=(3,)).wait()
Closing 3 up to 8 exclusive
0,1,2
0

I also attach a possible test for pass_fds, and an example fix for Python-only 
implementation. The test requires either my tests patch for issue 7213, or the 
attached fd_status.py to be put in subprocessdata subdir of Lib/test. The fixed 
Python implementation passes my test and works fine in the console, I haven't 
tried the C one. (I don't have a patch for the fix, since it would conflict 
with the patches for issue 7213.)

--
nosy: +milko.krachounov
Added file: http://bugs.python.org/file20010/fd_status.py

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue6559
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue6559] [PATCH]add pass_fds paramter to subprocess.Popen()

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: http://bugs.python.org/file20011/test_pass_fds.py

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue6559
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue6559] [PATCH]add pass_fds paramter to subprocess.Popen()

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: 
http://bugs.python.org/file20012/subprocess-pass_fd_fix_example.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue6559
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-11 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

I add a patch that tests close_fds (there's no test for close_fds), that 
requires the tests1 patch. By the way, should there be a test for the atomicity 
of the operations?

--
Added file: 
http://bugs.python.org/file20013/subprocess-cloexec-atomic-py3k-tests2-close_fds.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

OK, I have created new updated patches. I haven't combined them in one patch 
because some of the changes can be applied independently, the three patches can 
be cat'ed together if anyone sees separate patches a problem. ;)

I. Changes:
* Now pipe2 would be used on all systems where available.
* I don't depend on external grep and cat, but use utils shipped with the tests.

II. The patches:
1. subprocess-00-subprocess_test_utils.patch
This contains the fd_status.py and input_reader.py utilities that are needed by 
the tests. A separate file because they can be used for creating unit tests for 
issue 6559. In case these patches aren't accepted, it could be useful there.

2. subprocess-01-atomic_cloexec_pipe2.patch
This file contains the changes that make the subprocess pipes created with the 
FD_CLOEXEC flag (atomically through pipe2) where possible. Can be used 
independently of the rest
of the patches.

3. subprocess-02-cloexec_tests.patch
This file contains the tests that verify that CLOEXEC is set on the pipes 
created by subprocess and this allows complex pipes between apps to work 
correctly. Requires subprocess-00-subprocess_test_utils.patch. It can be used 
for another implementation of CLOEXEC in case this implementation isn't 
accepted. With slight modification it can be used to test that pipes work fine 
with close_fds=True and no CLOEXEC.

III. Atomicity and issue 2320:

 I don't understand why do you talk about atomicity. Do you test add 
 non-atomic 
 operations? Was subprocess atomic?

 If I understood correctly, you are fixing a specific issue which can be 
 called 
 something like subprocess: close pipes on exec(), set FD_CLOEXEC flag to all 
 pipes, 
 and no more changing the default value of close_fds. Can you update the title 
 please?

The CLOEXEC flag needs to be set atomically (or at least in a way that another 
subprocess won't start in the middle of it) so that issue 2320 is also 
resolved. This is why I suggested the use of pipe2. On systems where pipe2 
isn't available, the patches rely on the coincidence that the GIL during both 
process execution and the pipe creation. I was wondering if a tests that verify 
for things like issue 2320 are wanted in general (there's no way to reliably 
test such, but there could be a test that does it with a certain probability).

In short, yes, the suggested patches attempt to solve this issue without a 
change to the default, but also to resolve issue 2320 (changing the default 
would also resolve it).

III. More issues:

1. What other possible situations exist causing the same issue that can be 
solved with close_fds=True and are caused by resources coming from other 
modules? Are any of those common? Maybe something involving os.openpty() and 
subprocess? Does anyone use those together? Are such issues worth worrying 
about (in other words, is changing the default still necessary if the 
subprocess pipes have the FD_CLOEXEC flag?)
2. What possible side-effects could the FD_CLOEXEC flag have? Any common 
use-case that is broken by the fact that the pipes are now with FD_CLOEXEC?
3. Can any changes be done to Python 2.7's subprocess? Can the issue be fixed 
there too, or it will have to remain as it is now?

--
title: Popen.subprocess change close_fds default to True - subprocess leaks 
open file descriptors between Popen instances causing hangs
Added file: 
http://bugs.python.org/file20017/subprocess-00-subprocess_test_utils.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: 
http://bugs.python.org/file20018/subprocess-01-atomic_cloexec_pipe2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: http://bugs.python.org/file20019/subprocess-02-cloexec_tests.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Removed file: http://bugs.python.org/file20019/subprocess-02-cloexec_tests.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: http://bugs.python.org/file20020/subprocess-02-cloexec_tests.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Removed file: 
http://bugs.python.org/file20005/subprocess-cloexec-atomic-py3k.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Removed file: 
http://bugs.python.org/file20009/subprocess-cloexec-atomic-py3k-tests1.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] subprocess leaks open file descriptors between Popen instances causing hangs

2010-12-11 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Removed file: 
http://bugs.python.org/file20013/subprocess-cloexec-atomic-py3k-tests2-close_fds.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-10 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

I'd offer two ideas.

1. Add a constant DISREGARD_FDS to the subprocess module could help. It would 
allow the user to specify his intent, and let the implementation choose the 
best action. Popen(..., close_fds=subprocess.DISREGARD_FDS) would mean that any 
fds additional fds are not needed, and can be closed if it is determined that 
it is the best course of action.

So, if DISREGARD_FDS gets passed on Windows assume close_fds=False, while on 
POSIX assume close_fds=True (although this has its downsides, see at the 
bottom).
2. Set the CLOEXEC flag on the other side of the pipes after the fork. With the 
attached patch, I don't get the issue I reported:

Python 3.2b1 (py3k:87158, Dec 10 2010, 20:13:57) 
[GCC 4.4.5] on linux2
Type help, copyright, credits or license for more information.
 from subprocess import *
 p1 = Popen(['cat'], stdin=PIPE, stdout=PIPE, close_fds=False)
 p2 = Popen(['grep', 'a'], stdin=p1.stdout, stdout=PIPE, close_fds=False)
 p1.stdin.write(b\n)
17
 p1.stdin.close()
 p2.stdout.read()
b'\n'
 

Additional problem with close_fds=True is that if MAXFD is huge, it can cause a 
huge problem with performance, so perhaps the default being True isn't all that 
great. On Linux you can close only the open fds, but I'm not sure if that's 
possible on other POSIX systems.

--
keywords: +patch
Added file: http://bugs.python.org/file1/subprocess-cloexec-py3k.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-10 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

The cloexec approach still doesn't help with issue 2320. In fact, with 
threading and people calling subprocess from multiple threads, *this* issue 
wouldn't be fixed with my patch either unless mutexes are used. It's impossible 
to avoid a race here with threads and no mutex as far as I can tell. It would 
mean that the creation of the pipe, the fork and the setting of the CLOEXEC 
flag needs to be done atomically... yikes.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-10 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Removed file: http://bugs.python.org/file1/subprocess-cloexec-py3k.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-10 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

 I'm +1 on it, but I think it should be the default; instead, 
 your proposed patch adds a new argument to the public API. Why do you 
 think it's necessary to do so?

I don't think it's necessary. I put it there because when I was testing I 
thought it might help. For example, you might want to keep the pipes open and 
then open another process with close_fds=False, thus changing the current 
default might cause some regressions in some software and an argument would 
allow an easier transition. I updated the patch removing anything unnecessary. 
Though it still has a huge race when used with threading.

--
Added file: http://bugs.python.org/file2/subprocess-cloexec-py3k.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-10 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

It's almost exactly the same race condition as the one described in issue 2320. 
The pipes are created and stay without the CLOEXEC flag for a while (until the 
process has been forked and fcntl has been called). During that time another 
thread can launch a subprocess, the subprocess will inherit the pipe, and there 
might be a hang similar to the one described here (or in issue 2320).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2010-12-10 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

I created another patch that attempts to create the pipes atomically.

On GNU/Linux, if pipe2 is available, it uses it to create the pipes, and there 
is no race. On other POSIX platforms, pipe and fcntl are called without 
releasing the GIL - relying on the fact that _posixsubprocess.fork_exec doesn't 
release the GIL either, so the two can't run at the same time (bonus: os.fork 
doesn't release the GIL either). I can't reproduce neither issue 7213 nor issue 
2320 with either implementation, so the patch seems to fix them.

Issues:
1. If the _posixsubprocess module isn't compiled, the race still exists (well, 
without it subprocess isn't safe to use with threads anyway).
2. On GNU/Linux systems where glibc was compiled with kernel headers =2.6.27, 
but the running kernel is 2.6.27, the subprocess module won't work. (There 
should be a fix for that?)
3. I have no way to tell that the non-Linux implementation works for sure. I've 
been running it in an endless loop, and so far there have been no hangs (*), 
but that doesn't mean that it doesn't have a rare race that's beyond my 
comprehension. With pipe2() you can be certain, but I have my doubts about the 
other implementation.

All unit tests seem to pass.

(*) Actually, I *thought* it hang on my first attempt, but I interrupted the 
process too soon to tell for sure. No hangs after that. :(

--
Added file: 
http://bugs.python.org/file20005/subprocess-cloexec-atomic-py3k.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue10549] help(cls1) breaks when cls1 has staticmethod(cls2) attribute

2010-11-27 Thread Milko Krachounov

New submission from Milko Krachounov pyt...@milko.3mhz.net:

If I make a class B, and add staticmethod(A) as an attribute when B is another 
class, help(B) breaks. The issue appears with Python 2.6.6, trunk, 3.1.3c1, and 
py3k SVN.

Python 2.7 (trunk:86836, Nov 27 2010, 18:23:07) 
[GCC 4.4.5] on linux2
Type help, copyright, credits or license for more information.
 class A(object):
... pass
... 
 class B(object):
... attr = staticmethod(A)
... 
 help(B)
Traceback (most recent call last):
  File stdin, line 1, in module
  File /home/milko/Среда/Python/trunk/Lib/site.py, line 453, in __call__
return pydoc.help(*args, **kwds)
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 1720, in __call__
self.help(request)
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 1767, in help
else: doc(request, 'Help on %s:')
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 1508, in doc
pager(render_doc(thing, title, forceload))
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 1503, in render_doc
return title % desc + '\n\n' + text.document(object, name)
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 327, in document
if inspect.isclass(object): return self.docclass(*args)
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 1216, in docclass
lambda t: t[1] == 'static method')
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 1162, in spill
name, mod, object))
  File /home/milko/Среда/Python/trunk/Lib/pydoc.py, line 327, in document
if inspect.isclass(object): return self.docclass(*args)
TypeError: docclass() takes at most 4 arguments (5 given)

Python 3.2a4+ (py3k:86836, Nov 27 2010, 18:35:01) 
[GCC 4.4.5] on linux2
Type help, copyright, credits or license for more information.
 class A:
... pass
... 
 class B:
... attr = staticmethod(A)
... 
 help(B)
Traceback (most recent call last):
  File stdin, line 1, in module
  File /home/milko/Среда/Python/py3k/Lib/site.py, line 447, in __call__
return pydoc.help(*args, **kwds)
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 1713, in __call__
self.help(request)
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 1760, in help
else: doc(request, 'Help on %s:')
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 1504, in doc
pager(render_doc(thing, title, forceload))
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 1499, in render_doc
return title % desc + '\n\n' + text.document(object, name)
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 319, in document
if inspect.isclass(object): return self.docclass(*args)
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 1214, in docclass
lambda t: t[1] == 'static method')
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 1159, in spill
name, mod, object))
  File /home/milko/Среда/Python/py3k/Lib/pydoc.py, line 319, in document
if inspect.isclass(object): return self.docclass(*args)
TypeError: docclass() takes at most 4 positional arguments (5 given)

--
components: Library (Lib)
messages: 122535
nosy: milko.krachounov
priority: normal
severity: normal
status: open
title: help(cls1) breaks when cls1 has staticmethod(cls2) attribute
versions: Python 2.6, Python 2.7, Python 3.1, Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue10549
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue6472] Inconsistent use of iterator in ElementTree doc diff between Py and C modules

2009-12-05 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

This isn't just a documentation issue. A function named getiterator(),
for which the docs say that it returns an iterator, should return an
iterator, not just an iterable. They have different semantics and can't
be used interchangeably, so the behaviour of getiterator() in
ElementTree is wrong. I was using this in my program:

iterator = element.getiterator()
next(iterator)
subelement = next(iterator)

Which broke when I tried switching to ElementTree from cElementTree,
even though the docs tell me that I'll get an iterator there.

Also, for findall() and friends, is there any reason why we can't stick
to either an iterator or list, and not both? The API will be more clear
if findall() always returned a list, or always an iterator, regardless
of the implementation. It is currently not clear what will happen if I do:

for x in tree.findall(path):
 mutate_tree(tree, x)

--
components: +Library (Lib)
nosy: +milko.krachounov
type:  - behavior
versions: +Python 2.6, Python 2.7

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue6472
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue4356] Add key argument to bisect module functions

2009-11-02 Thread Milko Krachounov

Milko Krachounov pyt...@milko.3mhz.net added the comment:

I've been bugged by the lack of key= argument for bisect for some time
now, and today I got to read this and the previous issues about the
matter. I still fail to understand the reasons for the rejections. It
might encourage bad design in which expensive key functions will get
called more than once for the same element. However:

1. Not all key= functions are computationally expensive, in fact a quick
search shows that most key= uses come are with itemgetter, attrgetter or
some cousin.
2. Even for a computationally expensive function precached key isn't
necessarily the right answer. And if the keys are huge, a key= function
can actually be useful in implementing intelligent caching.
3. The reason for rejection is merely a performance issue which *can* be
fixed without any significant changes to the design of the app should it
prove to be a real issue.


I was writing a module that keeps a sorted list of items in which items
can be added, removed, or changed (they might get reordered). I used
bisect to avoid useless slow linear searches, which would be one
obstacle for the app to scale well. However, the sort key can be changed
at any time, so implementing __lt__ wasn't an option. Keeping a second
list with the keys is both memory and computationally expensive as
inserting into a list is slow. It's not a shiny example of a bisect
use-case as the only thing I've used my module so far is to display the
files in a directory, which means the lists are small and the changes
are seldom, so any implementation is good enough.  But bisect with key=
would have been best if it was available. What's worse, I have a buggy
unreadable ad hoc implementation of bisect in my code right now. ;)

I see the following as reasons why key= would provide benefit:
1. If you have a sorted list, bisect is a net win. Having a key= would
enable you to utilize it without refactoring anything. The lack of key
may as well encourage you to continue using linear searches, or other
sub-optimal solutions.
2. Using key= is more readable and less error-prone than keeping two
lists in simple cases where defining a class with __le__ is an overkill.
Two examples I had where this was the case:
a) A class I implemented to pick numbers from a certain discrete random
distribution with bisect. Oh, but yeah, the implementation without key=
is twice faster.
b) I gave a hand to someone who was implementing a dictionary looked up
the words as you typed them with bisect.
3. Insort. Insort is already slow enough as it is, a key= wouldn't slow
it down much if at all. In fact, if you are keeping two lists, key=
would speed it up. Insort with key= is a net win.

I've implemented key= and quickly hacked a benchmark to show what
performance hit it might have, and to put things into perspective. The
results:

1. Cheap key function, integers and abs(), 100 items, 5000 bisects
or insorts:
a) Bisect with a key: 0.02205014s
b) Bisect with a second list: 0.01328707s
c) Insort with a key: 5.83688211s
d) Bisect with a second list, and two inserts: 16.17302299s

2. Cheap key function, ~4000 char bytestrings and len(), 10 items,
5000 bisects or insorts:
a) Bisect with a key: 0.01829195s
b) Bisect with a second list: 0.00945401s
c) Insort with a key: 0.25511408s
d) Bisect with a second list, and two inserts: 0.49303603s

3. Expensive key function, ~4000 char bytestrings and str.lower(),
10 (500 MB) items, 5000 bisects or insorts:
a) Bisect with a key: 1.26837015s
b) Bisect with a second list: 0.08390594s
c) Insort with a key: 1.50406289s
d) Bisect with a second list, and two inserts: 0.57737398s

4. Expensive key function, ~4000 char bytestrings and str.lower(),
50 (2.5 GB) items, 5000 bisects or insorts:
a) Bisect with a key: 1.46136308s
b) Bisect with a second list: 0.08768606s
c) Insort with a key: 3.05218720s
d) Bisect with a second list, and two inserts: 6.43227386s

5. Expensive key function, ~4000 char bytestrings and str.strip(),
10 (500 MB) items, 5000 bisects or insorts:
a) Bisect with a key: 0.03311396s
b) Bisect with a second list: 0.01374602s
c) Insort with a key: 0.27423000s
d) Bisect with a second list, and two inserts: 0.49585080s

6. Expensive key function, ~4000 char bytestrings and str.strip(),
50 (2.5 GB) items, 5000 bisects or insorts:
a) Bisect with a key: 0.04530501
b) Bisect with a second list: 0.01912594
c) Insort with a key: 1.62209797
d) Bisect with a second list, and two inserts: 5.91734695

Also, I tried to bench linear searches, but as they had to run in Python
code they aren't representative of anything. In the integer test they
went for about 250 seconds without recalculating the key, and for about
500 with. Also, replacing them with .index() gave about 60 seconds if I
ensured there's high probability that the number is in the list, and for
500 if I didn't.

In short, key= for bisect would be convenient and neat, really useful in
rare cases, leading to more

[issue4356] Add key argument to bisect module functions

2009-11-02 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: http://bugs.python.org/file15249/bisect-py3k.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4356
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue4356] Add key argument to bisect module functions

2009-11-02 Thread Milko Krachounov

Changes by Milko Krachounov pyt...@milko.3mhz.net:


Added file: http://bugs.python.org/file15250/bench_bisect_key.py

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4356
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue7213] Popen.subprocess change close_fds default to True

2009-10-26 Thread Milko Krachounov

New submission from Milko Krachounov pyt...@milko.3mhz.net:

Currently, close_fds defaults to False. The are few cases in which one
would want to leave the fds open, in all the rest leaving them open can
lead to unpleasant side effects. For example, the following doesn't work:

 p1 = Popen(['cat'], stdin=PIPE, stdout=PIPE)
 p2 = Popen(['grep', 'a'], stdin=p1.stdout, stdout=PIPE)
 p1.stdin.write(\n)
 p1.stdin.close()
 p2.stdout.read()

It would block forever, and it is not obvious that p1.stdin remains
open, because p2 was created without close_fds=True. On the other hand,
in each and every case where close_fds=True is required, the programmer
is aware that he needs to leave some fds open (and usually knows which fds).

The current default is harmful, because in each case where the file
descriptors are not explicitly needed in the child process, they can
cause problems hard to debug. It seems that only about 10% of the
current uses of Popen have close_fds=True.

I propose that the close_fds default is changed to True. Alternatively,
this could be combined with bug #6559 by completely removing the
close_fds argument, and leaving only pass_fds, which could accept
subprocess.ALL_FDS as a value.

There are two issues with my proposal:
1. close_fds would have to be changed to give a warning each time it is
not specified before the change of the default can be adopted. Otherwise
it would break any programs relying on close_fds being False by default.
2. Closing fds has a slight performance impact.

However, I think that close_fds=True is much more sensible default. I
think it will be a good idea if at least py3k adopts the change.

--
components: Library (Lib)
messages: 94538
nosy: milko.krachounov
severity: normal
status: open
title: Popen.subprocess change close_fds default to True
type: behavior
versions: Python 2.7, Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue7213
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue6973] subprocess.Popen.send_signal doesn't check whether the process has terminated

2009-09-22 Thread Milko Krachounov

New submission from Milko Krachounov pyt...@milko.3mhz.net:

When subprocess.Popen.send_signal is called, it simply calls
os.kill(self.pid, ...) without checking whether the child has already
terminated. If the child has been terminated, and Popen.wait() or
Popen.poll() have been called, a process with PID self.pid no longer
exists, and what's worse, could belong to a totally unrelated process.

A better behavior would be to raise an exception when self.returncode is
not None. Alternatively, self.pid might be set to None after the process
has been terminated, as it is no longer meaningful. Or to another value
that would be invalid pid and invalid argument to os.kill and os.wait,
but unlike None would still evaluate to True.

--
components: Library (Lib)
messages: 93022
nosy: milko.krachounov
severity: normal
status: open
title: subprocess.Popen.send_signal doesn't check whether the process has 
terminated
type: behavior
versions: Python 2.6, Python 3.1

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue6973
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com