Skip to content

Conversation

@Jan-E
Copy link
Contributor

@Jan-E Jan-E commented Jun 18, 2019

@remicollet @cmb69
In PHP 7.4 uint and ulong have been removed, although this is not mentioned in the upgrading notes.
This PR fixes compiling of the oauth extension.

See php/pecl-text-ereg#1 as well.

@cmb69
Copy link
Member

cmb69 commented Jun 19, 2019

@Jan-E, thanks for the ping. Removal documented with php/php-src@571c6bc. The direct replacement of uint and ulong would be unsigned int and unsigned long, respectively; unless uint32_t fits better, I'd stick with unsigned int.

@Jan-E
Copy link
Contributor Author

Jan-E commented Jun 19, 2019

@cmb69 With respect to uint, I just copied what @weltling did in php/php-src@afb6ca2
Thanks for the documentation of the change.

And @remicollet Choose whatever new type you seem to be fitting best.

#if OAUTH_USE_CURL
static size_t soo_read_response(char *ptr, size_t size, size_t nmemb, void *ctx) /* {{{ */
{
uint relsize;
Copy link
Member

Choose a reason for hiding this comment

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

This one should be size_t, as it is the result of multiplying two size_t's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I will not modify the PR anymore, because I am not that familiar with the OAuth details. I will close the PR when a fix has been applied.

@cmb69
Copy link
Member

cmb69 commented Jun 19, 2019

@Jan-E, using something more specific than unsigned int|long is likely reasonable, but I wouldn't do it unless I'd time to examine the code. (Since unsigned int and uint32_t are the same for all supported platforms, this doesn't matter much, though.) However, be careful to replace ulong with zend_ulong, since the former is 32bit on LLP64 platforms (such as Windows), but the latter is 32bit on 32bit platforms, and 64bit on 64bit platforms.

@nikic
Copy link
Member

nikic commented Jun 19, 2019

From a not too deep look, the ulong -> zend_ulong replacements here look fine, and some might actually be fixing potential stack corruption bugs on Windows (where ulong* is passed bug zend_ulong* is expected).

@Jan-E
Copy link
Contributor Author

Jan-E commented Jul 16, 2019

Closing this PR as it has conflicts with #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants