Repository navigation
Fix function cast warning in thread_pthread.h #77196
Description
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 2018-11-30.23:11:09.461>
created_at = <Date 2018-03-06.18:56:13.815>
labels = ['3.8', 'build', '3.7']
title = 'Fix function cast warning in thread_pthread.h'
updated_at = <Date 2018-11-30.23:11:09.460>
user = 'https://github.com/siddhesh'bugs.python.org fields:
activity = <Date 2018-11-30.23:11:09.460>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-11-30.23:11:09.461>
closer = 'vstinner'
components = ['Build']
creation = <Date 2018-03-06.18:56:13.815>
creator = 'siddhesh'
dependencies = []
files = ['47888']
hgrepos = []
issue_num = 33015
keywords = ['patch']
message_count = 32.0
messages = ['313357', '316332', '316348', '316415', '316639', '316709', '328298', '328300', '328301', '328302', '328308', '328310', '328313', '328325', '328387', '328388', '328409', '328415', '328417', '328838', '330791', '330794', '330798', '330799', '330800', '330806', '330807', '330810', '330829', '330830', '330832', '330835']
nosy_count = 10.0
nosy_names = ['gregory.p.smith', 'pitrou', 'vstinner', 'benjamin.peterson', 'skrah', 'serhiy.storchaka', 'steve.dower', 'siddhesh', 'izbyshev', 'miss-islington']
pr_nums = ['6008', '10057', '10821', '10822', '10823', '10828', '10829']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue33015'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']
Activity
siddhesh commentedon Mar 6, 2018
The PyThread_start_new_thread function takes a void ()(void *) as the function argument, which does not match with the pthread_create callback which has type void *()(void *). I've got a fix for this that adds a wrapper function of the right type that subsequently calls the function passed to PyThread_start_new_thread.
PR coming up.
zooba commentedon May 9, 2018
Can't we just fix the cast? We shouldn't have to do heap allocations for this.
siddhesh commentedon May 10, 2018
gcc8.1 throws this warning irrespective of the cast since converting function pointers may result in undefined behaviour unless it is cast back to its original type.
zooba commentedon May 11, 2018
In this case, we know the behaviour is okay and the warning is wrong. We should suppress the warning around the offending line, rather than adding significant code that may introduce genuine bugs.
siddhesh commentedon May 15, 2018
Actually it is not; the parameter passed to Pythread_start_new_thread has a different type (void ()(void *)) from what's accepted (and executed by) pthread_create (void *()(void *)). That is undefined behaviour.
zooba commentedon May 15, 2018
Ah okay, fair enough. Knowing that, I'll take another look at the PR. I'd really like to simplify this as much as possible to avoid risk of regression.
vstinner commentedon Oct 23, 2018
Do you have a reference explaining this issue? I dislike adding a memory allocation to call a function just to fix a compiler warning. From my point of view, at the end, the function pointer is just an integer. I don't understand how an additional memory allocation solves the issue.
vstinner commentedon Oct 23, 2018
"Fix function cast warning in thread_pthread.h"
Can you please paste the warning?
vstinner commentedon Oct 23, 2018
func_cast.c: C program reproducing the issue. Using an additional (void*) cast, it's possible to workaround the cast warning.
/* Test GCC 8.1 -Wcast-function-type for https://bugs.python.org/issue33015
*
*/
vstinner commentedon Oct 23, 2018
The GCC warning is:
func_cast.c:34:30: warning: cast between incompatible function types from 'python_callback' {aka 'void ()(void *)'} to 'void * ()(void *)' [-Wcast-function-type]
pthread_callback func2 = (pthread_callback)func;
^
vstinner commentedon Oct 23, 2018
I wrote a different fix for the compiler warning using a temporary cast to "void*": PR 10057.
pitrou commentedon Oct 23, 2018
Right, so one PR is a real fix, the other PR is a workaround (avoids the warning without fixing the underlying problem).
The underlying problem is: if a platform has incompatible ABIs for the two function types, casting one function type to another may produce bugs (memory corruptions, crashes, etc). We can however decide to consider those platforms unlikely, as the current code has been working for years or decades.
It would be nice to have an opinion from C specialists.
zooba commentedon Oct 23, 2018
Unfortunately, this isn't really a safe cast, as we're going from void to non-void return value.
On x86 with current calling conventions, this is okay, since the return value is in a register that does not change or require cleanup by the caller. However, I wouldn't want to assume that all future calling conventions on other architectures would also permit this - returning a pointer value on the stack or in some way that requires cleanup is entirely possible, and is the sort of problem that would likely only be detectable by this warning or very careful memory measurements (or possibly a very confusing crash due to invalid stack modifications).
It's also possible that returning an invalid pointer could cause a compiler to one day invoke its undefined behavior clause. Even though *we* don't use the return value, it still gets returned somewhere.
I'm not thrilled about the memory allocation here either, but there isn't really much of an option in my opinion.
10 remaining items
miss-islington commentedon Nov 30, 2018
New changeset b135535 by Miss Islington (bot) in branch '3.7':
bpo-33015: Fix UB in pthread PyThread_start_new_thread (GH-6008)
b135535
vstinner commentedon Nov 30, 2018
New changeset 8f83c2f by Victor Stinner in branch '2.7':
bpo-33015: Fix UB in pthread PyThread_start_new_thread (GH-6008) (GH-10823)
8f83c2f
vstinner commentedon Nov 30, 2018
New changeset 03b1200 by Victor Stinner in branch '3.6':
bpo-33015: Fix UB in pthread PyThread_start_new_thread (GH-6008) (GH-10822)
03b1200
vstinner commentedon Nov 30, 2018
Ok, the bug should now be fixed.
vstinner commentedon Nov 30, 2018
Oh... test_threaded_import started to crash on Python 2.7:
$ ./python -m test -F -v test_threaded_import (...) 0:00:00 load avg: 1.06 [ 3] test_threaded_import Trying 20 threads ... OK. Trying 50 threads ... OK. Trying 20 threads ... OK. Segmentation fault (core dumped)The problem is that PyMem_Malloc() is not thread safe when Python is compiled in debug mode! In release mode, it's safe because PyMem_Malloc() is a thin wrapper to malloc(). But in debug mode, PyMem_Malloc() uses the debug hooks and... pymalloc allocator which is not thread-safe!
vstinner commentedon Nov 30, 2018
I wrote PR 10828 to make PyMem_Malloc() thread-safe in debug mode as well.. But I'm not sure that it's ok to push such change later in the 2.7 development cycle...
So I wrote PR 10829 which only modified PyThread_start_new_thread(): use malloc/free instead of PyMem_Malloc/PyMem_Free.
vstinner commentedon Nov 30, 2018
New changeset bc9f53f by Victor Stinner in branch '2.7':
bpo-33015: Use malloc() in PyThread_start_new_thread() (GH-10829)
bc9f53f
gpshead commentedon Nov 30, 2018
why was that only an issue on 2.7?
vstinner commentedon Nov 30, 2018
I added PyMem_RawMalloc() to Python 3.4 and this function must be thread-safe.
https://docs.python.org/dev/c-api/memory.html#raw-memory-interface
"These functions are thread-safe, the GIL does not need to be held."
I replaced PyMem_RawMalloc() with PyMem_Malloc() when I backported the change, but I didn't know that PyMem_Malloc() isn't thread-safe *in debug mode*!
Gregory: do you think that it would be be crazy to fix PyMem_Malloc() to make it thread-safe in debug mode as well? I implemented a fix: PR 10828.
gpshead commentedon Nov 30, 2018
I don't think it is crazy for 2.7, but I'd move that to its own issue as you already nicely addressed the problem related to this PR without needing that.
vstinner commentedon Nov 30, 2018
Good idea. I created bpo-35368. I close this issue since it's now fixed.