classification
Title: PyLong_FromPid() is not correctly defined on Windows 64-bit
Type: Stage: resolved
Components: Windows Versions: Python 3.2, Python 3.3, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brian.curtin, haypo, mark.dickinson, pitrou, python-dev, sbt, serhiy.storchaka, tim.golden
Priority: normal Keywords: patch

Created on 2013-05-07 22:11 by haypo, last changed 2013-06-06 18:46 by sbt. This issue is now closed.

Files
File name Uploaded Description Edit
win_sizeof_pid_t.patch haypo, 2013-05-07 22:11 review
win_sizeof_pid_t-2.patch haypo, 2013-05-16 21:23 review
py_parse_intptr.patch sbt, 2013-06-05 15:29 review
Messages (12)
msg188688 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-05-07 22:11
The issue #1983 was not fixed on Windows: pid_t is HANDLE on Windows, which is a pointer. SIZEOF_PID_T is not defined in PC/pyconfig.h and so longobject.h takes the default implementation (use C long type):

/* Issue #1983: pid_t can be longer than a C long on some systems */
#if !defined(SIZEOF_PID_T) || SIZEOF_PID_T == SIZEOF_INT
#define _Py_PARSE_PID "i"
#define PyLong_FromPid PyLong_FromLong
#define PyLong_AsPid PyLong_AsLong
#elif SIZEOF_PID_T == SIZEOF_LONG
...

The consequence is a compiler warning:

..\Modules\posixmodule.c(6603): warning C4244: 'function' : conversion from 'Py_intptr_t' to 'long', possible loss of data [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\pythoncore.vcxproj]

It would be safer to define SIZEOF_PID_T on Windows:

#define SIZEOF_PID_T SIZEOF_VOID_P

I didn't test attached patch on Windows.

Python 3.2 is affected, but I don't think that the issue is important enough to touch this branch (which now only accept security fixes).

See also issue #17870.
msg189410 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-05-16 20:53
@Antoine (author of the commit fixing #1983): any opinion?
msg189411 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-16 20:56
Sounds fine to me, but perhaps better test the patch before committing?
(or wait for the buildbots to crash)
msg189414 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-05-16 21:23
Oh, I just noticed the following check in pyport.h:

#if SIZEOF_PID_T > SIZEOF_LONG
#   error "Python doesn't support sizeof(pid_t) > sizeof(long)"
#endif

I don't understand this test, longobject.h contains:

#elif defined(SIZEOF_LONG_LONG) && SIZEOF_PID_T == SIZEOF_LONG_LONG
#define _Py_PARSE_PID "L"
#define PyLong_FromPid PyLong_FromLongLong
#define PyLong_AsPid PyLong_AsLongLong

I suppose that this path was never tested before.

Here is a new patch removing the check from pyport.h, longobject.h already fails if SIZEOF_PID_T value is not supported:

#else
#error "sizeof(pid_t) is neither sizeof(int), sizeof(long) or sizeof(long long)"
#endif /* SIZEOF_PID_T */
msg190618 - (view) Author: Roundup Robot (python-dev) Date: 2013-06-04 21:57
New changeset 2298bcba6ec9 by Victor Stinner in branch 'default':
Close #17931: Fix PyLong_FromPid() on Windows 64-bit: processes are identified
http://hg.python.org/cpython/rev/2298bcba6ec9
msg190651 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-06-05 11:14
> pid_t is HANDLE on Windows, which is a pointer.

I think this is wrong.

The signature of getpid() is

    int _getpid(void);

so pid_t should be equivalent to int.

The complication is that the return values of spawn*() etc are process handles (cast to intptr_t), not pids:

    intptr_t _spawnv(int mode, const char *cmdname, const char *const *argv);

See

    http://msdn.microsoft.com/en-us/library/t2y34y40%28v=vs.100%29.aspx
    http://msdn.microsoft.com/en-us/library/7zt1y878%28v=vs.80%29.aspx
msg190653 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-06-05 11:49
> The complication is that the return values of spawn*() etc are process handles (cast to intptr_t), not pids:

I opened this issue because of a compiler warning in os.waitpid(). On Windows, the C function _cwait() is used and it expects a intptr_t ("handle to the process to wait on") and returns a intptr_t ("returns the handle of the specified process").... But os.waitpid() uses PyLong_FromPid() to convert the C intptr_t to a Python int, which may loose most significant bits (sizeof(void*) > sizeof(int) on Windows x64).

I see that PC/pyconfig.h defines "typedef int pid_t;". Because intptr_t is bigger than int, using intptr_t is compatible with pid_t, at least for PyLong_FromPid(). It may be a problem in the other direction: PyLong_AsPid().

Python code using Windows HANDLE is not consistent. Sometimes, intptr_t is used, sometimes long is used. Examples:

- msvcrt.open_osfhandle() parses an handle with "l" (long) format, whereas it should uses a wider type.

- msvcrt.get_osfhandle() uses PyLong_FromVoidPtr() to convert an C Py_intptr_t to a Python int, with the following comment:

    /* technically 'handle' is not a pointer, but a integer as
       large as a pointer, Python's *VoidPtr interface is the
       most appropriate here */

--

@sbt: Would you like to have a strict separation between UNIX-like pid (pid_t) and Windows process identifier (HANDLE)? 

Can we consider that HANDLE is a pointer? Or can we consider that HANDLE is intptr_t or intmax_t? See the issue #17870 which proposes an API to handle intmax_t.

If we want to be more explicit, we should add functions to handle the C HANDLE type. Example:

- PyLong_FromHandle(): C HANDLE to Python int
- PyLong_AsHandle(): Python int to C HANDLE
- "P" format (or another letter): PyArg_Parse*() format
msg190657 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-06-05 13:15
> @sbt: Would you like to have a strict separation between UNIX-like pid 
> (pid_t) and Windows process identifier (HANDLE)? 

Yes.  And would I certainly like SIZEOF_PID_T == sizeof(pid_t) ;-)

Note that _winapi takes the policy of treating HANDLE as an unsigned quantity (as PyLong_*VoidPtr() does for pointers).  I am not sure if signed or unsigned is better, but I lean towards unsigned.  It is easy enough to cast to intptr_t if we need to.

I think it is enough to treat HANDLE as void*, but adding PyLong_*Handle() is easy enough.

There does not seem to be a format character for void* (or size_t), and adding one would be useful.

Or maybe rather than adding ever more format characters which are aliases for old ones, we could just create macros like

#define PY_PARSE_INT "i"
#define PY_PARSE_UINTPTR_T "K"
#define PY_PARSE_VOID_PTR PY_PARSE_UINTPTR_T
#define PY_PARSE_HANDLE PY_PARSE_UINTPTR_T
#define PY_PARSE_PID_T PY_PARSE_INT
msg190659 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-06-05 13:22
I see _Py_PARSE_PID already exists but no others ...
msg190666 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-06-05 15:29
Attached is a patch that adds _Py_PARSE_INTPTR and _Py_PARSE_UINTPTR to Include/longobject.h.

It also uses _Py_PARSE_INTPTR in Modules/posixmodule.c and PC/msvcrtmodule.c and removes the definition for SIZEOF_PID.
msg190695 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-06-05 21:40
-/* The size of `pid_t' (HANDLE). */
-#define SIZEOF_PID_T SIZEOF_VOID_P

I would prefer to have SIZEOF_PID_T defined:
#define SIZEOF_PID_T SIZEOF_INT

win32_kill() uses DWORD type, not pid_t.

Except these nits, your patch looks good and is more correct than my patch.
msg190701 - (view) Author: Roundup Robot (python-dev) Date: 2013-06-05 22:31
New changeset 0410bf251e10 by Richard Oudkerk in branch 'default':
Issue #17931: Resolve confusion on Windows between pids and process handles.
http://hg.python.org/cpython/rev/0410bf251e10
History
Date User Action Args
2013-06-06 18:46:35sbtsetstatus: open -> closed
2013-06-05 22:31:40python-devsetmessages: + msg190701
2013-06-05 21:40:09hayposetmessages: + msg190695
2013-06-05 15:29:05sbtsetfiles: + py_parse_intptr.patch

messages: + msg190666
2013-06-05 13:22:58sbtsetmessages: + msg190659
2013-06-05 13:15:32sbtsetmessages: + msg190657
2013-06-05 11:49:56hayposetmessages: + msg190653
2013-06-05 11:14:30sbtsetstatus: closed -> open
nosy: + sbt
messages: + msg190651

2013-06-04 21:57:55python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg190618

resolution: fixed
stage: resolved
2013-05-18 11:44:19mark.dickinsonsetnosy: + mark.dickinson
2013-05-16 21:23:58hayposetfiles: + win_sizeof_pid_t-2.patch

messages: + msg189414
2013-05-16 20:56:07pitrousetmessages: + msg189411
2013-05-16 20:53:50hayposetnosy: + pitrou
messages: + msg189410
2013-05-08 00:58:27pitrousetnosy: + brian.curtin
2013-05-07 22:11:49haypocreate