This repository was archived by the owner on Oct 3, 2023. It is now read-only.
Conversation
zend_parse_parameters is not setting optional zval pointers to NULL. Instead, we need to default the pointer to NULL so we can check for it and set a default value.
php_mt_rand() returns 32 bits of randomness even for 64 bit integers. In the 64 bit case, a 0 is always right shifted into the 32nd bit. In the 32 bit case, a right shift is dependent on the compiler implementation. Generally, negative numbers will remain negative to make bitshift arthmatic work.
chingor13
changed the title
tmatsuo
reviewed
Aug 15, 2017
| } | ||
| #endif | ||
| span->span_id = (php_mt_rand() >> 1); | ||
| span->span_id = ((uint32_t) php_mt_rand()) >> 1; |
Contributor
There was a problem hiding this comment.
So does it pass the test on 32bit? Worth adding some comments?
Member
Author
There was a problem hiding this comment.
It should pass on 32-bit versions of PHP we now test against PHP 7.0 and 7.1 on a 32-bit debian build.
Also added comment about why this is in here.
Contributor
There was a problem hiding this comment.
Thanks. It might be a dumb question.
The return value of php_mt_rand is uint32_t (https://github.com/php/php-src/blob/869fccea24ac829de8ebf9947f6515f03b7d1fe1/ext/standard/mt_rand.c#L163) and I still don't understand why it works.
tmatsuo
approved these changes
Aug 15, 2017
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Fixes #13 (unsigned int issue)
Fixes #14 (segfaults with zend_parse_parameters)