Fix incorrect check in fpm_shm_free() #13797
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`if (fpm_shm_size - size > 0)` will be rewritten by the compiler as this: `if (fpm_shm_size == size)`, which is undesirable. The reason this happens is that both variables are size_t, so subtracting them cannot be negative. The only way it can be not > 0, is if they're equal because the result will then be 0. This means that if we have multiple fpm shm allocations, the shm size will not decrease correctly. To showcase that the compiler actually does this, take a look at this isolated case: https://godbolt.org/z/azobdWcrY. Here we can see the usage of the compare instruction + cmove, so the "then" branch is only done if the variables are equal.
devnexen
approved these changes
Mar 24, 2024
nielsdos
added a commit
that referenced
this pull request
Mar 24, 2024
* PHP-8.2: [ci skip] NEWS Fix incorrect check in fpm_shm_free() (#13797)
nielsdos
added a commit
that referenced
this pull request
Mar 24, 2024
* PHP-8.3: [ci skip] NEWS Fix incorrect check in fpm_shm_free() (#13797)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
if (fpm_shm_size - size > 0)will be rewritten by the compiler as this:if (fpm_shm_size != size), which is undesirable. The reason this happens is that both variables are size_t, so subtracting them cannot be negative. The only way it can be not > 0, is if they're equal because the result will then be 0. This means that the else branch won't work properly. E.g. iffpm_shm_size == 50andsize == 51, thenfpm_shm_sizewill wraparound instead of becoming zero.To showcase that the compiler actually does this, take a look at this isolated case: https://godbolt.org/z/azobdWcrY. Here we can see the usage of the compare instruction + cmove, so the "then" branch is only done if the variables are equal.
That all being said, I think it's better to be an assertion + an unconditional subtraction, as the check itself already looks weird.