Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix PHP 7 compatibility #23

Closed
wants to merge 12 commits into from
Closed

fix PHP 7 compatibility #23

wants to merge 12 commits into from

Conversation

remicollet
Copy link
Contributor

Probably could be improved, but initial PHP 7 support seems OK.

 =====================================================================
 PHP         : /opt/remi/php70/root/usr/bin/php 
 PHP_SAPI    : cli
 PHP_VERSION : 7.0.0-dev
 ZEND_VERSION: 3.0.0-dev
 PHP_OS      : Linux - Linux schrodingerscat.famillecollet.com 3.19.3-200.fc21.x86_64 #1 SMP Thu      Mar 26 21:39:42 UTC 2015 x86_64
 INI actual  : /work/GIT/libsodium-php/tmp-php.ini
 More .INIs  :  
 CWD         : /work/GIT/libsodium-php
 Extra dirs  : 
 VALGRIND    : Not used
 =====================================================================
 TIME START 2015-04-02 16:40:24
 =====================================================================
 PASS Check for libsodium AEAD [tests/crypto_aead.phpt] 
 PASS Check for libsodium box [tests/crypto_box.phpt] 
 PASS Check for libsodium generichash [tests/crypto_generichash.phpt] 
 PASS Check for libsodium bin2hex [tests/crypto_hex.phpt] 
 PASS Check for libsodium secretbox [tests/crypto_secretbox.phpt] 
 PASS Check for libsodium shorthash [tests/crypto_shorthash.phpt] 
 PASS Check for libsodium ed25519 signatures [tests/crypto_sign.phpt] 
 PASS Check for libsodium stream [tests/crypto_stream.phpt] 
 PASS Check for libsodium presence [tests/installed.phpt] 
 PASS Check for libsodium utils [tests/pwhash.phpt] 
 PASS Check for libsodium randombytes [tests/randombytes.phpt] 
 PASS Check for libsodium utils [tests/utils.phpt] 
 PASS Check for libsodium version [tests/version.phpt] 
 =====================================================================
 TIME END 2015-04-02 16:40:24

@remicollet
Copy link
Contributor Author

Notice, run-tests.php doesn't have to be in git, you can remove it and add it to .gitignore

@remicollet
Copy link
Contributor Author

The main issue is that I haven't found a easy wait to create a STRING without copy...
Should use zend_string_allocate and then directly use the buffer. But seems difficult to use and keep PHP 5 compatiblity.

@remicollet
Copy link
Contributor Author

Previous commit is an example about fixing this....
Seems really less legible.

@remicollet
Copy link
Contributor Author

Any feedback on this PR ?

@jedisct1
Copy link
Owner

jedisct1 commented Apr 8, 2015

Hi Remi,

And thanks a lot for this!

A few comments, though.

Introducing strsize_t is great. However, if string sizes can be larger than INT_MAX, there are additional changes to make. There are quite a few checks specifically to avoid going beyond INT_MAX, even though the underlying API doesn't have this limitation.
If we can go up to SIZE_MAX or SSIZE_MAX, these checks should be adjusted accordingly. Or if we consider that INT_MAX is the maximum string length even on PHP7, additional checks on input arguments are required.

The double allocation is a bit annoying. And it renders sodium_memzero() moot since we can have a previous copy around. This can be fixed with a systematic sodium_memzero() call before the efree() call in your macro, but this is yet more overhead piling up.
Wondering if the #if PHP_MAJOR_VERSION < 7 couldn't be replaced by additional macros.

@remicollet
Copy link
Contributor Author

Yes, this is a quick fix, and of course, I don't know enough of libsodium to do the perfect fix.

There is a difficult choice with zend_string,

  • keep code compatible using a few macro (my preferred choice when possible)
  • maintain 2 versions of the sources

I don't see simple way to use zend_string and get rid of #if PHP_MAJOR_VERSION < 7, or perhaps by providing a zend_string for PHP < 7

I think keeping INT_MAX is probably enought for a few years ;)

@remicollet
Copy link
Contributor Author

What do you think of rev 5134347 ?

Code really looks like PHP 7 code, all managed by the compatibility layer.

@remicollet
Copy link
Contributor Author

Waiting for your feedback before trying to remove all remaining call to _RETURN_STRINGL

@remicollet
Copy link
Contributor Author

Rebased on 0.1.3

@jedisct1
Copy link
Owner

Finally merged, with minor changes.

Thanks a ton for your help, Remy!

@jedisct1 jedisct1 closed this Jul 24, 2015
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.

2 participants