Skip to content

Conversation

remicollet
Copy link
Member

So other ext can use RANDOM_G macro

Ex: https://github.com/laruence/yar/blob/master/yar_request.c#L37

@remicollet
Copy link
Member Author

also ping @laruence as this is for yar extension

This was referenced Sep 19, 2022
@cmb69
Copy link
Member

cmb69 commented Sep 19, 2022

Good catch! I don't think this would work on Windows, though, where we would likely need something like

#ifdef PHP_WIN32
# ifdef PHP_SOCKETS_EXPORTS
# define PHP_SOCKETS_API __declspec(dllexport)
# else
# define PHP_SOCKETS_API __declspec(dllimport)
# endif
#elif defined(__GNUC__) && __GNUC__ >= 4
# define PHP_SOCKETS_API __attribute__ ((visibility("default")))
#else
# define PHP_SOCKETS_API
#endif

@remicollet
Copy link
Member Author

remicollet commented Sep 19, 2022

@cmb69 I don't see the problem, PHPAPI is what is used in other ext, and is globally available

https://github.com/php/php-src/blob/PHP-8.2/main/php.h#L58

$ grep 'PHPAPI ZEND_EXTERN_MODULE_GLOBALS' ext/*/*.h 
ext/mysqlnd/mysqlnd.h:PHPAPI ZEND_EXTERN_MODULE_GLOBALS(mysqlnd)
ext/pcre/php_pcre.h:PHPAPI ZEND_EXTERN_MODULE_GLOBALS(pcre)
ext/session/php_session.h:PHPAPI ZEND_EXTERN_MODULE_GLOBALS(ps)

P.S. also thinking about why we still have some specific PHP_xxx_API used instead of PHPAPI

@cmb69
Copy link
Member

cmb69 commented Sep 19, 2022

The general problem is that PHP_EXPORTS is only declared when the php.dll is build, but not when shared extensions are build (in which case PHP_EXPORTS must not be defined, because then the exported symbols from php.dll would not be available; MSVC distinguishes visibility for export and import). Since ext/random can't be built shared, there is no problem here. So just disregard my comment. :)

@TimWolla
Copy link
Member

This change is likely appropriate for PHP 8.2 to minimize short-term breakage, but I'd like to point out that the global Mt19937 (and Mt19937 in general) really should not be used any longer.

Extensions should probably switch to either:

  • php_random_int_throw(). On modern kernels / operating systems, the CSPRNG will not fail.
  • A extension-specific instance of Xoshiro256** seeded from the CSPRNG if the CSPRNG is too slow for the use case and cryptographic security is not a requirement.

Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remicollet
Thanks for finding it!

However, I don't understand why you are checking the seeding status in the yar Extension. It seems unnecessary since php_mt_rand() does seeding as needed.

I don't see any problem with the change, so I approve it

@remicollet
Copy link
Member Author

Thanks all

Squashed and merged as 28a4d76

@remicollet remicollet closed this Sep 20, 2022
@remicollet remicollet deleted the issue-rand-glob branch September 20, 2022 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants