Bug #75633 [Opn]: In multi-threaded code, if a thread handle is reused by the OS, the app crashes

Edit report at https://bugs.php.net/bug.php?id=75633&edit=1 ID: 75633 Updated by: ab@php.net Reported by: rperper at litespeedtech dot com Summary: In multi-threaded code, if a thread handle is reused by the OS, the app crashes Status: Open Type: Bug Package: Reproducible crash Operating System: OpenSuSE PHP Version: 7.2.0 Block user comment: N Private report: N New Comment: @rperper please lets see the patch first then. Whether it's suitable for a patch version depends on the extent and severity. As general principles, a patch that targets a stable branch has to retain BC and not breach binary compatibility. Obviously it also should be ensured CLI and Apache still work. Depending on all that, an approval through RM could be required. Otherwise, you could target master. Thanks. Previous Comments: ------------------------------------------------------------------------ [2017-12-06 14:10:02] rperper at litespeedtech dot com We consider this to be a critical path problem as there's no reasonable way in our code to have a thread never terminate. So we'll be generating a patch to allow cleanup of resources when a thread terminates and posting it to this problem within the next couple of days. We really hope it can be included in the next PHP patch release. Please let me know if this is a problem for you. Thanks much. Bob Perper rperper@litespeedtech.com ------------------------------------------------------------------------ [2017-12-06 11:10:21] ab@php.net Thanks for the tests. Actually, that's an interesting experiment, from the usability perspective it seems however to make a little sense. Once a thread is started, it serves multiple requests, not just one. The most meaningful parallel in this case were probably to check how Apache handles it with MinSpareThreads/MaxSpareThreads. There might be also different handling in regard to whether an MPM uses multiple processes, but i haven't seen so far that threads would be joined until the process exit. When looking it from the MPM implementation perspective - it seems also logic, as a MPM cannot know about implementation details of a concrete SAPI module. Until MPM also doesn't provide callback for thread cleanups, it should not join threads for this reason. Now, I don't know the exact features the litespeed threading provides, but with the regard to the current TSRM implementation the test code doesn't seem correct. Once a thread() is called, it simply should start a new serving instance. After that it can accept clients and serve multiple requests per process_req(). Furthermore, the lone thread(NULL) calls in the test code look wrong, they should only be called in the new thread context so the correct TLS items are used. At this point - perhaps the TSRM layer could be extended with the thread cleanup callbacks for the case a server module provides such a functionality, but otherwise the functionality is OK when sage of the TSRM API is sufficient. Thanks. ------------------------------------------------------------------------ [2017-12-05 15:17:19] rperper at litespeedtech dot com I would have expected that it would be seen more often as well. But one of the people here suggested that perhaps other developers create a thread and reuse it, only creating new threads for larger amounts of work - and not deleting the existing ones. I don't see a lot of thread questions and bugs posted so I can't speak to how much threads are used. Except that I was told that 5.x threading is no longer supported and not until 7.2 has threading really been functional. Thanks, Bob rperper@litespeedtech.com ------------------------------------------------------------------------ [2017-12-05 14:56:10] danack@php.net interesting bug, a possibly stupid question: Any idea why isn't this a more commonly seen problem? I would have imagined if the code wasn't handling that scenario correctly, then other servers that dynamically allocate threads would have seen similar issues. ------------------------------------------------------------------------ [2017-12-05 14:35:40] rperper at litespeedtech dot com Description: ------------ I am a developer at LiteSpeed Technologies and am working on a thread-capable version of the PHP module to be included in the Open-LiteSpeed web server. During load testing, our application would occasionally crash and always at the same place: at a "zend_first_try". In testing, the crash would occur just by reading the value in EG(bailout) (the first thing done by zend_first_try). After quite a bit of examination, it was determined that a thread was being created, destroyed, a new thread was created and it had the same pthread_self value. In examining TSRM.c it appears that the thread_self value is hashed to reference globals. This will fail if the thread_self value is reused (which it is by the operating system). I have recreated the problem with a test program which is included. Test script: --------------- Install in a php 7.2.0 installation in the sapi directory: https://drive.google.com/file/d/1VK7tcSq3zk-DFNF678OQSWxo03QPpmaK/view?usp=sharing Copy the tar file to that directory and extract it: tar xvf phptest.tar The instructions on how to compile and test it are in the README. But it basically comes down to running ./buildconf --force, configure and make. The crash will occur on execution of the generated phptest program. Expected result: ---------------- Either have a method to automatically detect that a pthread_self value is an actual different thread (which can be done using the syscall interface) or provide a function for us to call to clear out a thread's details when we destroy the thread. Actual result: -------------- Backtrace in gdb: #0 process_req() at /home/user/proj/phptest/php-7.2.0/sapi/phptest/phptest.c:392 #1 begin_process() at /home/user/proj/phptest/php-7.2.0/sapi/phptest/phptest.c:423 #2 thread() at /home/user/proj/phptest/php-7.2.0/sapi/phptest/phptest.c:429 #3 start_thread() at /lib64/libpthread.so.0 #4 clone() at /lib64/libc.so.6 ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=75633&edit=1