classification
Title: AIX POLLNVAL definition causes problems
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: David.Edelsohn, christian.heimes, delhallt, haubi, haypo, python-dev, serhiy.storchaka, skrah
Priority: normal Keywords: patch

Created on 2013-05-06 15:06 by delhallt, last changed 2013-12-16 13:35 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
Python-2.7.4-pollnval.patch delhallt, 2013-05-06 15:06 accepted both signed and unsigned short for polling events
python-tip-unsigned-pollfd-events.patch haubi, 2013-11-12 14:50 [hg-tip] Treat pollfd events as unsigned short. review
poll_events_mask_overflow.patch serhiy.storchaka, 2013-12-13 10:56 review
Messages (18)
msg188558 - (view) Author: Delhallt (delhallt) Date: 2013-05-06 15:06
I encounted exactly the same issue http://bugs.python.org/issue923315 with test_asyncore, test_asynchat and test_poll.

On AIX6.1, POLLNVAL=0x8000=SHRT_MIN=SHRT_MAX+1 (on 2 bytes) and parsing events with PyArg_ParseTuple as a signed short 'h' do not work, i.e
"OverflowError: signed short integer is greater than maximum" occurs.

I changed 'h' to 'H' in the attached patch, and delete associated Overflow test.

Perhaps, they're a better way to handle that ?
msg188612 - (view) Author: David Edelsohn (David.Edelsohn) * Date: 2013-05-07 00:25
That's the way AIX allocated the bits. It's nice to check for overflow, but I think the test is imposing more semantics on the mask bits than POSIX requires. It just happens that the GLibc allocation of bits works out okay.
msg202692 - (view) Author: Michael Haubenwallner (haubi) * Date: 2013-11-12 14:50
This is a regression since 2.7.4 because of http://bugs.python.org/issue15989.

As the 'events' and 'revents' members of 'struct pollfd' both are bitfields, the question actually is why they need to be signed at all.

Additionally: I'm wondering why poll_modify() and internal_devpoll_register() haven't been updated along issue#15989, as both still have 'i' for the 'events' arg.

Attached patch (for hg-tip) changes the 'events' for 'struct pollfd' to be generally treated as unsigned short bitfield.

Not that I know of one, but the &0xffff hack may break on platforms where 'short' is not 16bits - casting to (unsigned short) instead feels more save.

Additional question: Would it make sense to have the 'BHIkK' types check for overflow?

Thank you!
msg206038 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-13 10:00
The disadvantage of 'H' is that it never raises OverflowError.

The simplest solution is just revert a part of the patch for issue15989.
msg206039 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-13 10:12
New changeset 08c95dd68cfc by Serhiy Storchaka in branch '3.3':
Issue #17919: select.poll.poll() again works with poll.POLLNVAL on AIX.
http://hg.python.org/cpython/rev/08c95dd68cfc

New changeset e78743e03c8f by Serhiy Storchaka in branch 'default':
Issue #17919: select.poll.poll() again works with poll.POLLNVAL on AIX.
http://hg.python.org/cpython/rev/e78743e03c8f

New changeset 64f32a31cd49 by Serhiy Storchaka in branch '2.7':
Issue #17919: select.poll.poll() again works with poll.POLLNVAL on AIX.
http://hg.python.org/cpython/rev/64f32a31cd49
msg206040 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 10:13
> The disadvantage of 'H' is that it never raises OverflowError.

The correct fix is to write a new parser just for this function.

See for example uint_converter() in Modules/zlibmodule.c. I would prefer to add such new parser to Python/getargs.c directly, but I was not motivated when I fixed #18294 (integer overflow in the zlib module).
msg206046 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 10:21
> The simplest solution is just revert a part of the patch for issue15989.

The revert reintroduced the integer overflow:

         self->ufds[i].events = (short)PyLong_AsLong(value);
msg206052 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-13 10:56
Here is a patch which uses special converter.
msg206054 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 11:00
+    if (uval > USHRT_MAX) {
+        PyErr_SetString(PyExc_OverflowError,
+                        "Python int too large for C unsigned int");

The message should be "C unsigned short".

The unit tests don't check that USHRT_MAX value is accepted.
msg206056 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 11:04
With  poll_events_mask_overflow.patch, the following hack can maybe be removed?

            /* The &0xffff is a workaround for AIX.  'revents'
               is a 16-bit short, and IBM assigned POLLNVAL
               to be 0x8000, so the conversion to int results
               in a negative number. See SF bug #923315. */
            num = PyLong_FromLong(self->ufds[i].revents & 0xffff);
msg206191 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-14 16:38
> The message should be "C unsigned short".

Thanks.

> The unit tests don't check that USHRT_MAX value is accepted.

And shouldn't. Not all values make sense. Meaning of USHRT_MAX is platform depended.

> With  poll_events_mask_overflow.patch, the following hack can maybe be removed?

No. Otherwise the result can be negative.
msg206194 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-14 17:19
New changeset 9bee6cc30db7 by Serhiy Storchaka in branch '2.7':
Issue #17919: Fixed integer overflow in the eventmask parameter.
http://hg.python.org/cpython/rev/9bee6cc30db7

New changeset 87bbe810e4e7 by Serhiy Storchaka in branch '3.3':
Issue #17919: Fixed integer overflow in the eventmask parameter.
http://hg.python.org/cpython/rev/87bbe810e4e7

New changeset 2fbb3c77f157 by Serhiy Storchaka in branch 'default':
Issue #17919: Fixed integer overflow in the eventmask parameter.
http://hg.python.org/cpython/rev/2fbb3c77f157
msg206201 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-14 20:08
Thank you Delhallt for your report.
msg206264 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-12-15 22:55
Hi, this happens on the OpenIndiana bot:

http://buildbot.python.org/all/builders/x86%20OpenIndiana%203.3/builds/1259/steps/test/logs/stdio

test_devpoll1 (test.test_devpoll.DevPollTests) ... ok
test_events_mask_overflow (test.test_devpoll.DevPollTests) ... ERROR
test_timeout_overflow (test.test_devpoll.DevPollTests) ... ok

======================================================================
ERROR: test_events_mask_overflow (test.test_devpoll.DevPollTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/export/home/buildbot/32bits/3.3.cea-indiana-x86/build/Lib/test/test_devpoll.py", line 96, in test_events_mask_overflow
    self.assertRaises(OverflowError, pollster.register, 0, USHRT_MAX + 1)
NameError: global name 'USHRT_MAX' is not defined

----------------------------------------------------------------------
Ran 3 tests in 0.006s

FAILED (errors=1)
test test_devpoll failed
msg206265 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-12-15 22:58
I have fixed the issue in http://hg.python.org/cpython/rev/039306b45230
msg206286 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-16 09:23
> I have fixed the issue in http://hg.python.org/cpython/rev/039306b45230

You forget 2.7 and 3.3 branches.
msg206295 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-16 12:28
New changeset c42647d76bd1 by Christian Heimes in branch '3.3':
Issue #17919: add missing import of USHRT_MAX
http://hg.python.org/cpython/rev/c42647d76bd1

New changeset 1f3f4147c35e by Christian Heimes in branch 'default':
Issue #17919: add missing import of USHRT_MAX
http://hg.python.org/cpython/rev/1f3f4147c35e
msg206308 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-16 13:35
Thank you Christian.
History
Date User Action Args
2013-12-16 13:35:56serhiy.storchakasetmessages: + msg206308
2013-12-16 12:28:59python-devsetmessages: + msg206295
2013-12-16 09:23:39hayposetmessages: + msg206286
2013-12-15 22:58:44christian.heimessetnosy: + christian.heimes
messages: + msg206265
2013-12-15 22:55:55skrahsetnosy: + skrah
messages: + msg206264
2013-12-14 20:08:37serhiy.storchakasetmessages: + msg206201
2013-12-14 20:08:00serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: resolved
2013-12-14 17:19:16python-devsetmessages: + msg206194
2013-12-14 16:38:27serhiy.storchakasetmessages: + msg206191
2013-12-13 11:04:18hayposetmessages: + msg206056
2013-12-13 11:00:32hayposetmessages: + msg206054
2013-12-13 10:56:30serhiy.storchakasetfiles: + poll_events_mask_overflow.patch

messages: + msg206052
versions: + Python 3.3
2013-12-13 10:21:23hayposetmessages: + msg206046
2013-12-13 10:13:02hayposetnosy: + haypo
messages: + msg206040
2013-12-13 10:12:08python-devsetnosy: + python-dev
messages: + msg206039
2013-12-13 10:00:33serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg206038
versions: - Python 3.2, Python 3.3, Python 3.5
2013-11-12 14:53:57hayposetnosy: + serhiy.storchaka
2013-11-12 14:50:36haubisetversions: + Python 3.2, Python 3.3, Python 3.4, Python 3.5
2013-11-12 14:50:12haubisetfiles: + python-tip-unsigned-pollfd-events.patch
nosy: + haubi
messages: + msg202692

2013-05-07 00:25:13David.Edelsohnsetmessages: + msg188612
2013-05-06 20:26:13pitrousetnosy: + David.Edelsohn
2013-05-06 15:06:57delhalltcreate