Skip to content

Conversation

alexandre-daubois
Copy link
Member

Internals book explicitly specifies that sprintf() should be avoided where possible for safer alternatives.

@alexandre-daubois alexandre-daubois marked this pull request as ready for review August 26, 2025 17:53
@nielsdos
Copy link
Member

I did a round on this once a long time ago, strange I missed these, thanks for catching this

@alexandre-daubois alexandre-daubois force-pushed the safe-sprintf branch 3 times, most recently from ff9c9d4 to 98c6ee4 Compare August 27, 2025 08:28
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

The FPM code is not easy to understand, but I believe it is correct.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks correct to me. The fpm code is definitely a readability improvement. Waiting to see if Jakub wants to review, otherwise we can merge in a day or two. Thanks!

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

I thought a little more about the FPM code. Suggestions are not tested, but I've tried to carefully explain my train of thought.

@alexandre-daubois alexandre-daubois force-pushed the safe-sprintf branch 2 times, most recently from 41fa2c8 to ef2d470 Compare August 29, 2025 06:54
@alexandre-daubois
Copy link
Member Author

Your suggestions are correct, thank you Tim!

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Double-checked the FPM logic again and it LGTM.

@TimWolla
Copy link
Member

@php/release-managers-85 This probably is halfway between bugfix and feature, so please take a look. You can “Squash and merge” directly if you are good with it, no NEWS necessary and the author can't merge themselves.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I think it would be best to separate the FPM changes. I'm really not sure that part should go to 8.5 as it needs proper testing once corrected.

if (i % FPM_ENV_SOCKET_SET_SIZE == 0) {
socket_set[socket_set_count] = p + socket_set_buf;
smart_string_appendc(&env_str, '\0');
Copy link
Member

Choose a reason for hiding this comment

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

This is master process code - it should not use zend alloc. Ideally all master code should also not immediately exit during the alloc failure so it should keep using standard malloc / realloc / free. It means even the persistent version would not be preferred here. I would prefer not the use smart string at all here

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.

6 participants