Add base32 decode base32 encode

[nyamsprod]

This PR is based on the following RFC:

https://gist.github.com/nyamsprod/8a5cf21c136952a46ec8836f29738c82

The feature is currently incomplete and would need to be finised before submitting it for consideration to be included in the next PHP version.

There's a polyfill in PHP that can be found at aide-base32

[nyamsprod]

For reference the part that validates the padding and the alphabet should be extract in an internal function as this part is identical for both base32_encode and base32_decode functions,
So I believe one or two inline static functions should be used if I understood correctly 🤔

[derickr]

Yes, you should use a static function for that, but marking it as inline is probably not needed, as compilers usually decide this on their own, for only small functions.

[derickr]

Hi!

I've done a more thorough review of base32_encode — I hope I was thorough explaining the how and why, but feel free to ask (here, or somewhere else) if you're unsure.

It's a good effort!

cheers,
Derick

[derickr]

These three calls, allocated new memory. That means that this function should also be responsible for freeing them, before returning with zend_string_release(). You return in lines 63, 71, 80, and 106. It is not unheard of to use goto failure, with a failure: block and the end of the function for this.

You could also only call zend_string_* just before they are actually needed. The new upper_padding and reserved_chars aren't used in the section on lines 61-63, so they could come below.

You can find this kind of memory leak, by compiling PHP in debug mode with the --enable-debug flag. When running your tests, you should then see warnings.

Alternatively, you can run your tests with php run-tests.php TESTS="-m ext/standard/tests/url" — the -m will use valgrind (which you need to install) to run memory analyses.

[derickr]

I would probably move all the things below into a php_base32_encode() function, just like is done for base64 in the PHP_FUNCTION(base64_encode)/PHP_FUNCTION(base64_decode) functions in ext/standard/base64.c. You will also have to add an equivalent PHPAPI extern zend_string *php_base64_encode(...) declaration in base32.h (again, see how base64.h has done that).

By splitting it out, you make this function available to other PHP extensions as well.

[derickr]

There is a PHP variant of strcspn, called php_strcspn (defined in ext/standard/php_string.h). I think because not all platforms might have it. The API is slightly different, but it is NULL safe:

PHPAPI size_t php_strspn(const char *s1, const char *s2, const char *s1_end, const char *s2_end);

[derickr]

There is a NULL-safe PHP variant of strstr too: zend_memnstr(const char *haystack, const char *needle, size_t needle_len, const char *end);

[derickr]

Yes, you should use a static function for that, but marking it as inline is probably not needed, as compilers usually decide this on their own, for only small functions.

[derickr]

That's probably usually defined as: const char *chars = ZSTR_VAL(decoded);.

The [] isn't really wrong, but not common.

The const indicates that the function is not going to modify the contents, which is a good hint to the compiler (and reader of code).

[derickr]

You haven't freed the encoded smart_str, like you did above for with smart_str_free(&unique_chars);.

In this case, that is right, as RETURN_STR(ret_val) doesn't create a duplicate.

Usually, it's folded into one line though:

RETURN_STR(smart_str_extract(&buf));