Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

random extension is not thread safe #9067

Closed
ophian opened this issue Jul 20, 2022 · 5 comments
Closed

random extension is not thread safe #9067

ophian opened this issue Jul 20, 2022 · 5 comments

Comments

@ophian
Copy link

ophian commented Jul 20, 2022

Description

I am sorry for having to ask this, but I just hopped on 8..2-beta1 from QA on my local server and had the issue that all simple pages like phpinfo or my simple file system navigation worked well with it, but none of the more advanced frameworks that were doing well with 8.2-alpha-1, alpha-2 and 8.2-alpha-3 before.
Browser is set to use HTTPs and uses h2.
I turned down the server, dropped files over and restarted again
I then tried searching the UPGRADING file but could not really find the issue.

All these pages just stay as they were or when re-opening the browser just fail (with some networking issue, as if there is no internet connection), Nothing is noted in the 8.2 error_log file. I even tried adding error_log_mode to the ini file, with no success. So I returned to 8.2.0alpha3 for short.

Anything I could try before tomorrows release?

PHP Version

PHP 8.2-beta1

Operating System

Win xampp Apache module

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2022

I can confirm segfaults with non-trivial scripts with apache2handler. I need to investigate closer.

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2022

Simple reproducer:

<?php
session_start();

Stack backtrace:

>	php8ts.dll!php_combined_lcg() Zeile 426	C
 	php8ts.dll!php_session_gc(bool immediate) Zeile 373	C
 	php8ts.dll!php_session_initialize() Zeile 453	C
 	php8ts.dll!php_session_start() Zeile 1617	C
 	php8ts.dll!zif_session_start(_zend_execute_data * execute_data, _zval_struct * return_value) Zeile 2542	C
 	php8ts.dll!ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER(_zend_execute_data * execute_data) Zeile 1265	C
 	php8ts.dll!execute_ex(_zend_execute_data * ex) Zeile 55689	C
 	php8ts.dll!zend_execute(_zend_op_array * op_array, _zval_struct * return_value) Zeile 60255	C
 	php8ts.dll!zend_execute_scripts(int type, _zval_struct * retval, int file_count, ...) Zeile 1771	C
 	php8ts.dll!php_execute_script(_zend_file_handle * primary_file) Zeile 2535	C
 	php8apache2_4.dll!php_handler(request_rec * r) Zeile 708	C
 	[Externer Code]	

php_random_status *status = RANDOM_G(combined_lcg);

status == NULL after initialization.

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2022

The problem is that the new random extension doesn't implement a GINIT function and as such is not properly thread safe. I think that the following code needs to be moved (and adapted) to GINIT:

php-src/ext/random/random.c

Lines 843 to 849 in c2f5bd1

RANDOM_G(random_fd) = -1;
RANDOM_G(combined_lcg) = php_random_status_alloc(&php_random_algo_combinedlcg, true);
RANDOM_G(combined_lcg_seeded) = false;
RANDOM_G(mt19937) = php_random_status_alloc(&php_random_algo_mt19937, true);
RANDOM_G(mt19937_seeded) = false;

@cmb69 cmb69 changed the title PHP 8.2-beta1 Win build has networking issue random extension is not thread safe Jul 20, 2022
@ophian
Copy link
Author

ophian commented Jul 20, 2022

I hear you Christoph... Hoping all the best. 😎
Thank you for taking all that extra time and effort to get this sorted out!

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2022

Thanks for reporting! I'm consulting with the release managers on what to do about this issue.

cmb69 added a commit to cmb69/php-src that referenced this issue Jul 20, 2022
For thread-safety, we need to initialize global variables in GINIT (or
RINIT), but not in MINIT.
@cmb69 cmb69 linked a pull request Jul 20, 2022 that will close this issue
@cmb69 cmb69 closed this as completed in 8487d8f Jul 21, 2022
@zeriyoshi zeriyoshi mentioned this issue Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants