[PATCH] QUIC: fixed post-close use-after-free

[Roman Arutyunyan]

# HG changeset patch
# User Roman Arutyunyan <arut at nginx.com>
# Date 1684659758 -14400
#      Sun May 21 13:02:38 2023 +0400
# Node ID 321b69f47a0a9874a063e464d13706339a706bb8
# Parent  235d482ef6bc8c40a956b2413865d42c94e0fc05
QUIC: fixed post-close use-after-free.

Previously, ngx_quic_close_connection() could be called in a way that QUIC
connection was accessed after the call.  In most cases the connection is not
closed right away, but close timeout is scheduled.  However, it's not always
the case.  Also, if the close process started earlier for a different reason,
calling ngx_quic_close_connection() may actually close the connection.  The
connection object should not be accessed after that.  Particularly, accessing
c->pool can result in null pointer dereference.

Now, when possible, return statement is added to eliminate post-close connection
object access.  In other places ngx_quic_close_connection() is substituted with
posting close event.

Prodded by Coverity (CID 1530402).

diff --git a/src/event/quic/ngx_event_quic.c b/src/event/quic/ngx_event_quic.c
--- a/src/event/quic/ngx_event_quic.c
+++ b/src/event/quic/ngx_event_quic.c
@@ -844,7 +844,7 @@ ngx_quic_handle_packet(ngx_connection_t 
                               "quic stateless reset packet detected");
 
                 qc->draining = 1;
-                ngx_quic_close_connection(c, NGX_OK);
+                ngx_post_event(&qc->close, &ngx_posted_events);
 
                 return NGX_OK;
             }
@@ -1390,7 +1390,7 @@ ngx_quic_handle_frames(ngx_connection_t 
 
     if (do_close) {
         qc->draining = 1;
-        ngx_quic_close_connection(c, NGX_OK);
+        ngx_post_event(&qc->close, &ngx_posted_events);
     }
 
     if (pkt->path != qc->path && nonprobing) {
diff --git a/src/event/quic/ngx_event_quic_ack.c b/src/event/quic/ngx_event_quic_ack.c
--- a/src/event/quic/ngx_event_quic_ack.c
+++ b/src/event/quic/ngx_event_quic_ack.c
@@ -806,6 +806,7 @@ void ngx_quic_lost_handler(ngx_event_t *
 
     if (ngx_quic_detect_lost(c, NULL) != NGX_OK) {
         ngx_quic_close_connection(c, NGX_ERROR);
+        return;
     }
 
     ngx_quic_connstate_dbg(c);
diff --git a/src/event/quic/ngx_event_quic_streams.c b/src/event/quic/ngx_event_quic_streams.c
--- a/src/event/quic/ngx_event_quic_streams.c
+++ b/src/event/quic/ngx_event_quic_streams.c
@@ -1084,7 +1084,8 @@ ngx_quic_stream_cleanup_handler(void *da
 {
     ngx_connection_t *c = data;
 
-    ngx_quic_stream_t  *qs;
+    ngx_quic_stream_t      *qs;
+    ngx_quic_connection_t  *qc;
 
     qs = c->quic;
 
@@ -1092,16 +1093,23 @@ ngx_quic_stream_cleanup_handler(void *da
                    "quic stream id:0x%xL cleanup", qs->id);
 
     if (ngx_quic_shutdown_stream(c, NGX_RDWR_SHUTDOWN) != NGX_OK) {
-        ngx_quic_close_connection(c, NGX_ERROR);
-        return;
+        goto failed;
     }
 
     qs->connection = NULL;
 
     if (ngx_quic_close_stream(qs) != NGX_OK) {
-        ngx_quic_close_connection(c, NGX_ERROR);
-        return;
+        goto failed;
     }
+
+    return;
+
+failed:
+
+    qc = ngx_quic_get_connection(qs->parent);
+    qc->error = NGX_QUIC_ERR_INTERNAL_ERROR;
+
+    ngx_post_event(&qc->close, &ngx_posted_events);
 }