Bug #80284 [PATCH]: Potential issue in win32/signal.c: Return Value Not Checked from Function Call

Edit report at https://bugs.php.net/bug.php?id=80284&edit=1 ID: 80284 Patch added by: cmb@php.net Reported by: sagpant at microsoft dot com Summary: Potential issue in win32/signal.c: Return Value Not Checked from Function Call Status: Open Type: Bug Package: *General Issues PHP Version: 7.4.11 Block user comment: N Private report: N New Comment: The following pull request has been associated: Patch Name: Fix #80284: Return Value Not Checked from Function Call On GitHub: https://github.com/php/php-src/pull/6412 Patch: https://github.com/php/php-src/pull/6412.patch Previous Comments: ------------------------------------------------------------------------ [2020-10-27 15:39:31] sagpant at microsoft dot com I agree with you that the retval initialization is taken care by the initialization function itself. So, as you pointed out that it would only make sense to check the value of retval as IS_UNDEF. The reason our system flagged this issue is because it found different instances of this function call where the return value from the function call was checked for SUCCESS/FAILURE. Since our system learns from the repo codebase itself, in this case php, it found inconsistent usage of this function call_user_function. This function is called for a total of 39 times and out these 39 times, return value is checked at 30 instances and at 2 separate instances, argument is initialized before making the function call. ------------------------------------------------------------------------ [2020-10-26 17:47:02] cmb@php.net call_user_function() calls _call_user_function_ex()[1] which in turn calls zend_call_function()[2]. The latter defines the return value (IS_UNDEF) before the function could fail. So if call_user_function() fails, the following zval_ptr_dtor() would practically be a NOP. As such I don't see a security issue here. I also don't see that there is any special code path that should be exercised if call_user_function() fails here, except maybe for asserting that retval is indeed IS_UNDEF. [1] <https://github.com/php/php-src/blob/php-7.4.11/Zend/zend_execute_API.c#L633> [2] <https://github.com/php/php-src/blob/php-7.4.11/Zend/zend_execute_API.c#L649> ------------------------------------------------------------------------ [2020-10-26 17:39:32] stas@php.net Could you please explain what is the security issue here? Code readability is not a security issue per se. ------------------------------------------------------------------------ [2020-10-26 15:06:46] sagpant at microsoft dot com Description: ------------ In this codebase, you often check the return value of the implicated function when calling it, but in this instance, it appears that you didn’t. Using a consistent return value checking and/or error handling approach can improve code robustness and readability. File: PHP-7.4.11/win32/signal.c Line Number: 39 Function: call_user_function Correct reference usage found in main/streams/userspace.c line: 940 Test script: --------------- Analyzer points out inconsistencies in the code. ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=80284&edit=1