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

That would be issue 1983.

I'm assuming that this has never been implemented, am I correct?

It appears as though this was fixed as part of issue 1983.  Antoine, can you confirm that?

The pid_t issue was fixed but not uid_t/gid_t.

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.

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.

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?

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. ;-)

Of course PyLong_AsUnsignedLong* should be used only if the type is unsigned. In such case any negative values except -1 are invalid.

Checking whether a type is unsigned in configure.ac might be cumbersome...

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

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.

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.

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.

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.

Feel free to finalize this. I don't have such a system to test it anyway.

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

I think the patch will break on Unix systems that don't have uid_t or gid_t types.

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"

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

> 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

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

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.

The issue has been languishing for 8 years. I'm closing it as "fixed".