php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #77580 DeleteTimerQueueTimer() return code
Submitted: 2019-02-07 14:21 UTC Modified: 2019-02-14 18:14 UTC
From: oliver dot pfister at contaware dot com Assigned: ab (profile)
Status: Closed Package: Scripting Engine problem
PHP Version: Irrelevant OS: Windows
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: oliver dot pfister at contaware dot com
New email:
PHP Version: OS:

 

 [2019-02-07 14:21 UTC] oliver dot pfister at contaware dot com
Description:
------------
DeleteTimerQueueTimer(HANDLE TimerQueue, HANDLE Timer, HANDLE CompletionEvent) return code problem in file zend_execute_API.c

Because of vague Microsoft API documentation (see point 3. below) there is an important implementation difference between Windows and Wine/ReactOS in case of CompletionEvent set to NULL (PHP uses DeleteTimerQueueTimer with CompletionEvent set to NULL):

Windows (tested on all versions of Windows from XP to Win10):
calling DeleteTimerQueueTimer returns TRUE if the callback code is not yet executing, then when the thread (of the pool) enters the callback code, calling DeleteTimerQueueTimer returns FALSE with ERROR_IO_PENDING. When the thread (of the pool) has terminated execution of the callback code, calling DeleteTimerQueueTimer returns again TRUE.

ReactOS/Wine (tested on latest versions of Wine and ReactOS):
calling DeleteTimerQueueTimer always returns FALSE with ERROR_IO_PENDING.

Important points of the DeleteTimerQueueTimer Microsoft Windows documentation (https://msdn.microsoft.com/en-us/library/windows/desktop/ms682569(v=vs.85).aspx):

1. If CompletionEvent is NULL, the function marks the timer for deletion and returns immediately. If the timer has already expired, the timer callback function will run to completion.

2. If the error code is ERROR_IO_PENDING, it is not necessary to call this function again. For any other error, you should retry the call.

3. If there are outstanding callback functions and CompletionEvent is NULL, the function will fail and set the error code to ERROR_IO_PENDING. This indicates that there are outstanding callback functions. Those callbacks either will execute or are in the middle of executing. The timer is cleaned up when the callback function is finished executing.

As a result in Wine/ReactOS it's not possible to use any PHP, it fails with a fatal error. In Windows there is the possibility that the timer is deleted exactly when a thread (of the pool) is executing the callback, in that case a PHP fatal error would be risen for no good reason. So I propose to correct the PHP code and always check GetLastError() making sure it is not returning ERROR_IO_PENDING before failing.

Test script:
---------------
In zend_set_timeout_ex() and zend_unset_timeout() please correct the return code check from: 

if (!DeleteTimerQueueTimer(NULL, tq_timer, NULL)) {

to:

if (!DeleteTimerQueueTimer(NULL, tq_timer, NULL) && GetLastError() != ERROR_IO_PENDING) {


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2019-02-08 19:07 UTC] ab@php.net
-Status: Open +Status: Feedback
 [2019-02-08 19:07 UTC] ab@php.net
Thanks for the report and detailed analysis. Out of curiosity - I can certainly test Wine, but how do you run ReactOS? Would it work under with Hyper-V or VMware?

Disregarding of that, we should not pass when ERROR_IO_PENDING is returned. The timer callback sets flags for Zend VM to recognize the timeout and to properly shutdown. If the timer callback was too slow and didn't set the flags, chances are that ZVM will continue executing some instructions where timeout should happen.

The handling could be possibly improved by two things

- use INVALID_HANDLE_VALUE for the completion event + care about thread safety
- repeat the call once when `GetLastError() != ERROR_IO_PENDING`

If the timer deletion still fails after that, there's no choice other than dying hard, because in that case we can't ensure the timeout is handled properly. Would it workout on ReactOS?

Thanks.
 [2019-02-10 21:52 UTC] oliver dot pfister at contaware dot com
-Status: Feedback +Status: Open
 [2019-02-10 21:52 UTC] oliver dot pfister at contaware dot com
Thanks for the report and detailed analysis. Out of curiosity - I can certainly test Wine, but how do you run ReactOS? Would it work under with Hyper-V or VMware?

-> it's possible to run ReactOS in VMWare, VirtualBox or Hyper-V (note that ReactOS supports only the WinXP API, they are working on Vista and higher API support).


Disregarding of that, we should not pass when ERROR_IO_PENDING is returned. The timer callback sets flags for Zend VM to recognize the timeout and to properly shutdown. If the timer callback was too slow and didn't set the flags, chances are that ZVM will continue executing some instructions where timeout should happen.

-> there is a chance that the callback sets the timed_out global variable after or before it is cleared in zend_set_timeout() or zend_unset_timeout(), I agree that this is not so clean.


The handling could be possibly improved by two things:


- repeat the call once when `GetLastError() == ERROR_IO_PENDING`

-> repeating the DeleteTimerQueueTimer() call when GetLastError() is ERROR_IO_PENDING is not possible, I tested it, it will rise an exception, and also the Microsoft documentation clearly states that when GetLastError() is ERROR_IO_PENDING the timer is scheduled for deletion as soon as the callback exits, so that we should not delete it again. But we are allowed to create a new timer before the old callback terminates, that works, I tested it.


- use INVALID_HANDLE_VALUE for the completion event + care about thread safety

-> that's the cleanest solution, but I do not know whether the access to zend_executor_globals *eg is serialized, if yes we have a problem and must take care of that. Note that the INVALID_HANDLE_VALUE solution would also work well in Wine/ReactOS as in that case the implementation is done coherently with Windows. I made a small Win32 application to test the different timer behaviors, if you are interested, tell me.
 [2019-02-11 17:27 UTC] ab@php.net
About repeating the call, as from the doc

[doc]
If the function fails, the return value is zero. To get extended error information, call GetLastError. If the error code is ERROR_IO_PENDING, it is not necessary to call this function again. For any other error, you should retry the call.
[/doc]

so with `GetLastError() == ERROR_IO_PENDING` one should retry.

With using INVALID_HANDLE_VALUE - i'll do more tests, if you confirm it also works with Wine/ReactOS, it might be the only thing needed.

Thanks.
 [2019-02-12 07:45 UTC] oliver dot pfister at contaware dot com
About repeating the call, as from the doc:

[doc]
If the function fails, the return value is zero. To get extended error information, call GetLastError. If the error code is ERROR_IO_PENDING, it is not necessary to call this function again. For any other error, you should retry the call.
[/doc]

so with `GetLastError() == ERROR_IO_PENDING` one should NOT retry to call DeleteTimerQueueTimer again as the timer has been scheduled for deletion by the DeleteTimerQueueTimer call (if DeleteTimerQueueTimer is called again an unrecoverable Windows exception is risen). With all other error codes php must fail with a fatal error, like already done now.


Back to the idea of using INVALID_HANDLE_VALUE:

Passing that parameter will force DeleteTimerQueueTimer to wait the termination of the callback if at the time of the DeleteTimerQueueTimer call the callback is executing. If we call DeleteTimerQueueTimer before the timer expires then the timer is deleted without any wait. If we call DeleteTimerQueueTimer after the timer callback has been executed then the timer is deleted without any wait. So the possible death lock remains only when we call DeleteTimerQueueTimer while the callback is executing (the callback is executed by a system thread), this is why I asked you whether the access to zend_executor_globals *eg is controlled by a critical section/mutex, is it the case?
 [2019-02-12 18:04 UTC] ab@php.net
Ohh, of course it was a mistake, of course repeat when GetLastError() != ERROR_IO_PENDING :)


The zend_executor_globals access is not guarded. Still one probably should use InterlockedExchange to set the flag, as the timer runs on a separate thread. Thread safe builds and extensions like pthreads should still operate on an isolated copy of the globals, but safer is safer.

The timer callback is supposed to only set the timeout flags, so it should be quite fast. From that POV, probably INVALID_HANDLE_VALUE or NULL shouldn't matter much. And thus, waiting for the callback to complete should not be a big issue.

Thanks.
 [2019-02-12 22:22 UTC] oliver dot pfister at contaware dot com
Using DeleteTimerQueueTimer in locking mode (with the INVALID_HANDLE_VALUE parameter) guarantees that an old callback which could set timed_out after we reset timed_out in zend_set_timeout() or zend_unset_timeout() cannot exist. For this reason in zend_set_timeout_ex() and zend_unset_timeout() I suggest changing: 

if (!DeleteTimerQueueTimer(NULL, tq_timer, NULL)) {

to:

if (!DeleteTimerQueueTimer(NULL, tq_timer, INVALID_HANDLE_VALUE)) {


For types which are not bigger than the size of a native integer, if the memory is properly aligned (as it is by default), reads and writes with literals (like setting or resetting a flag) are always atomic. timed_out is of type zend_bool which is an unsigned char, read and writes on that 8-bit variable is always atomic (no alignment restrictions for a single byte). So it's not necessary to use InterlockedExchange.
 [2019-02-13 05:11 UTC] ab@php.net
-Status: Open +Status: Feedback
 [2019-02-13 05:11 UTC] ab@php.net
I pushed a patch with usage of INVALID_HANDLE_VALUE to 7.4+. Could you check the latest master snapshots on Wine/ReactOS please? https://windows.php.net/snapshots/

Regarding the interlocked API - yes, the are atomic but not in the sense of C++. Atomic in this case means that the bits will be written at once, not that the access would be synchronized.  As the timer callback runs on a separate thread, at least the main thread can produce a race condition. Such issues have never been reported, but it is theoretically possible. But anyway, it's a separate topic, not to be handled here.

Thanks.
 [2019-02-13 22:49 UTC] oliver dot pfister at contaware dot com
-Status: Feedback +Status: Open
 [2019-02-13 22:49 UTC] oliver dot pfister at contaware dot com
I back-ported the INVALID_HANDLE_VALUE patch to my custom PHP 5.6.40 build, it works well under Wine!

I read your commit comment, maybe if you can edit it, I would add that the patch fixes a bug: DeleteTimerQueueTimer with the original NULL parameter, returns FALSE (with ERROR_IO_PENDING) if the callback is in execution. For that case the old code fatally failed, but in reality it is not an error condition at all, it is just the way by which DeleteTimerQueueTimer tells you that right now the callback is executing and that the deletion of the timer will happen when the callback terminates.

Thanks for all
 [2019-02-14 18:14 UTC] ab@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: ab
 [2019-02-14 18:14 UTC] ab@php.net
Many thanks for the collaboration. Closing this for now.

Thanks.
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Thu Jun 20 18:01:27 2019 UTC