[issue17976] file.write doesn't raise IOError when it should

[Jaakko Moisio]

New submission from Jaakko Moisio:

file.write doesn't sometimes raise IOError when it should, e.g. writing to 
/dev/full in line buffered mode:

jaakko@jm-laptop:~$ python
Python 2.7.5+ (2.7:a32a3b79f5e8, May 14 2013, 14:20:11) 
[GCC 4.7.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> f=open('/dev/full','w',1)
>>> f.write('hello\n')
Traceback (most recent call last):
  File "", line 1, in 
IOError: [Errno 28] No space left on device
>>> f.close()
>>> f=open('/dev/full','w',1)
>>> f.write('hello')
>>> f.write('\n')
>>> f.close()
>>> # No IOError!
... 
>>> 

The current implementation of file.write relies on comparing the return value 
of fwrite to the expected number of bytes written to detect errors. I haven't 
dug deep enough into the C standard to know for sure if fwrite return value == 
expected should always imply no error, but in my example it clearly doesn't (I 
assume my glibc and fwrite aren't broken though). However using ferror to 
detect the error works fine and IOError was raised as expected.

Python3 and io module use different implementation where this is no longer an 
issue. For us still using Python 2.7 I suggest the attached simple patch to fix 
the issue.

--
components: Interpreter Core
files: fileobject-fix.patch
keywords: patch
messages: 189226
nosy: jasujm
priority: normal
severity: normal
status: open
title: file.write doesn't raise IOError when it should
type: behavior
versions: Python 2.7
Added file: http://bugs.python.org/file30258/fileobject-fix.patch

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Charles-François Natali]

Charles-François Natali added the comment:

> I assume my glibc and fwrite aren't broken though

Actually, it's a glibc bug when the last character is a '\n':

$ python -c "f = open('/dev/full', 'w', 1); f.write('hello'); f.close()"
Traceback (most recent call last):
  File "", line 1, in 
IOError: [Errno 28] No space left on device

Normal.

Now, you add a trailing newline:
$ strace -e write python -c "f = open('/dev/full', 'w', 1); f.write('hello'); 
f.write('\n'); f.close()"
write(3, "hello\n", 6)  = -1 ENOSPC (No space left on device)

write() still returns ENOSPC, but it gets ignored by fwrite().

I've had a quick look at the source, and I think the culprit is here:
http://sourceware.org/git/?p=glibc.git;a=blob;f=libio/fileops.c#l1270

1336   if (do_write)
1337 {
1338   count = new_do_write (f, s, do_write);
1339   to_do -= count;
1340   if (count < do_write)
1341 return n - to_do;
1342 }

It looks like there's a check missing here for count < 0.

--
nosy: +neologix

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Indeed, fwrite() can return expected number of items and set errno. Here is a 
simple example on C. An output is:

setvbuf 0 0
fwrite 5 0
fwrite 1 28
fwrite 1 28

On writing "\n" fwrite returns 1 and set errno to ENOSPC.

--
nosy: +serhiy.storchaka
Added file: http://bugs.python.org/file30259/fullwrite.c

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Charles-François Natali]

Charles-François Natali added the comment:

> Indeed, fwrite() can return expected number of items and set errno. Here is a 
> simple example on C. An output is:

Yeah, who's volunteering to report it to the glibc?

That's not a python bug, but I feel bad ignoring it.

Note that ferror() isn't reliable (not on my box at least), so we have to use 
errno directly.
And of course keep the check that the value returned by fwrite matches.

--
nosy: +pitrou

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Antoine Pitrou]

Antoine Pitrou added the comment:

Yuck. We can of course workaround this (the glibc is commonly used :-)). Why is 
ferror() not reliable?

--
priority: normal -> low
stage:  -> needs patch

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Charles-François Natali]

Charles-François Natali added the comment:

> Why is ferror() not reliable?

Because the glibc doesn't check the errno return code after the
write() syscall, and thus doesn't set the file's stream error flag
(ferror() just checks this flag).

That's what I saw from the code.

I was a little surprised when Jaako says that ferror() is enough to
detect this, so I modified Serhiy code to print ferror(), and actually
ferror() reports an error for subsequent writes, not for the first one
(probably because the error goes unnoticed only when the buffer is in
a particular state).

So in short, errno is the only reliable way to check for errors :-(

--

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Jaakko Moisio]

Jaakko Moisio added the comment:

Thank you for your comments.

> I was a little surprised when Jaako says that ferror() is enough to
> detect this, so I modified Serhiy code to print ferror(), and actually
> ferror() reports an error for subsequent writes, not for the first one
> (probably because the error goes unnoticed only when the buffer is in
> a particular state).

Strange. I too modified Serchiy's code and my version of glibc (2.15) set the 
error flag at the same fwrite call as errno was set:

setvbuf 0 0 0
fwrite 5 0 0
fwrite 1 28 1
fwrite 1 28 1

(the last column being the return value of ferror after each fwrite call)

I've trusted ferror until now but I'm not an expert on the subject. It's a good 
point that errno is set by the underlying system call and is thus more 
reliable. So would checking errno be sufficient (in addition to checking that 
the lengths agree)? It can only serve to make file_write more robust.

--
Added file: http://bugs.python.org/file30260/fileobject-fix2.patch

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Charles-François Natali]

Charles-François Natali added the comment:

> Strange. I too modified Serchiy's code and my version of glibc (2.15) set the 
> error flag at the same fwrite call as errno was set:
>
> setvbuf 0 0 0
> fwrite 5 0 0
> fwrite 1 28 1
> fwrite 1 28 1
>
> (the last column being the return value of ferror after each fwrite call)

Hum, you're right, I just re-ran the test, and I'm finding the same
thing as you (I must have been dreaming).

I just re-checked the glibc code, and indeed the write() error is
checked, and the error flag is set:
http://sourceware.org/git/?p=glibc.git;a=blob;f=libio/fileops.c#l1255
1241 _IO_ssize_t
1242 _IO_new_file_write (f, data, n)
[...]
1251   count = (__builtin_expect (f->_flags2
1252  & _IO_FLAGS2_NOTCANCEL, 0)
1253? write_not_cancel (f->_fileno, data, to_do)
1254: write (f->_fileno, data, to_do));
1255   if (count < 0)
1256 {
1257   f->_flags |= _IO_ERR_SEEN;
1258   break;
1259 }

But then this value is returned, and depending on the position in the
buffer, this -1 ends up being incremented to what's left to write, and
can result in returning exactly the same size that was requested.

That's definitely a bug, and a bad one since you can get silent
corruption (fclose() won't even return an error in most cases).

Anyway, this means that ferror() should be enough - in addition to
checking that the returned value matches.

> I've trusted ferror until now but I'm not an expert on the subject. It's a 
> good point that errno is set by the underlying system call and is thus more 
> reliable. So would checking errno be sufficient (in addition to checking that 
> the lengths agree)? It can only serve to make file_write more robust.

Yeah, would you like to write a patch?

--

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Jaakko Moisio]

Jaakko Moisio added the comment:

> Yeah, would you like to write a patch?

Yes. It's fileobject-fix3.patch attached to this issue record.

--
Added file: http://bugs.python.org/file30262/fileobject-fix3.patch

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Jaakko Moisio]

Changes by Jaakko Moisio :


Added file: http://bugs.python.org/file30266/fileobject-fix4.patch

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Jaakko Moisio]

Jaakko Moisio added the comment:

I tried to reply to the review of my last patch but this tracker software 
itself crashed. Is there anyone who would be interested in the traceback?

Anyway, I'll reply here.

On 2013/05/15 08:37:29, Charles-François Natali wrote:
> http://bugs.python.org/review/17976/diff/8158/Objects/fileobject.c
> File Objects/fileobject.c (right):
> 
> http://bugs.python.org/review/17976/diff/8158/Objects/fileobject.c#newcode1856
> Objects/fileobject.c:1856: if (n2 != n || errno != 0) {
> Hum, we saw that ferror() could be used to detect the error, so it should be
> used instead.
> 
> Also, there's a problem with this patch: it's checking errno too late: for
> example, it could have been cleared by FILE_END_ALLOW_THREADS or Py_XDECREF in
> the meantime.

Ok. However I must point out that if errno is cleared in the meantime, that 
also affects PyErr_SetFromErrno. I've submitted another patch where I'm extra 
careful with errno.

--

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Jaakko Moisio]

Jaakko Moisio added the comment:

> I tried to reply to the review of my last patch but this tracker
> software itself crashed. Is there anyone who would be interested in
> the traceback?

Never mind. I found the meta tracker and posted my traceback there.

--

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[STINNER Victor]

STINNER Victor added the comment:

Attached: Test expressed as an unit test, test_dev_null.py.

The test pass with Python 3 which does not use the "FILE*" API anymore. So you 
should maybe migrate to Python 3 :-)


$ python3.4 test_dev_null.py
..
--
Ran 2 tests in 0.005s

OK


$ python2.7 test_dev_null.py
.F
==
FAIL: test_write2 (__main__.TestFull)
--
Traceback (most recent call last):
  File "test_dev_null.py", line 13, in test_write2
self.assertRaises(IOError, self.check_write, 'hello', '\n')
AssertionError: IOError not raised

--
Ran 2 tests in 0.002s

FAILED (failures=1)

--
nosy: +haypo
Added file: http://bugs.python.org/file30302/test_dev_null.py

___
Python tracker 

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

[issue17976] file.write doesn't raise IOError when it should

[Jaakko Moisio]

Jaakko Moisio added the comment:

> The test pass with Python 3 which does not use the "FILE*" API
> anymore. So you should maybe migrate to Python 3 :-)

Yes. I will eventually. But not all the libraries I'm using are migrated yet.

--
Added file: http://bugs.python.org/file30317/fileobject-fix5.patch

___
Python tracker 

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