classification
Title: webbrowser.open on unix fails.
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: eric.araujo, ezio.melotti, georg.brandl, gregory.p.smith, ideasman42, neologix, pitrou, python-dev, terry.reedy
Priority: normal Keywords: patch

Created on 2011-03-07 09:30 by ideasman42, last changed 2011-03-19 05:17 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess_same_fd.diff neologix, 2011-03-08 20:17
Messages (9)
msg130245 - (view) Author: Campbell Barton (ideasman42) Date: 2011-03-07 09:30
On Linux - tested on: Arch linux @ Debian Squeeze, this fails
 python -c "__import__('webbrowser').open('http://python.org')"

The exception thats raised is:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python3.2/webbrowser.py", line 70, in open_new_tab
    return open(url, 2)
  File "/usr/lib/python3.2/webbrowser.py", line 62, in open
    if browser.open(url, new, autoraise):
  File "/usr/lib/python3.2/webbrowser.py", line 276, in open
    success = self._invoke(args, True, autoraise)
  File "/usr/lib/python3.2/webbrowser.py", line 239, in _invoke
    stderr=inout, preexec_fn=setsid)
  File "/usr/lib/python3.2/subprocess.py", line 736, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.2/subprocess.py", line 1330, in _execute_child
    raise child_exception_type(errno_num, err_msg)
OSError: [Errno 9] Bad file descriptor


Noticed that this wont raise an error when stdin arg isn't passed  to Popen:
line 237:
        p = subprocess.Popen(cmdline, close_fds=True, stdin=inout,

Removing tdin=inout makes firefox load ok, however this is there for a reason so its not a useful fix ofcourse.
msg130312 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-03-08 10:11
The problem lies here:

/* Close pipe fds. Make sure we don't close the same fd more than */
/* once, or standard fds. */
if (p2cread > 2) {
    POSIX_CALL(close(p2cread));
}
(c2pwrite > 2) {
    POSIX_CALL(close(c2pwrite));
}
if (errwrite != c2pwrite && errwrite > 2) {
    POSIX_CALL(close(errwrite));
}

If p2cread == c2pwrite (which is the case here since /dev/null is passed as stdin and stderr), we end up closing the same FD twice, hence the EBADF.
Just passing 
-(c2pwrite > 2) {
+(c2pwrite > 2 && c2pwrite != p2cread) {
    POSIX_CALL(close(c2pwrite));
}

Solves this (but you probably also want to check for (errwrite != p2cread) when closing c2pwrite).

Note that the Python implementation uses a set to avoid closing the same FD twice:

# Close pipe fds. Make sure we don't close the
# same fd more than once, or standard fds.
closed = set()
for fd in [p2cread, c2pwrite, errwrite]:
    if fd > 2 and fd not in closed:
        os.close(fd)
        closed.add(fd)

It might be cleaner to use a fd_set, i.e.:
fd_set set;
FD_ZERO(&set);
FD_SET(0, &set);
FD_SET(1, &set);
FD_SET(2, &set);
if (!FD_ISSET(p2cread, &set)) {
    POSIX_CALL(close(p2cread));
    FD_SET(p2cread, &fd);
}
if (!FD_ISSET(c2pwrite, &set)) {
    POSIX_CALL(close(c2pwrite));
    FD_SET(c2pwrite, &fd);
}
if (!FD_ISSET(errwrite, &set)) {
    POSIX_CALL(close(errwrite));
    FD_SET(errwrite, &fd);
}

But maybe it's just too much (and also, fd_set can be defined in different header files, and while I'm sure it's async-safe on Linux, I don't know if it's required as part of a standard...).
msg130367 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-03-08 20:17
Attached is a patch checking that no FD is closed more once when
closing pipe FDs, along with an update for test_subprocess.
msg130369 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-08 20:20
That probably won't make a difference here, but be aware that we switched to Mercurial and the SVN repo won't get updates anymore.
See http://docs.python.org/devguide/ and http://docs.python.org/devguide/setup.html#getting-the-source-code for details.
msg130630 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-11 22:19
Is this a webbrowser or subprocess bug?  Please add the corresponding expert from http://docs.python.org/devguide/experts to the nosy list.
msg130639 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-03-11 22:53
subprocess in 3.2 bug from the looks of it.  not sure if 2.7 or 3.1 are impacted at all, i'll remove them from the list after confirming.
msg131031 - (view) Author: Roundup Robot (python-dev) Date: 2011-03-15 19:52
New changeset 08daf3ef6509 by Gregory P. Smith in branch 'default':
Add unittests demonstrating issue #11432.
http://hg.python.org/cpython/rev/08daf3ef6509

New changeset adcf03b074b7 by Gregory P. Smith in branch 'default':
Fix issue #11432. if the stdin pipe is the same file descriptor as either stdout or stderr
http://hg.python.org/cpython/rev/adcf03b074b7

New changeset ead52adcd0c8 by Gregory P. Smith in branch '3.2':
Fix issue #11432. if the stdin pipe is the same file descriptor as either stdout or stderr
http://hg.python.org/cpython/rev/ead52adcd0c8

New changeset ad2bd8d338b0 by Gregory P. Smith in branch '3.2':
Add unittests demonstrating issue #11432.
http://hg.python.org/cpython/rev/ad2bd8d338b0
msg131033 - (view) Author: Roundup Robot (python-dev) Date: 2011-03-15 20:05
New changeset 82e010943c68 by Gregory P. Smith in branch '3.2':
issue 11432 news entry.
http://hg.python.org/cpython/rev/82e010943c68

New changeset 8d3bcf57977b by Gregory P. Smith in branch 'default':
Misc/NEWS entry for issue 11432
http://hg.python.org/cpython/rev/8d3bcf57977b
msg131390 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-03-19 05:17
With fix, test, and news in 3.2 and 3.3, is anything left to do?
History
Date User Action Args
2011-05-08 17:18:03neologixlinkissue12031 superseder
2011-03-19 05:17:12terry.reedysetnosy: + terry.reedy
messages: + msg131390
2011-03-15 20:07:42gregory.p.smithsetstatus: open -> closed
nosy: georg.brandl, gregory.p.smith, pitrou, ezio.melotti, eric.araujo, ideasman42, neologix, python-dev
resolution: fixed
versions: - Python 3.1, Python 2.7
2011-03-15 20:05:01python-devsetnosy: georg.brandl, gregory.p.smith, pitrou, ezio.melotti, eric.araujo, ideasman42, neologix, python-dev
messages: + msg131033
2011-03-15 19:52:40python-devsetnosy: + python-dev
messages: + msg131031
2011-03-11 22:53:51gregory.p.smithsetassignee: gregory.p.smith
messages: + msg130639
nosy: georg.brandl, gregory.p.smith, pitrou, ezio.melotti, eric.araujo, ideasman42, neologix
2011-03-11 22:19:42eric.araujosetnosy: + eric.araujo

messages: + msg130630
versions: + Python 3.1
2011-03-08 20:20:13pitrousetversions: + Python 2.7, Python 3.3
nosy: + pitrou

messages: + msg130369

stage: patch review
2011-03-08 20:17:19neologixsetfiles: + subprocess_same_fd.diff

messages: + msg130367
keywords: + patch
nosy: georg.brandl, gregory.p.smith, ezio.melotti, ideasman42, neologix
2011-03-08 10:11:33neologixsetnosy: + neologix
messages: + msg130312
2011-03-08 00:04:08pitrousetnosy: + gregory.p.smith
2011-03-07 09:32:12ezio.melottisetnosy: + georg.brandl, ezio.melotti
type: behavior
components: + Library (Lib), - Extension Modules
2011-03-07 09:30:26ideasman42create