classification
Title: posixmodule expects sizeof(pid_t/gid_t/uid_t) <= sizeof(long)
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.2, Python 3.3, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: 1983 4591 Superseder:
Assigned To: Nosy List: ajaksu2, brian.curtin, christian.heimes, ezio.melotti, hynek, larry, loewis, ned.deily, petri.lehtinen, pitrou, r.david.murray, ronaldoussoren, sandro.tosi, serhiy.storchaka, sjoerd, tim.golden, trent, tzimmo, vstinner
Priority: normal Keywords: patch

Created on 2008-02-03 19:58 by christian.heimes, last changed 2013-02-14 09:19 by serhiy.storchaka.

Files
File name Uploaded Description Edit
issue2005_py3.patch tzimmo, 2012-10-23 12:40 Patching uid/gid issues in posixmodule.c review
posix_uid_gid_conv_2.patch serhiy.storchaka, 2012-11-12 15:46 review
posix_uid_gid_conv_signed_long_long.patch serhiy.storchaka, 2013-02-14 09:19 review
Messages (24)
msg62026 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-03 19:58
The posix module except that a pid_t, uid_t and gid_t always fit into a
long or can be parsed by "i". On some OSes and combination of 64bit
typess and 32bit long it's an invalid assumption.

The code should use long long where available or at least do some
overflow checks.

See r1983
msg87716 - (view) Author: Daniel Diniz (ajaksu2) (Python triager) Date: 2009-05-13 22:02
That would be issue 1983.
msg116942 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-09-20 14:18
I'm assuming that this has never been implemented, am I correct?
msg117237 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-09-23 21:55
It appears as though this was fixed as part of issue 1983.  Antoine, can you confirm that?
msg117244 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-23 22:34
The pid_t issue was fixed but not uid_t/gid_t.
msg173605 - (view) Author: Kimmo Mustonen (tzimmo) * Date: 2012-10-23 12:40
Tried to fix uid/gid issues but I didn't have a system with uid/gid larger than long so wasn't able to verify the fix.
msg173633 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-23 19:00
uid_t and gid_t converters can not be just typedefs. uid_t and gid_t can be unsigned integral types (but including -1).

Also should be fixed Modules/grpmodule.c, Modules/pwdmodule.c and Modules/signalmodule.c.
msg173694 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2012-10-24 18:33
Yeah, it seems that uid_t and gid_t can be signed or unsigned, while pid_t is always signed.

Serhiy: What do you suggest?
msg173702 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-24 20:38
I suggest use PyLong_AsLongAndOverflow/PyLong_AsLongLongAndOverflow and then PyLong_AsUnsignedLong/PyLong_AsUnsignedLongLong in case if positive overflow. Perhaps you need also special converters for "O&" format in PyArg_ParseTuple. This issue is not as easy as it seemed. ;-)
msg173703 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-24 20:41
Of course PyLong_AsUnsignedLong* should be used only if the type is unsigned. In such case any negative values except -1 are invalid.
msg174148 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2012-10-29 19:43
Checking whether a type is unsigned in configure.ac might be cumbersome...
msg174154 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-29 21:17
> Checking whether a type is unsigned in configure.ac might be cumbersome...

This can be done in compile time. Something like:

  if ((uid_t)-1 > 0) ...
msg174182 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2012-10-30 06:28
Serhiy Storchaka wrote:
> This can be done in compile time. Something like:
> 
>   if ((uid_t)-1 > 0) ...

Ah, neat. I would have expected that to issue a compiler warning,
though, because the comparison is always true if the type is unsigned,
but at least gcc doesn't seem to warn.
msg174205 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-10-30 12:58
AFAIK C89 doesn't specify integer overflows:

If an exceptional condition occurs during the evaluation of an expression (that is, if the result is not mathematically defined or not in the range of representable values for its type), the behavior is undefined.

With GCC and -fwrapv integer overflow work as you have expected. But other (and mostly older) compilers can fail.
msg174207 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2012-10-30 13:10
Christian Heimes wrote:
> AFAIK C89 doesn't specify integer overflows:

Casting a signed integer to an unsigned integer is always defined, if
that's what you're talking about.

See http://flash-gordon.me.uk/ansi.c.txt, section 3.2.1.2.

If you're talking about PyLong_AsLongWithOverflow() and friends, they
do bitshift magic to detect overflows.
msg175118 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-07 18:15
See also issue4591, issue7365, issue15301, and issue15989.

Kimmo Mustonen, if you are not working on the patch, I'll take the work.  I don't want to interrupt your work, but if you are having trouble, maybe I can do it.  In any case, I have already spent the time to study the issue and have ideas for a solution.
msg175119 - (view) Author: Kimmo Mustonen (tzimmo) * Date: 2012-11-07 18:31
Feel free to finalize this. I don't have such a system to test it anyway.
msg175180 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-08 15:58
Here is a patch that introduces four private function for convert an integer to uid_t/gid_t and back, and consistently apply these functions in all cases of uid_t/gid_t conversions.  This should fix this issue and issues 4591, 7365, 15301, and a part of 15989.  Also found and fixed some other uid_t/gid_t related bugs.

Please review and test on Windows and Macintosh.  uid_t/gid_t not used on Windows, need to check whether I right disable this part of the code.  Macintosh has the peculiarities (I found yet one), need to check whether I missed something.

If the patch is good, I will port it to 2.7 and 3.2 (there were many code changes in 3.3).
msg175447 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-11-12 14:44
I think the patch will break on Unix systems that don't have uid_t or gid_t types.
msg175448 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-11-12 14:57
The patch should work on OSX, although I haven't actually tested it yet. I've verified that sizeof(uid_t) and sizeof(gid_t) are the same for x86_64 and i386, which means SIZEOF_UID_T doesn't have to be added to pymacconfig.h.

A smal nit with the patch: it uses "long long" as a type instead of PY_LONG_LONG"
msg175456 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-12 15:46
> I think the patch will break on Unix systems that don't have uid_t or gid_t types.

I think defines to int will be used on such systems.  As I understand the configure script.

> A smal nit with the patch: it uses "long long" as a type instead of PY_LONG_LONG"

Thank you.  Fixed.
msg175834 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-17 23:56
> I think the patch will break on Unix systems that don't have uid_t or 
> gid_t types.

Do these exist? uid_t and gid_t are part of POSIX:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html
msg175835 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-11-18 00:12
> On some OSes and combination of 64bit typess and 32bit long
> it's an invalid assumption.

Can you give an example of OS where pid_t, gid_t or uid_t type are longer than the long type?

(If I remember correctly, the "identifier" of a Windows process is an addresse, so it has a size of sizeof(void*), whereas long can be smaller on 64 bit Windows. But this issue looks to be specific to UNIX.)
msg182082 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-14 09:19
Auxiliary conversion functions for uid_t and gid_t was added in issue4591. They are supports unsigned types not larger than unsigned long.

If someone need the support of the platform with signed uid_t/gid_t or with uid_t/gid_t larger than long (I don't know such modern platforms), here's the patch.
History
Date User Action Args
2013-02-14 09:19:52serhiy.storchakasetfiles: + posix_uid_gid_conv_signed_long_long.patch
2013-02-14 09:19:17serhiy.storchakasetdependencies: + 32-bits unsigned user/group identifier
messages: + msg182082
2012-11-18 00:52:56trentsetnosy: + trent
2012-11-18 00:12:16vstinnersetmessages: + msg175835
2012-11-17 23:56:40pitrousetmessages: + msg175834
2012-11-12 15:46:57serhiy.storchakasetfiles: + posix_uid_gid_conv_2.patch

messages: + msg175456
2012-11-12 15:45:17serhiy.storchakasetfiles: - posix_uid_gid_conv.patch
2012-11-12 14:57:13ronaldoussorensetmessages: + msg175448
2012-11-12 14:44:46loewissetmessages: + msg175447
2012-11-08 15:58:15serhiy.storchakasetfiles: + posix_uid_gid_conv.patch

nosy: + loewis, sjoerd, ronaldoussoren, vstinner, larry, tim.golden, ned.deily, brian.curtin, sandro.tosi, hynek
messages: + msg175180

stage: needs patch -> patch review
2012-11-07 18:31:20tzimmosetmessages: + msg175119
2012-11-07 18:15:28serhiy.storchakasetmessages: + msg175118
2012-10-30 13:10:41petri.lehtinensetmessages: + msg174207
2012-10-30 12:58:47christian.heimessetmessages: + msg174205
2012-10-30 06:28:21petri.lehtinensetmessages: + msg174182
2012-10-29 21:17:18serhiy.storchakasetmessages: + msg174154
2012-10-29 19:43:51petri.lehtinensetmessages: + msg174148
2012-10-24 21:26:00ezio.melottisetnosy: + ezio.melotti
2012-10-24 20:41:46serhiy.storchakasetkeywords: - easy

messages: + msg173703
2012-10-24 20:38:12serhiy.storchakasetmessages: + msg173702
2012-10-24 18:33:52petri.lehtinensetnosy: + petri.lehtinen
messages: + msg173694
2012-10-24 08:55:44serhiy.storchakasetversions: + Python 3.4
2012-10-24 08:55:19serhiy.storchakasetstage: needs patch
2012-10-23 19:00:58serhiy.storchakasetmessages: + msg173633
2012-10-23 12:40:54tzimmosetfiles: + issue2005_py3.patch

nosy: + tzimmo
messages: + msg173605

keywords: + patch
2012-05-28 10:48:04serhiy.storchakasetversions: + Python 3.3, - Python 3.1
2012-05-28 10:47:17serhiy.storchakasetnosy: + serhiy.storchaka
2010-09-23 22:34:17pitrousetmessages: + msg117244
2010-09-23 21:55:08r.david.murraysetnosy: + r.david.murray, pitrou, - BreamoreBoy

messages: + msg117237
versions: + Python 3.1, Python 2.7, Python 3.2, - Python 2.6, Python 3.0
2010-09-20 14:18:05BreamoreBoysetnosy: + BreamoreBoy
messages: + msg116942
2009-05-13 22:02:51ajaksu2setnosy: + ajaksu2
dependencies: + Return from fork() is pid_t, not int
messages: + msg87716
2008-02-03 19:58:43christian.heimescreate