[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added 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.

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Dima Tisnek]

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?

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added the comment:

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()

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Dima Tisnek]

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

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added the comment:

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 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> 
> 
> 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...

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Dima Tisnek]

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 
#include 
#include 
#include 
#include 
#include 
#include 


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?

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added the comment:

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

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added the comment:

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.

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Dima Tisnek]

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.

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added the comment:

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.

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Dima Tisnek]

Dima Tisnek added the comment:

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.

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev
resolution:  -> fixed
stage:  -> committed/rejected
status: open -> closed

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added the comment:

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

--
resolution: wont fix -> 
status: closed -> open

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Dima Tisnek]

Dima Tisnek added the comment:

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?

--

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Benjamin Peterson]

Benjamin Peterson added the comment:

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.

--
nosy: +benjamin.peterson
resolution:  -> wont fix
status: open -> closed

___
Python tracker 

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

[issue21191] os.fdopen() may eat file descriptor and still raise exception

[Dima Tisnek]

New submission from Dima Tisnek:

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

--
components: IO
messages: 215832
nosy: Dima.Tisnek
priority: normal
severity: normal
status: open
title: os.fdopen() may eat file descriptor and still raise exception
type: resource usage
versions: Python 2.7

___
Python tracker 

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