classification
Title: SystemError if custom opener returns -1
Type: crash Stage: resolved
Components: IO Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: barry Nosy List: barry, ppperry, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-05-19 23:50 by barry, last changed 2016-06-09 01:42 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
27066-1.patch barry, 2016-05-20 01:14 review
27066-2.patch barry, 2016-05-23 15:12 review
Messages (8)
msg265901 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2016-05-19 23:50
Let's say you use a custom opener, and that opener happens to return exactly -1.  You end up with a SystemError because NULL got returned without an exception being set:

def negative(fname, flags):
    return -1


with open('/tmp/foo.txt', 'w', encoding='utf-8', opener=negative) as fp:
    print('oops', file=fp)


% python3 /tmp/foo.py 
Traceback (most recent call last):
  File "/tmp/foo.py", line 5, in <module>
    with open('/tmp/foo.txt', 'w', encoding='utf-8', opener=negative) as fp:
SystemError: <class '_io.FileIO'> returned NULL without setting an error


Anything else and you get a relatively decent exception.  E.g. return -2 and you get an OSError.  Raise an exception and you get that exception.

The problem is pretty clear to see; when an opener is set, after coercing the fd to an integer, the check is made for that integer being -1, and then it jumps right to the exit.

Let's say you return some non-integer, like 'foo'.  Then the _PyLong_AsInt() will fail and a proper exception will be set.  So I think the "if (self->fd == -1)" clause just needs to check for an exception set first and set one if there isn't one before it does the "goto error".  I guess you'd want to see the same exception as if it returned say, -2:

Traceback (most recent call last):
  File "/tmp/foo.py", line 5, in <module>
    with open('/tmp/foo.txt', 'w', encoding='utf-8', opener=negative) as fp:
OSError: [Errno 0] Error: '/tmp/foo.txt'
msg265902 - (view) Author: (ppperry) Date: 2016-05-20 00:12
Also, `OSError [Errno 0] Error` isn't the most helpful error message.
msg265903 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2016-05-20 00:15
On May 20, 2016, at 12:12 AM, ppperry wrote:

>Also, `OSError [Errno 0] Error` isn't the most helpful error message.

No, definitely not. ;)
msg265905 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2016-05-20 01:14
Here's a proposed fix.
msg265908 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-20 04:38
Added comments on Rietveld.
msg266176 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-23 19:33
In general LGTM, but I left style comment on Rietveld. And you can use assertRaisesRegex in tests if you prefers.
msg266194 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2016-05-23 21:46
BTW, I may wait to commit this until after we've moved to GitHub. :)
msg267904 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-08 21:57
New changeset 4af64ab34eef by Barry Warsaw in branch '3.5':
Issue #27066: Fixed SystemError if a custom opener (for open()) returns
https://hg.python.org/cpython/rev/4af64ab34eef

New changeset 84c91d7d4667 by Barry Warsaw in branch 'default':
Issue #27066: Fixed SystemError if a custom opener (for open()) returns a
https://hg.python.org/cpython/rev/84c91d7d4667
History
Date User Action Args
2016-06-09 01:42:01berker.peksagsetstage: patch review -> resolved
2016-06-08 21:58:54barrysetstatus: open -> closed
resolution: fixed
2016-06-08 21:57:39python-devsetnosy: + python-dev
messages: + msg267904
2016-05-23 21:46:22barrysetmessages: + msg266194
2016-05-23 19:33:59serhiy.storchakasetmessages: + msg266176
2016-05-23 15:12:25barrysetfiles: + 27066-2.patch
2016-05-20 04:38:14serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg265908
2016-05-20 01:14:44barrysetfiles: + 27066-1.patch
messages: + msg265905

assignee: barry
keywords: + patch
stage: needs patch -> patch review
2016-05-20 00:15:03barrysetmessages: + msg265903
2016-05-20 00:12:21ppperrysetnosy: + ppperry
messages: + msg265902
2016-05-19 23:50:49barrycreate