Bug #77580 [Opn]: DeleteTimerQueueTimer() return code

Edit report at https://bugs.php.net/bug.php?id=77580&edit=1 ID: 77580 User updated by: oliver dot pfister at contaware dot com Reported by: oliver dot pfister at contaware dot com Summary: DeleteTimerQueueTimer() return code Status: Open Type: Bug Package: Scripting Engine problem Operating System: Windows PHP Version: Irrelevant Block user comment: N Private report: N New Comment: 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. Previous Comments: ------------------------------------------------------------------------ [2019-02-12 18:04:08] 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 07:45:55] 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-11 17:27:25] 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-10 21:52:30] 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-08 19:07:05] 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. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=77580 -- Edit this bug report at https://bugs.php.net/bug.php?id=77580&edit=1