AIX POLLNVAL definition causes problems

[delhallt]

BPO	17919
Nosy	@vstinner, @tiran, @haubi, @skrah, @serhiy-storchaka
Files	Python-2.7.4-pollnval.patch: accepted both signed and unsigned short for polling events python-tip-unsigned-pollfd-events.patch: [hg-tip] Treat pollfd events as unsigned short. poll_events_mask_overflow.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2013-12-14.20:08:00.832>
created_at = <Date 2013-05-06.15:06:57.682>
labels = ['extension-modules', 'type-bug']
title = 'AIX POLLNVAL definition causes problems'
updated_at = <Date 2013-12-16.13:35:56.253>
user = 'https://bugs.python.org/delhallt'

bugs.python.org fields:

activity = <Date 2013-12-16.13:35:56.253>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2013-12-14.20:08:00.832>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2013-05-06.15:06:57.682>
creator = 'delhallt'
dependencies = []
files = ['30151', '32585', '33120']
hgrepos = []
issue_num = 17919
keywords = ['patch']
message_count = 18.0
messages = ['188558', '188612', '202692', '206038', '206039', '206040', '206046', '206052', '206054', '206056', '206191', '206194', '206201', '206264', '206265', '206286', '206295', '206308']
nosy_count = 8.0
nosy_names = ['vstinner', 'christian.heimes', 'haubi', 'skrah', 'python-dev', 'serhiy.storchaka', 'David.Edelsohn', 'delhallt']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17919'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

[delhallt]

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 ?

[DavidEdelsohn]

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.

[haubi]

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!

[serhiy-storchaka]

The disadvantage of 'H' is that it never raises OverflowError.

The simplest solution is just revert a part of the patch for bpo-15989.

[python-dev]

New changeset 08c95dd68cfc by Serhiy Storchaka in branch '3.3':
Issue bpo-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 bpo-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 bpo-17919: select.poll.poll() again works with poll.POLLNVAL on AIX.
http://hg.python.org/cpython/rev/64f32a31cd49

[vstinner]

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 bpo-18294 (integer overflow in the zlib module).

[vstinner]

The simplest solution is just revert a part of the patch for bpo-15989.

The revert reintroduced the integer overflow:

     self->ufds[i].events = (short)PyLong_AsLong(value);

[serhiy-storchaka]

Here is a patch which uses special converter.

[vstinner]

+ 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.

[vstinner]

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 bpo-923315. */
        num = PyLong_FromLong(self->ufds[i].revents & 0xffff);

[serhiy-storchaka]

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.

[python-dev]

New changeset 9bee6cc30db7 by Serhiy Storchaka in branch '2.7':
Issue bpo-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 bpo-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 bpo-17919: Fixed integer overflow in the eventmask parameter.
http://hg.python.org/cpython/rev/2fbb3c77f157

[serhiy-storchaka]

Thank you Delhallt for your report.

[skrah]

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

[tiran]

I have fixed the issue in http://hg.python.org/cpython/rev/039306b45230

[vstinner]

I have fixed the issue in http://hg.python.org/cpython/rev/039306b45230

You forget 2.7 and 3.3 branches.

[python-dev]

New changeset c42647d76bd1 by Christian Heimes in branch '3.3':
Issue bpo-17919: add missing import of USHRT_MAX
http://hg.python.org/cpython/rev/c42647d76bd1

New changeset 1f3f4147c35e by Christian Heimes in branch 'default':
Issue bpo-17919: add missing import of USHRT_MAX
http://hg.python.org/cpython/rev/1f3f4147c35e

[serhiy-storchaka]

Thank you Christian.