php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #80284 Potential issue in win32/signal.c: Return Value Not Checked from Function Call
Submitted: 2020-10-26 15:06 UTC Modified: 2020-10-27 15:39 UTC
From: sagpant at microsoft dot com Assigned:
Status: Open Package: *General Issues
PHP Version: 7.4.11 OS:
Private report: No CVE-ID: None
Have you experienced this issue?
Rate the importance of this bug to you:

 [2020-10-26 15:06 UTC] 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.


Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-10-26 17:39 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2020-10-26 17:39 UTC] 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 17:47 UTC] 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:48 UTC] stas@php.net
-Type: Security +Type: Bug
 [2020-10-27 15:39 UTC] sagpant at microsoft dot com
-Status: Feedback +Status: Open
 [2020-10-27 15:39 UTC] 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-11-09 12:50 UTC] cmb@php.net
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
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Thu Dec 03 14:01:27 2020 UTC