[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

> 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.

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Gregory P. Smith]

Gregory P. Smith  added the comment:

why was that only an issue on 2.7?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

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.

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +10069

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +10068

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

Ok, the bug should now be fixed.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[miss-islington]

miss-islington  added the comment:


New changeset b1355352d14a0a67107aba7ec6f26f17716a 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/b1355352d14a0a67107aba7ec6f26f17716a


--
nosy: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +10066

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +10065

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[miss-islington]

Change by miss-islington :


--
pull_requests: +10064

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

I closed my PR 10057: "Control Flow Integrity killed my PR :-)
https://bugs.python.org/issue33015#msg328325 "

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

"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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Antoine Pitrou]

Antoine Pitrou  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Benjamin Peterson]

Benjamin Peterson  added the comment:

Shall we introduce a new thread-starting API that takes a function with the 
"correct" pthread signature? It seems like Windows already allocates.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--
versions: +Python 2.7, Python 3.6, Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Gregory P. Smith]

Gregory P. Smith  added the comment:

I left comments on the github PRs with reasons why, but PR 6008 seems correct.  
PR 10057 would leave us with undefined behavior.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Alexey Izbyshev]

Alexey Izbyshev  added the comment:

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

--
nosy: +izbyshev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Steve Dower]

Steve Dower  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Antoine Pitrou]

Antoine Pitrou  added the comment:

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.

--
nosy: +gregory.p.smith, skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

I wrote a different fix for the compiler warning using a temporary cast to 
"void*": PR 10057.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +9394

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

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;
  ^

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

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
...
}

--
Added file: https://bugs.python.org/file47888/func_cast.c

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

"Fix function cast warning in thread_pthread.h"

Can you please paste the warning?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[STINNER Victor]

STINNER Victor  added the comment:

> 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.

--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Steve Dower]

Steve Dower  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Siddhesh Poyarekar]

Siddhesh Poyarekar  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Steve Dower]

Steve Dower  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Siddhesh Poyarekar]

Siddhesh Poyarekar  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Steve Dower]

Steve Dower  added the comment:

Can't we just fix the cast? We shouldn't have to do heap allocations for this.

--
nosy: +steve.dower

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Ned Deily]

Change by Ned Deily :


--
nosy: +benjamin.peterson

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Brett Cannon]

Change by Brett Cannon :


--
nosy:  -brett.cannon

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Terry J. Reedy]

Change by Terry J. Reedy :


--
nosy: +brett.cannon, pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Siddhesh Poyarekar]

Change by Siddhesh Poyarekar :


--
keywords: +patch
pull_requests: +5773
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33015] Fix function cast warning in thread_pthread.h

[Siddhesh Poyarekar]

New submission from Siddhesh Poyarekar :

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.

--
components: Build
messages: 313357
nosy: siddhesh
priority: normal
severity: normal
status: open
title: Fix function cast warning in thread_pthread.h
type: compile error

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com