php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

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
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: sagpant at microsoft dot com
New email:
PHP Version: OS:

 

 [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

Pull Requests

Pull requests:

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
 [2020-11-13 18:05 UTC] requinix@php.net 
Related To: Bug #80364
 [2024-02-14 11:27 UTC] StephendfgBaker at outlook dot com 
(https://github.com)(https://www.doglikesbest.com/)
That was so amazing.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Sat Dec 21 12:01:31 2024 UTC