os.fdopen() should either:
* consume file descriptor and return file/io object, or
* leave file descriptor alone and raise an exception

this invariant is broken in the following test case:
fd = os.open("/", os.O_RDONLY)
try:
    obj = os.fdopen(fd, "r")
except EnvironmentError:
    os.close(fd)  # cleanup

what happens:
fd = os.open("/", os.O_RDONLY) --> some fd
os.fdopen(fd, "r") --> exception, fd refers to a directory
os.close(fd) --> exception, no such fd


For reference:
1. invariant is held in Python 3.3.
2. following positive test case works correctly
fd = os.open("/etc/passwd", os.O_RDONLY)
try: obj = os.fdopen(fd, "r")  # success
except EnvironmentError: os.close(fd)  # not reached
3. following negative test case works correctly
fd = os.open("/etc/passwd", os.O_RDONLY)
try: obj = os.fdopen(fd, "w")  # wrong mode on purpose
except EnvironmentError: os.close(fd)  # closed ok

I think this is just won't fix. I agree the behavior in 3.x is better, but at least fdopen() is consistent about closing in 2.x, so you could work around it by dupping the fd.

Benjamin, I think you missed the key point:

file + matching mode -> fd eaten, object created
file + mode mismatch -> fd remains, exception raised
dir + matching mode -> fd eaten, exception raised

The issue is about resouce (fd) management

Thus, how can user code handle the error without either leaking file descriptor or possibly closing someone else's file descriptor?

Ah, you're right. I misread the code.

New changeset 4a3b455abf76 by Benjamin Peterson in branch '2.7':
make sure fdopen always closes the fd in error cases (closes #21191)
http://hg.python.org/cpython/rev/4a3b455abf76

I don't like proposed patch -- it changes semantics of more (?) common failure modes.

I think it's best to keep semantics in line with Python 3.3 -- if fdopen fails, it leaves file descriptor alone.

On Wed, Apr 9, 2014, at 12:48, Dima Tisnek wrote:
> 
> Dima Tisnek added the comment:
> 
> I don't like proposed patch -- it changes semantics of more (?) common
> failure modes.

I don't know if opening the file with the wrong mode is any more wrong
than opening a directory.

Good point.

Personally I think it's more pythonic to consume fd only on success. I accept that's just an opinion.

In any case, let's keep error semantics in py2.7 and py3.3 same.

On Wed, Apr 9, 2014, at 13:18, Dima Tisnek wrote:
> 
> Dima Tisnek added the comment:
> 
> Good point.
> 
> Personally I think it's more pythonic to consume fd only on success. I
> accept that's just an opinion.
> 
> In any case, let's keep error semantics in py2.7 and py3.3 same.

Unfortunately, it's not easy to implement with the 2.7 implementation of
the file type.

I should note the C level fdopen has the the 2.x semantics.

I'm not sure if you are referring to Python's C-level fdopen or GNU libc fdopen.

GNU libc fdopen does not consume file descriptor on error:

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <unistd.h>


int main(int argc, char** argv)
{
    int fd = -1;
    int rv = 0;
    FILE* fh = NULL;
    if (argc<3) return 1;

    errno = 0;
    fd = open(argv[1], O_RDONLY);
    printf("got fd %d errno %d text %s\n", fd, errno, strerror(errno));

    errno = 0;
    fh = fdopen(fd, argv[2]);
    printf("got fh %x errno %d text %s\n", fh, errno, strerror(errno));

    errno = 0;
    rv = close(fd);
    printf("got rv %d errno %d text %s\n", rv, errno, strerror(errno));
}

[dima@bmg ~]$ ./a.out /etc/passwd w
got fd 4 errno 0 text Success
got fh 0 errno 22 text Invalid argument
got rv 0 errno 0 text Success

To be fair, GNU libc fdopen doesn't consider it an error to use a file descriptor that refers to a directory, which is the crux of this bug.

Anyhow, point is the semantics change your patch brings in sets Python 2.7+ in contrast with both Python 3.x and GNU libc. 

Perhaps if it's too hard to implement properly, it's better to leave this issue as won't fix / technical limitation?

On Thu, Apr 10, 2014, at 10:46, Dima Tisnek wrote:
> 
> Dima Tisnek added the comment:
> 
> I'm not sure if you are referring to Python's C-level fdopen or GNU libc
> fdopen.
> 
> GNU libc fdopen does not consume file descriptor on error:
> 
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <string.h>
> #include <errno.h>
> #include <unistd.h>
> 
> 
> int main(int argc, char** argv)
> {
>     int fd = -1;
>     int rv = 0;
>     FILE* fh = NULL;
>     if (argc<3) return 1;
> 
>     errno = 0;
>     fd = open(argv[1], O_RDONLY);
>     printf("got fd %d errno %d text %s\n", fd, errno, strerror(errno));
> 
>     errno = 0;
>     fh = fdopen(fd, argv[2]);
>     printf("got fh %x errno %d text %s\n", fh, errno, strerror(errno));
> 
>     errno = 0;
>     rv = close(fd);
>     printf("got rv %d errno %d text %s\n", rv, errno, strerror(errno));
> }
> 
> [dima@bmg ~]$ ./a.out /etc/passwd w
> got fd 4 errno 0 text Success
> got fh 0 errno 22 text Invalid argument
> got rv 0 errno 0 text Success
> 
> To be fair, GNU libc fdopen doesn't consider it an error to use a file
> descriptor that refers to a directory, which is the crux of this bug.

I meant once you manage to get fdopen to succeed, the fd has basically
been consumed.

> 
> Anyhow, point is the semantics change your patch brings in sets Python
> 2.7+ in contrast with both Python 3.x and GNU libc. 

I realize.

> 
> Perhaps if it's too hard to implement properly, it's better to leave this
> issue as won't fix / technical limitation?

I mean if you'd prefer for it to just be inconsistent in 2.x...

I think consistency between Python versions is just as important as consistency between fd types.

Here's my hack quickfix outline:

fd = os.open(...)
try:
    if not stat.S_ISREG(os.fstat(fd).st_mode):
        raise OSError(None, "Not a regular file", ...)
    f = os.fdopen(fd, ...)
except EnvironmentError:
    os.close(fd)

Can something like this be implemented in os.py
There's already a check `if not isinstance(fd, int): raise ...`

Granted we'd have to get fd type check exactly right.
fdopen should probably succeed for regular files, pipes, char devices, block device, ptry's ...
fdopen should fail where underlying implementation fails, i.e. directories, sockets(?), epoll(?), timerfd(?)

There's a list at http://en.wikipedia.org/wiki/File_descriptor
I'm not sure about some types.

P.S. I wish there was a way to rescue fd from FILE*, but nothing like that seems to exist...

P.P.S. another option is to always use dup(), but that may break existing programs is they expect fd == fdopen(fd).fileno()

Feel free to submit a patch.

On Fri, Apr 11, 2014, at 2:40, Dima Tisnek wrote:
> 
> Dima Tisnek added the comment:
> 
> I think consistency between Python versions is just as important as
> consistency between fd types.
> 
> Here's my hack quickfix outline:
> 
> fd = os.open(...)
> try:
>     if not stat.S_ISREG(os.fstat(fd).st_mode):
>         raise OSError(None, "Not a regular file", ...)
>     f = os.fdopen(fd, ...)
> except EnvironmentError:
>     os.close(fd)
> 
> Can something like this be implemented in os.py
> There's already a check `if not isinstance(fd, int): raise ...`
> 
> Granted we'd have to get fd type check exactly right.

Well, you just have check exactly what it's checking now, which seems to
be !S_ISDIR.

> fdopen should probably succeed for regular files, pipes, char devices,
> block device, ptry's ...
> fdopen should fail where underlying implementation fails, i.e.
> directories, sockets(?), epoll(?), timerfd(?)
> 
> There's a list at http://en.wikipedia.org/wiki/File_descriptor
> I'm not sure about some types.
> 
> P.S. I wish there was a way to rescue fd from FILE*, but nothing like
> that seems to exist...

Yes, this is one of the main problems.

> 
> P.P.S. another option is to always use dup(), but that may break existing
> programs is they expect fd == fdopen(fd).fileno()

New changeset 339c79791b65 by Benjamin Peterson in branch '2.7':
when an exception is raised in fdopen, never close the fd (changing on my mind on #21191)
http://hg.python.org/cpython/rev/339c79791b65

Banjamin, Your patch looks good to me!

I have a small concern regarding "We now know we will succeed..." -- should there be a test case to make sure fstat test here matches whatever test is/was done on a lower level?

Or is your code now such that it will explicitly succeed because it doesn't call anything that can fail after the comment?

On Tue, Apr 15, 2014, at 2:34, Dima Tisnek wrote:
> 
> Dima Tisnek added the comment:
> 
> Banjamin, Your patch looks good to me!
> 
> I have a small concern regarding "We now know we will succeed..." --
> should there be a test case to make sure fstat test here matches whatever
> test is/was done on a lower level?
> 
> Or is your code now such that it will explicitly succeed because it
> doesn't call anything that can fail after the comment?

That's what I mean.