Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
May 21, 2023 11:24PM
Hello!

On Sun, May 21, 2023 at 01:20:13PM +0400, Roman Arutyunyan wrote:

> # HG changeset patch
> # User Roman Arutyunyan <arut@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).

Note that "Prodded by..." is generally used for changes which
aren't real bug fixes (that is, for mostly style changes like the
one I've submitted for sockaddr initialization). For bugs found
by Coverity which are indeed bugs we usually prefer "Found by ...".

>
> 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);
> }
>
>

I'm not really familiar with the code, but looks good to me.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

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

Roman Arutyunyan 295 May 21, 2023 05:22AM

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

Maxim Dounin 93 May 21, 2023 11:24PM

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

Sergey Kandaurov 91 May 22, 2023 07:30AM

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

Roman Arutyunyan 95 May 22, 2023 08:02AM

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

Sergey Kandaurov 123 May 22, 2023 08:46AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 315
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready