classification
Title: Fix function cast warning in thread_pthread.h
Type: compile error Stage: resolved
Components: Build Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, gregory.p.smith, izbyshev, miss-islington, pitrou, serhiy.storchaka, siddhesh, skrah, steve.dower, vstinner
Priority: normal Keywords: patch

Created on 2018-03-06 18:56 by siddhesh, last changed 2018-11-30 23:11 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
func_cast.c vstinner, 2018-10-23 10:46
Pull Requests
URL Status Linked Edit
PR 6008 merged siddhesh, 2018-03-06 18:57
PR 10057 closed vstinner, 2018-10-23 12:31
PR 10821 merged miss-islington, 2018-11-30 15:14
PR 10822 merged vstinner, 2018-11-30 15:21
PR 10823 merged vstinner, 2018-11-30 15:28
PR 10828 merged vstinner, 2018-11-30 16:52
PR 10829 merged vstinner, 2018-11-30 16:54
Messages (32)
msg313357 - (view) Author: Siddhesh Poyarekar (siddhesh) * Date: 2018-03-06 18:56
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.
msg316332 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-05-09 21:30
Can't we just fix the cast? We shouldn't have to do heap allocations for this.
msg316348 - (view) Author: Siddhesh Poyarekar (siddhesh) * Date: 2018-05-10 06:41
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.
msg316415 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-05-11 21:44
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.
msg316639 - (view) Author: Siddhesh Poyarekar (siddhesh) * Date: 2018-05-15 11:25
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.
msg316709 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-05-15 19:45
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.
msg328298 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 10:22
> 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.

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.
msg328300 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 10:29
"Fix function cast warning in thread_pthread.h"

Can you please paste the warning?
msg328301 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 10:46
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
 *
 * Compiled on Linux with:
 * gcc x.c -o x -Wall -Wextra -lpthread
 *
 * Workaround the cast:
 * gcc x.c -o x -Wall -Wextra -lpthread -D UGLY_CAST
 */


/* No result value */
typedef void (*python_callback) (void *);

/* Result type: "void*" (untyped pointer) */
typedef void* (*pthread_callback) (void *);

int test_cast(python_callback func)
{
    ...
#ifdef UGLY_CAST
    pthread_callback func2 = (pthread_callback)(void *)func;
#else
    pthread_callback func2 = (pthread_callback)func;
#endif
    ...
}
msg328302 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 10:47
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;
                              ^
msg328308 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 12:35
I wrote a different fix for the compiler warning using a temporary cast to "void*": PR 10057.
msg328310 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-10-23 12:59
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.
msg328313 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-10-23 13:20
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.
msg328325 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2018-10-23 16:34
Such casts will also trigger a CFI violation if somebody tries to build Python (and the libc, in this case) with a signature-based CFI [1, 2]. It checks that the compile-time callee signature matches the signature of the actually called function in runtime. 

[1] https://clang.llvm.org/docs/ControlFlowIntegrity.html
[2] https://android-developers.googleblog.com/2018/10/control-flow-integrity-in-android-kernel.html
msg328387 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-10-24 19:48
I left comments on the github PRs with reasons why, but PR 6008 seems correct.  PR 10057 would leave us with undefined behavior.
msg328388 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-10-24 19:52
This is presumably also present in 3.6 and 2.7 so I've tagged those on the issue, but for this kind of change i'd leave it up to release managers to see if they want to backport something of this nature that late in those release cycles.

Observable side effect: It adds a small memory allocation & free around every thread creation and an additional small C stack frame.  I do not expect that to be observable performance wise given CPython threading not being a high performer thanks to the GIL anyways.
msg328409 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-10-25 04:30
Shall we introduce a new thread-starting API that takes a function with the "correct" pthread signature? It seems like Windows already allocates.
msg328415 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-10-25 07:03
> Shall we introduce a new thread-starting API that takes a function with the "correct" pthread signature?

Do we need it? I don't think saving a single memory allocation is worth the bother.
msg328417 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-25 07:18
"Shall we introduce a new thread-starting API that takes a function with the "correct" pthread signature?"

Extract of my PR:

"Python uses pthread_detach() and doesn't use pthread_join(), the thread return value is ignored."

Python doesn't give access to the return value.
msg328838 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-29 15:41
I closed my PR 10057: "Control Flow Integrity killed my PR :-)
https://bugs.python.org/issue33015#msg328325 "
msg330791 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 15:14
New changeset 9eea6eaf23067880f4af3a130e3f67c9812e2f30 by Victor Stinner (Siddhesh Poyarekar) in branch 'master':
bpo-33015: Fix UB in pthread PyThread_start_new_thread (GH-6008)
https://github.com/python/cpython/commit/9eea6eaf23067880f4af3a130e3f67c9812e2f30
msg330794 - (view) Author: miss-islington (miss-islington) Date: 2018-11-30 15:32
New changeset b1355352d14a0a67107aba7ec6f233336f17716a by Miss Islington (bot) in branch '3.7':
bpo-33015: Fix UB in pthread PyThread_start_new_thread (GH-6008)
https://github.com/python/cpython/commit/b1355352d14a0a67107aba7ec6f233336f17716a
msg330798 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 16:04
New changeset 8f83c2fb19c45350c2161d9e75dab4cd2bcaee28 by Victor Stinner in branch '2.7':
bpo-33015: Fix UB in pthread PyThread_start_new_thread (GH-6008) (GH-10823)
https://github.com/python/cpython/commit/8f83c2fb19c45350c2161d9e75dab4cd2bcaee28
msg330799 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 16:04
New changeset 03b1200dfd03061e9ad0bff8199967bd80b9b900 by Victor Stinner in branch '3.6':
bpo-33015: Fix UB in pthread PyThread_start_new_thread (GH-6008) (GH-10822)
https://github.com/python/cpython/commit/03b1200dfd03061e9ad0bff8199967bd80b9b900
msg330800 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 16:05
Ok, the bug should now be fixed.
msg330806 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 16:54
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!
msg330807 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 16:56
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.
msg330810 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 17:08
New changeset bc9f53f69e8207027bf2b18e3d01b30401e76ace by Victor Stinner in branch '2.7':
bpo-33015: Use malloc() in PyThread_start_new_thread() (GH-10829)
https://github.com/python/cpython/commit/bc9f53f69e8207027bf2b18e3d01b30401e76ace
msg330829 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-11-30 22:38
why was that only an issue on 2.7?
msg330830 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 22:53
> why was that only an issue on 2.7?

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.
msg330832 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-11-30 23:06
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.
msg330835 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-30 23:11
> 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.

Good idea. I created bpo-35368. I close this issue since it's now fixed.
History
Date User Action Args
2018-11-30 23:11:09vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg330835
2018-11-30 23:06:01gregory.p.smithsetmessages: + msg330832
2018-11-30 22:53:05vstinnersetmessages: + msg330830
2018-11-30 22:38:23gregory.p.smithsetmessages: + msg330829
2018-11-30 17:08:05vstinnersetmessages: + msg330810
2018-11-30 16:56:08vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg330807
2018-11-30 16:54:56vstinnersetmessages: + msg330806
2018-11-30 16:54:32vstinnersetpull_requests: + pull_request10069
2018-11-30 16:52:14vstinnersetpull_requests: + pull_request10068
2018-11-30 16:05:17vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg330800

stage: patch review -> resolved
2018-11-30 16:04:48vstinnersetmessages: + msg330799
2018-11-30 16:04:38vstinnersetmessages: + msg330798
2018-11-30 15:32:15miss-islingtonsetnosy: + miss-islington
messages: + msg330794
2018-11-30 15:28:33vstinnersetpull_requests: + pull_request10066
2018-11-30 15:21:16vstinnersetpull_requests: + pull_request10065
2018-11-30 15:14:42miss-islingtonsetpull_requests: + pull_request10064
2018-11-30 15:14:35vstinnersetmessages: + msg330791
2018-10-29 15:41:07vstinnersetmessages: + msg328838
2018-10-25 07:18:24vstinnersetmessages: + msg328417
2018-10-25 07:03:07pitrousetmessages: + msg328415
2018-10-25 04:30:02benjamin.petersonsetmessages: + msg328409
2018-10-24 19:52:11gregory.p.smithsetmessages: + msg328388
versions: + Python 2.7, Python 3.6, Python 3.7, Python 3.8
2018-10-24 19:48:07gregory.p.smithsetmessages: + msg328387
2018-10-23 19:06:06serhiy.storchakasetnosy: + serhiy.storchaka
2018-10-23 16:34:29izbyshevsetnosy: + izbyshev
messages: + msg328325
2018-10-23 13:20:11steve.dowersetmessages: + msg328313
2018-10-23 12:59:24pitrousetnosy: + gregory.p.smith, skrah
messages: + msg328310
2018-10-23 12:35:57vstinnersetmessages: + msg328308
2018-10-23 12:31:42vstinnersetpull_requests: + pull_request9394
2018-10-23 10:47:41vstinnersetmessages: + msg328302
2018-10-23 10:46:48vstinnersetfiles: + func_cast.c

messages: + msg328301
2018-10-23 10:29:53vstinnersetmessages: + msg328300
2018-10-23 10:22:01vstinnersetnosy: + vstinner
messages: + msg328298
2018-05-15 19:45:46steve.dowersetmessages: + msg316709
2018-05-15 11:25:00siddheshsetmessages: + msg316639
2018-05-11 21:44:14steve.dowersetmessages: + msg316415
2018-05-10 06:41:06siddheshsetmessages: + msg316348
2018-05-09 21:30:30steve.dowersetnosy: + steve.dower
messages: + msg316332
2018-04-22 19:02:19ned.deilysetnosy: + benjamin.peterson
2018-03-10 19:53:14brett.cannonsetnosy: - brett.cannon
2018-03-09 20:27:54terry.reedysetnosy: + brett.cannon, pitrou
2018-03-06 18:57:40siddheshsetkeywords: + patch
stage: patch review
pull_requests: + pull_request5773
2018-03-06 18:56:13siddheshcreate