Return from fork() is pid_t, not int

[stutsman]

BPO	1983
Nosy	@loewis, @pitrou, @tiran, @devdanzin
Files	issue1983.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 = None
closed_at = <Date 2009-05-23.16:22:03.371>
created_at = <Date 2008-01-31.20:07:26.991>
labels = ['type-bug', 'library']
title = 'Return from fork() is pid_t, not int'
updated_at = <Date 2009-05-23.16:22:03.370>
user = 'https://bugs.python.org/stutsman'

bugs.python.org fields:

activity = <Date 2009-05-23.16:22:03.370>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2009-05-23.16:22:03.371>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2008-01-31.20:07:26.991>
creator = 'stutsman'
dependencies = []
files = ['14016']
hgrepos = []
issue_num = 1983
keywords = ['patch']
message_count = 17.0
messages = ['61926', '61927', '61928', '61931', '61932', '61939', '61947', '61948', '61965', '61966', '61972', '61974', '87717', '88058', '88073', '88228', '88241']
nosy_count = 6.0
nosy_names = ['loewis', 'pitrou', 'christian.heimes', 'ajaksu2', 'rstutsman', 'stutsman']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1983'
versions = ['Python 2.6', 'Python 3.0', 'Python 3.1', 'Python 2.7']

[stutsman]

In current trunk (60097). Return from fork is not int but pid_t.
Treating this as an int causes total breakage on systems with 64-bit pids.

[loewis]

Can you provide a patch? It would be good if it still worked on systems
that don't have pid_t (although we could also require pid_t and then see
what systems that breaks for).

Also, what systems use 64-bit pids?

[stutsman]

Yeah; I shuold be able to provide one. I just hacked 2.4.4 to work so I
think I could provide a fix easily. The version I put together here is
rough, so I'll try to create a cleaner solution tonight or this weekend.

HiStar does (http://www.scs.stanford.edu/histar/).

Not sure if it's better to test for pid_t (in configure) or just use a
long long value all around a punt the problem? I'll probably just go
with the former unless there are objections.

[tiran]

It's probably better to use pid_t through the code and add a typedef int
pid_t for system's without pid_t. Or better typedef long pid_t?

On my Linux, pid_t is defined as __PID_T_TYPE, which is defines as
__S32_TYPE which ends up as int.

[loewis]

Don't use long long literally; not all systems have such a type, and
even if they do, they might not call it "long long".

[tiran]

I fixed the bug in r60484. Python was already using pid_t on several
occasions like getpid(). I added a test to verify that PyInt_FromLong()
can always handle pid_t.

Do you want to port the fix to 2.5?

[loewis]

Do you want to port the fix to 2.5?

I'm not quite sure that the patch actually fixes the
problem. IIUC, HiStar is available in a 32-bit version,
too, yet it may still use a 64-bit pid_t (Ryan, can
you confirm whether that's the case?).

If so, Python would now fail to compile under that
patch. Backporting a change that causes Python to fail
to compile on some systems is not a good idea.

If that aspect was fixed also (e.g. by always returning
long ints on systems where sizeof(pid_t)>sizeof(long)),
a backport would be ok. For a perfect backport, that
change might still cause a behavior change: on
a system where sizeof(pid_t)>sizeof(long), yet the
system only ever uses pid_t values < INT_MAX, people
would see that the fork return type changes unreasonably;
a perfect backport would only return longs if the values
are out of range. This is probably over-cautious, as
it's fairly unlikely that such systems actually exist.

[stutsman]

IIUC, HiStar is available in a 32-bit version,
too, yet it may still use a 64-bit pid_t (Ryan, can
you confirm whether that's the case?).

Great point. pid_t is always 64-bit on HiStar.

[stutsman]

Actually the current trunk of as of this morning (60484) is still broken
in a couple of ways. First, converting the pid_t using PyInt is a
problem and second the waitpids aren't corrected. This would cause
waits on invalid pids.

[tiran]

Ryan Stutsman wrote:

Actually the current trunk of as of this morning (60484) is still broken
in a couple of ways. First, converting the pid_t using PyInt is a
problem and second the waitpids aren't corrected. This would cause
waits on invalid pids.

I'll see what I kind do about it.

Christian

[tiran]

Martin v. Löwis wrote:

If so, Python would now fail to compile under that
patch. Backporting a change that causes Python to fail
to compile on some systems is not a good idea.

I added the size comparison to identify systems with sizeof(pid_t) >
sizeof(long).

If that aspect was fixed also (e.g. by always returning
long ints on systems where sizeof(pid_t)>sizeof(long)),
a backport would be ok. For a perfect backport, that
change might still cause a behavior change: on
a system where sizeof(pid_t)>sizeof(long), yet the
system only ever uses pid_t values < INT_MAX, people
would see that the fork return type changes unreasonably;
a perfect backport would only return longs if the values
are out of range. This is probably over-cautious, as
it's fairly unlikely that such systems actually exist.

Your proposal looks sound and good to me, but it involves some work. The
chance would require a new format operator 'p' for argument parsing and
new functions like PyInt_FromPid_t() and PyInt_AsPid_t().

In r60504 I've changed the type for the remaining functions like waitpit
and getsid to pid_t. It should make it easy to spot the lines that have
to be changed.

Christian

[loewis]

Your proposal looks sound and good to me, but it involves some work. The
chance would require a new format operator 'p' for argument parsing and
new functions like PyInt_FromPid_t() and PyInt_AsPid_t().

No, it doesn't require that. You could use conditional compilation
throughout to achieve this, and the function might be more list
int_from_pid_t (i.e. static).

[devdanzin]

Is this pending for trunk and py3k too or just for 2.5 (and hence can be
closed)?

[rstutsman]

No, I don't think this is actually fixed in any version of Python at the
moment. The title may be a bit misleading, because all the versions now
store the result of fork in a pid_t and return it as a PyLong. However,
posix_waitpid is still pulling pid's as a PyInt. Changing this to
PyLong would probably work for our purposes, but I guess the hangup was
that in reality pid_t is supposed to be an opaque datatype and
implementing it in CPython is non-trivial and has little benefit.

Perhaps we close this as won't fix or I can create a patch to at least
give a hack for 64-bit pid's but still treated as a long.

[pitrou]

The following patch should do the trick.

[loewis]

The patch looks fine to me, please apply.

[pitrou]

Thanks, the patch was committed in r72852, r72853, r72854.
I then realized a couple of functions had been looked over (including
getpid() and getppid()), and committed a complement in r72855, r72856
and r72858.