From 897ca85d33ebeb25ba97793dc7ab9d1761c0a0a3 Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Tue, 9 Aug 2022 22:41:47 +0800 Subject: [PATCH 1/2] Revert "Fix GH-8409: SSL handshake timeout persistent connections hanging" This reverts commit d0527427be57957157aec5e26a28899b380140df. This patch makes Swoole/Swow can not work anymore, because Coroutine will yield to another one during socket operation, EG(record_errors) assertion will always fail, and zend_begin_record_errors() was only used during compile time before. Note: zend_emit_recorded_errors() and the typo fix are reserved. --- NEWS | 2 -- Zend/zend.c | 16 +--------------- Zend/zend.h | 1 - Zend/zend_globals.h | 4 +--- ext/standard/tests/streams/gh8409.phpt | 20 -------------------- main/streams/transports.c | 5 ----- 6 files changed, 2 insertions(+), 46 deletions(-) delete mode 100644 ext/standard/tests/streams/gh8409.phpt diff --git a/NEWS b/NEWS index e584e542032..45958625788 100644 --- a/NEWS +++ b/NEWS @@ -49,8 +49,6 @@ PHP NEWS - Streams: . Fixed bug GH-8472 (The resource returned by stream_socket_accept may have incorrect metadata). (Jakub Zelenka) - . Fixed bug GH-8409 (SSL handshake timeout leaves persistent connections - hanging). (Jakub Zelenka) 04 Aug 2022, PHP 8.1.9 diff --git a/Zend/zend.c b/Zend/zend.c index 5e830dad89c..be7fa210fff 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -1366,9 +1366,6 @@ ZEND_API ZEND_COLD void zend_error_zstr_at( EG(num_errors)++; EG(errors) = erealloc(EG(errors), sizeof(zend_error_info) * EG(num_errors)); EG(errors)[EG(num_errors)-1] = info; - if (EG(record_errors_without_emitting)) { - return; - } } /* Report about uncaught exception in case of fatal errors */ @@ -1622,25 +1619,14 @@ ZEND_API ZEND_COLD void zend_error_zstr(int type, zend_string *message) { zend_error_zstr_at(type, filename, lineno, message); } -static zend_always_inline void zend_begin_record_errors_ex(bool no_emmitting) +ZEND_API void zend_begin_record_errors(void) { ZEND_ASSERT(!EG(record_errors) && "Error recording already enabled"); EG(record_errors) = true; - EG(record_errors_without_emitting) = no_emmitting; EG(num_errors) = 0; EG(errors) = NULL; } -ZEND_API void zend_begin_record_errors(void) -{ - zend_begin_record_errors_ex(false); -} - -ZEND_API void zend_begin_record_errors_without_emitting(void) -{ - zend_begin_record_errors_ex(true); -} - ZEND_API void zend_emit_recorded_errors(void) { EG(record_errors) = false; diff --git a/Zend/zend.h b/Zend/zend.h index 52e30141961..001bf329b9e 100644 --- a/Zend/zend.h +++ b/Zend/zend.h @@ -394,7 +394,6 @@ ZEND_API void zend_save_error_handling(zend_error_handling *current); ZEND_API void zend_replace_error_handling(zend_error_handling_t error_handling, zend_class_entry *exception_class, zend_error_handling *current); ZEND_API void zend_restore_error_handling(zend_error_handling *saved); ZEND_API void zend_begin_record_errors(void); -ZEND_API void zend_begin_record_errors_without_emitting(void); ZEND_API void zend_emit_recorded_errors(void); ZEND_API void zend_free_recorded_errors(void); END_EXTERN_C() diff --git a/Zend/zend_globals.h b/Zend/zend_globals.h index 83a7e7a62c4..1726dee9ac1 100644 --- a/Zend/zend_globals.h +++ b/Zend/zend_globals.h @@ -261,10 +261,8 @@ struct _zend_executor_globals { zend_long fiber_stack_size; /* If record_errors is enabled, all emitted diagnostics will be recorded, - * in addition to being processed as usual unless record_errors_without_emitting - * is enabled which supresses processing when the errors are recorded. */ + * in addition to being processed as usual. */ bool record_errors; - bool record_errors_without_emitting; uint32_t num_errors; zend_error_info **errors; diff --git a/ext/standard/tests/streams/gh8409.phpt b/ext/standard/tests/streams/gh8409.phpt deleted file mode 100644 index 4ca9c13b587..00000000000 --- a/ext/standard/tests/streams/gh8409.phpt +++ /dev/null @@ -1,20 +0,0 @@ ---TEST-- -GH-8409: Error in socket creation when error handler does not clean persistent connection ---FILE-- - ---EXPECT-- -DONE diff --git a/main/streams/transports.c b/main/streams/transports.c index 1ccc92671a6..de6b968868e 100644 --- a/main/streams/transports.c +++ b/main/streams/transports.c @@ -131,8 +131,6 @@ PHPAPI php_stream *_php_stream_xport_create(const char *name, size_t namelen, in (char*)name, namelen, persistent_id, options, flags, timeout, context STREAMS_REL_CC); - zend_begin_record_errors_without_emitting(); - if (stream) { php_stream_context_set(stream, context); @@ -183,9 +181,6 @@ PHPAPI php_stream *_php_stream_xport_create(const char *name, size_t namelen, in stream = NULL; } - zend_emit_recorded_errors(); - zend_free_recorded_errors(); - return stream; } From b8d07451d49f17cd0fc2c5d0e4e939edda456355 Mon Sep 17 00:00:00 2001 From: twosee Date: Sun, 14 Aug 2022 19:50:58 +0800 Subject: [PATCH 2/2] Re-fix GH-8409: SSL handshake timeout persistent connections hanging This fix is another solution to replace d0527427be57957157aec5e26a28899b380140df, use zend_try and zend_catch to make sure persistent stream will be released when error occurred. Closes GH-9332. --- NEWS | 2 + ext/standard/tests/streams/gh8409.phpt | 25 ++++++++++ main/streams/transports.c | 68 ++++++++++++++------------ 3 files changed, 65 insertions(+), 30 deletions(-) create mode 100644 ext/standard/tests/streams/gh8409.phpt diff --git a/NEWS b/NEWS index 2f5bd4626bd..ba942a7196e 100644 --- a/NEWS +++ b/NEWS @@ -29,6 +29,8 @@ PHP NEWS - Streams: . Fixed bug GH-8472 (The resource returned by stream_socket_accept may have incorrect metadata). (Jakub Zelenka) + . Fixed bug GH-8409 (SSL handshake timeout leaves persistent connections + hanging). (Jakub Zelenka, Twosee) 04 Aug 2022, PHP 8.0.22 diff --git a/ext/standard/tests/streams/gh8409.phpt b/ext/standard/tests/streams/gh8409.phpt new file mode 100644 index 00000000000..7f0a28b183f --- /dev/null +++ b/ext/standard/tests/streams/gh8409.phpt @@ -0,0 +1,25 @@ +--TEST-- +GH-8409: Error in socket creation when error handler does not clean persistent connection +--FILE-- + +--EXPECTF-- +Fatal error: stream_socket_client(): %s in %sgh8409.php on line %d +OK: persistent stream closed diff --git a/main/streams/transports.c b/main/streams/transports.c index fcc3f293300..2e17969b977 100644 --- a/main/streams/transports.c +++ b/main/streams/transports.c @@ -61,7 +61,8 @@ PHPAPI php_stream *_php_stream_xport_create(const char *name, size_t namelen, in php_stream_transport_factory factory = NULL; const char *p, *protocol = NULL; size_t n = 0; - int failed = 0; + bool failed = false; + bool bailout = false; zend_string *error_text = NULL; struct timeval default_timeout = { 0, 0 }; @@ -132,46 +133,50 @@ PHPAPI php_stream *_php_stream_xport_create(const char *name, size_t namelen, in context STREAMS_REL_CC); if (stream) { - php_stream_context_set(stream, context); + zend_try { + php_stream_context_set(stream, context); - if ((flags & STREAM_XPORT_SERVER) == 0) { - /* client */ + if ((flags & STREAM_XPORT_SERVER) == 0) { + /* client */ - if (flags & (STREAM_XPORT_CONNECT|STREAM_XPORT_CONNECT_ASYNC)) { - if (-1 == php_stream_xport_connect(stream, name, namelen, - flags & STREAM_XPORT_CONNECT_ASYNC ? 1 : 0, - timeout, &error_text, error_code)) { + if (flags & (STREAM_XPORT_CONNECT|STREAM_XPORT_CONNECT_ASYNC)) { + if (-1 == php_stream_xport_connect(stream, name, namelen, + flags & STREAM_XPORT_CONNECT_ASYNC ? 1 : 0, + timeout, &error_text, error_code)) { - ERR_RETURN(error_string, error_text, "connect() failed: %s"); + ERR_RETURN(error_string, error_text, "connect() failed: %s"); - failed = 1; - } - } - - } else { - /* server */ - if (flags & STREAM_XPORT_BIND) { - if (0 != php_stream_xport_bind(stream, name, namelen, &error_text)) { - ERR_RETURN(error_string, error_text, "bind() failed: %s"); - failed = 1; - } else if (flags & STREAM_XPORT_LISTEN) { - zval *zbacklog = NULL; - int backlog = 32; - - if (PHP_STREAM_CONTEXT(stream) && (zbacklog = php_stream_context_get_option(PHP_STREAM_CONTEXT(stream), "socket", "backlog")) != NULL) { - backlog = zval_get_long(zbacklog); + failed = true; } + } - if (0 != php_stream_xport_listen(stream, backlog, &error_text)) { - ERR_RETURN(error_string, error_text, "listen() failed: %s"); - failed = 1; + } else { + /* server */ + if (flags & STREAM_XPORT_BIND) { + if (0 != php_stream_xport_bind(stream, name, namelen, &error_text)) { + ERR_RETURN(error_string, error_text, "bind() failed: %s"); + failed = true; + } else if (flags & STREAM_XPORT_LISTEN) { + zval *zbacklog = NULL; + int backlog = 32; + + if (PHP_STREAM_CONTEXT(stream) && (zbacklog = php_stream_context_get_option(PHP_STREAM_CONTEXT(stream), "socket", "backlog")) != NULL) { + backlog = zval_get_long(zbacklog); + } + + if (0 != php_stream_xport_listen(stream, backlog, &error_text)) { + ERR_RETURN(error_string, error_text, "listen() failed: %s"); + failed = true; + } } } } - } + } zend_catch { + bailout = true; + } zend_end_try(); } - if (failed) { + if (failed || bailout) { /* failure means that they don't get a stream to play with */ if (persistent_id) { php_stream_pclose(stream); @@ -179,6 +184,9 @@ PHPAPI php_stream *_php_stream_xport_create(const char *name, size_t namelen, in php_stream_close(stream); } stream = NULL; + if (bailout) { + zend_bailout(); + } } return stream;