classification
Title: PyThread_create_key(): fix comparison between signed and unsigned numbers in Python/thread_pthread.h
Type: behavior Stage: patch review
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, neologix, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2014-08-15 21:45 by haypo, last changed 2014-08-17 20:15 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
PyThread_create_key.patch haypo, 2014-08-15 21:45 review
pthread_key_create_overflow.patch haypo, 2014-08-16 12:08 review
Messages (4)
msg225367 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-08-15 21:45
The issue #22110 enabled more compiler warnings. I would like to fix this one:
---
gcc -pthread -c -Wno-unused-result -Wsign-compare -g -O0 -Wall -Wstrict-prototypes    -Werror=declaration-after-statement   -I. -IInclude -I./Include    -DPy_BUILD_CORE -o Python/thread.o Python/thread.c
In file included from Python/thread.c:86:0:
Python/thread_pthread.h: In function ‘PyThread_create_key’:
Python/thread_pthread.h:611:22: attention : signed and unsigned type in conditional expression [-Wsign-compare]
     return fail ? -1 : key;
                      ^
---

Attached patch uses Py_SAFE_DOWNCAST() to explicitly downcast to int.

On Linux (on my Fedora 20/amd64), pthread_key_t is defined as an unsigned int, whereas the result type of PyThread_create_key is a signed int.

Nobody complained before, so I get that nobody noticed the possible overflow for a key > INT_MAX. I checked the code, we only check if PyThread_create_key() returns -1, if you reach UINT_MAX keys. UINT_MAX keys sounds insane, you probably hit another limit before.

On Linux, it looks like the key is a counter and deleted values are reused:

haypo@selma$ ./python
Python 3.5.0a0 (default:a0b38f4eb79e, Aug 15 2014, 23:37:42) 
[GCC 4.8.3 20140624 (Red Hat 4.8.3-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ctypes
>>> ctypes.pythonapi.PyThread_create_key()
2
>>> ctypes.pythonapi.PyThread_create_key()
3
>>> ctypes.pythonapi.PyThread_create_key()
4
>>> ctypes.pythonapi.PyThread_delete_key(3)
0
>>> ctypes.pythonapi.PyThread_create_key()
3
msg225390 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-08-16 12:08
pthread_key_create_overflow.patch: safer patch, delete the newly created key and return an error on integer overflow.
msg225467 - (view) Author: Roundup Robot (python-dev) Date: 2014-08-17 20:14
New changeset 1b898b5d5ffe by Victor Stinner in branch 'default':
Issue #22206: Using pthread, PyThread_create_key() now sets errno to ENOMEM and
http://hg.python.org/cpython/rev/1b898b5d5ffe
msg225468 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-08-17 20:15
I fixed the issue in Python 3.5, I close the issue.

Even if Python 2.7 and 3.4 are also affected, I prefer to not modify them to not take the risk of introducing a regression for a corner case.
History
Date User Action Args
2014-08-17 20:15:23hayposetstatus: open -> closed
resolution: fixed
messages: + msg225468

versions: - Python 2.7, Python 3.4
2014-08-17 20:14:05python-devsetnosy: + python-dev
messages: + msg225467
2014-08-16 12:58:51serhiy.storchakasetnosy: + pitrou
stage: patch review
type: behavior

versions: + Python 2.7, Python 3.4
2014-08-16 12:08:16hayposetfiles: + pthread_key_create_overflow.patch

messages: + msg225390
2014-08-15 21:46:12hayposettitle: PyThread_create_key(): fix comparison between signed and unsigned numbers -> PyThread_create_key(): fix comparison between signed and unsigned numbers in Python/thread_pthread.h
2014-08-15 21:45:30haypocreate