Title: Excerpt of a PHP debugging session while working on Snuffleupagus Date: 2019-08-20 12:45 A couple of months ago (well, actually more than a year ago!), while polishing [snuffleupagus](https://github.com/nbs-system/snuffleupagus) for its first release, my [intern](https://github.com/xXx-caillou-xXx) and I encountered an "interesting" issue: our [hardened random](https://snuffleupagus.readthedocs.io/features.html#weak-prng-via-rand-mt-rand) feature was [failing in certain environments](https://github.com/nbs-system/snuffleupagus/issues/66), and we struggled to understand why. To spice things up, we only managed to reproduce it with binaries without debug symbols and on an [ArchLinux](https://www.archlinux.org/) machine. Here is the function in charge of everything random-related: ```C /* This function is needed because `rand` and `mt_rand` parameters * are optional, while the ones from `random_int` aren't. */ static void random_int_wrapper(INTERNAL_FUNCTION_PARAMETERS) { zend_long min, max, result; switch (EX_NUM_ARGS()) { case 0: min = 0; max = PHP_MT_RAND_MAX; break; case 1: // LCOV_EXCL_BR_START ZEND_PARSE_PARAMETERS_START_EX(ZEND_PARSE_PARAMS_QUIET, 1, 1); Z_PARAM_LONG(min); ZEND_PARSE_PARAMETERS_END(); // LCOV_EXCL_BR_END max = PHP_MT_RAND_MAX; break; case 2: default: ZEND_PARSE_PARAMETERS_START_EX(ZEND_PARSE_PARAMS_QUIET, 0, 2); Z_PARAM_LONG(min); Z_PARAM_LONG(max); ZEND_PARSE_PARAMETERS_END(); } if (min > max) { if (php_random_int_throw(max, min, &result) == FAILURE) { return; } } else { if (php_random_int_throw(min, max, &result) == FAILURE) { return; } } RETURN_LONG(result); } ``` We started by checking that it was indeed called when [`rand`](https://secure.php.net/manual/en/function.rand.php) was used in PHP, and it was the case. So we ran the failing test in GDB to investigate further. Since it struggled to find the function, we couldn't put a breakpoint on it; so we slapped the good ol' `__asm__("int3");` right at its beginning to trigger a `SIGTRAP`, giving us a way to investigate what's going on. It seemed that the function was returning early, but this is weird, because there is only a single `return`, and it is at the very end of it, and two others that are after calls to `php_random_int_throw` that would throw a warning before returning if something went wrong (hence the `throw` in the name). So we took at look at the *control flow graph* of the function, exported here thanks to [radare2](http://rada.re): ``` <@@@@@@> f t ┌──────┘ └──┐ │ │ __5574__ __5600__ f t v ┌────┘ └───────│────┐ │ │ │ __5579__ │ │ t f │ │ ┌────┘ └─┐ │ │ │ │ │ │ __5690__ │ │ │ v │ │ │ │ ┌──────┘ │ │ │ │ │ │ │ │ │ │ __5589__ │ │ f t │ │ ┌──────┘ └──┐ │ │ │ │ │ │ __5591__ __5670__ │ │ v f t │ │ └───┐ │ └───────────────┤ │ │ │ │ │ __5681__ │ │ │ v │ │ └─┌───────┘ │ │ │ │ │ │ │ │ __5598__ │ │ f t │ │ ┌────┘ └────┬───────────────┘ │ │ │ │ __55a1__ __560a__ │ t f t f │ ┌─────┘ └────┬──────┘ └─────┐ │ │ │ │ │ __5648__ __55c2__ __5624__ │ v v v │ └──────────────────────┬────┴────────────┘ │ v __55ef__ ``` There are 3 arrows at the end that are going to the terminal node, but oddly enough, there are 2 other ones on the right that are going there too. It seems that there is something fishy in the `ZEND_PARSE_PARAMETERS_START_EX` macros. ```c #define ZEND_PARSE_PARAMETERS_START_EX(flags, min_num_args, max_num_args) do { \ const int _flags = (flags); \ int _min_num_args = (min_num_args); \ int _max_num_args = (max_num_args); \ int _num_args = EX_NUM_ARGS(); \ int _i; \ zval *_real_arg, *_arg = NULL; \ zend_expected_type _expected_type = IS_UNDEF; \ char *_error = NULL; \ zend_bool _dummy; \ zend_bool _optional = 0; \ int error_code = ZPP_ERROR_OK; \ ((void)_i); \ ((void)_real_arg); \ ((void)_arg); \ ((void)_expected_type); \ ((void)_error); \ ((void)_dummy); \ ((void)_optional); \ \ do { \ if (UNEXPECTED(_num_args < _min_num_args) || \ (UNEXPECTED(_num_args > _max_num_args) && \ EXPECTED(_max_num_args >= 0))) { \ if (!(_flags & ZEND_PARSE_PARAMS_QUIET)) { \ zend_wrong_parameters_count_error(_flags & ZEND_PARSE_PARAMS_THROW, _num_args, _min_num_args, _max_num_args); \ } \ error_code = ZPP_ERROR_FAILURE; \ break; \ } \ _i = 0; \ _real_arg = ZEND_CALL_ARG(execute_data, 0); ``` After a lot of exploration (protip: [ctags](http://ctags.sourceforge.net/) is your friend.), we didn't find anything suspicious that would trigger an early return. But notice how the `do {` aren't closed: this is why this macro has a friend called `ZEND_PARSE_PARAMETERS_END()` that you must call when you're done parsing arguments. It's looking like this: ```c #define ZEND_PARSE_PARAMETERS_END_EX(failure) \ } while (0); \ if (UNEXPECTED(error_code != ZPP_ERROR_OK)) { \ if (!(_flags & ZEND_PARSE_PARAMS_QUIET)) { \ if (error_code == ZPP_ERROR_WRONG_CALLBACK) { \ zend_wrong_callback_error(_flags & ZEND_PARSE_PARAMS_THROW, E_WARNING, _i, _error); \ } else if (error_code == ZPP_ERROR_WRONG_CLASS) { \ zend_wrong_parameter_class_error(_flags & ZEND_PARSE_PARAMS_THROW, _i, _error, _arg); \ } else if (error_code == ZPP_ERROR_WRONG_ARG) { \ zend_wrong_parameter_type_error(_flags & ZEND_PARSE_PARAMS_THROW, _i, _expected_type, _arg); \ } \ } \ failure; \ } \ } while (0) #define ZEND_PARSE_PARAMETERS_END() \ ZEND_PARSE_PARAMETERS_END_EX(return) ``` Still nothing suspiciou… wait. The `failure` parameter of the macro is used as a statement near the end of it! This is where our returns are coming from! The (ghetto) solution is the call `ZEND_PARSE_PARAMETERS_END_EX((void)0)` instead of `ZEND_PARSE_PARAMETERS_END`, and this is exactly [what we did](https://github.com/nbs-system/snuffleupagus/pull/72)! If you know a better way to do it, please [tell us](https://github.com/nbs-system/snuffleupagus/pulls)! Until next time, see you in your php stack ;)