Skip to content

Conversation

@bukka
Copy link
Member

@bukka bukka commented May 7, 2025

The is another chunk of work to support custom libctx - this time in cipher and message digest code.

@bukka
Copy link
Member Author

bukka commented May 7, 2025

This needs a bit more work to free stuff correctly so moving to draft.

@bukka bukka force-pushed the openssl_libctx_md_and_cipher branch from 4b93dca to 2655b22 Compare May 21, 2025 21:21
@bukka bukka marked this pull request as ready for review May 22, 2025 09:49
@bukka
Copy link
Member Author

bukka commented May 22, 2025

Pipeline is green so this is ready for review!

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Preliminary suggestion


const EVP_CIPHER *php_openssl_get_evp_cipher_from_algo(zend_long algo)
{
if (algo < 0 || algo >= (zend_long)(sizeof(php_openssl_cipher_names) / sizeof(*php_openssl_cipher_names))) {
Copy link
Member

Choose a reason for hiding this comment

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

One neat trick is to make the type of algo zend_ulong. Then you only need the >= check and don't need the zend_long cast. Same suggestion for php_openssl_get_evp_md_from_algo

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense but algo might come from PHP constant (for example argument in openssl_pkcs7_encrypt) so this check would just need to be done elsewhere. I think it's better to leave it for separate PR as it's a bit unrelated to this change.

@bukka bukka merged commit 2f5ef4d into php:master Jun 4, 2025
9 checks passed
@bukka
Copy link
Member Author

bukka commented Jun 4, 2025

Just a note that I need to look how to make the whole libctx configurable as it might be a regression for custom OPENSSL_CONF which I don't think is currently loaded in new libctx

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.

2 participants