From d1bc0d0b51bb8145c4d1597a39f72d85b28f8b35 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 22 Jul 2022 16:28:03 +0100 Subject: [PATCH] s3/smbd: Use after free when iterating smbd_server_connection->connections Change conn_free() to just use a destructor. We now catch any other places where we may have forgetten to call conn_free() - it's implicit on talloc_free(conn). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128 Based on code from Noel Power . Signed-off-by: Jeremy Allison Reviewed-by: Noel Power Autobuild-User(master): Noel Power Autobuild-Date(master): Wed Aug 17 09:54:06 UTC 2022 on sn-devel-184 (cherry picked from commit f92bacbe216d2d74ea3ccf3fe0df5c1cc9860996) --- source3/smbd/conn.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/source3/smbd/conn.c b/source3/smbd/conn.c index 044242d5697..776d7af4c12 100644 --- a/source3/smbd/conn.c +++ b/source3/smbd/conn.c @@ -24,6 +24,25 @@ #include "smbd/globals.h" #include "lib/util/bitmap.h" +static void conn_free_internal(connection_struct *conn); + +/**************************************************************************** + * Remove a conn struct from conn->sconn->connections + * if not already done. +****************************************************************************/ + +static int conn_struct_destructor(connection_struct *conn) +{ + if (conn->sconn != NULL) { + DLIST_REMOVE(conn->sconn->connections, conn); + SMB_ASSERT(conn->sconn->num_connections > 0); + conn->sconn->num_connections--; + conn->sconn = NULL; + } + conn_free_internal(conn); + return 0; +} + /**************************************************************************** Return the number of open connections. ****************************************************************************/ @@ -115,6 +134,11 @@ connection_struct *conn_new(struct smbd_server_connection *sconn) DLIST_ADD(sconn->connections, conn); sconn->num_connections++; + /* + * Catches the case where someone forgets to call + * conn_free(). + */ + talloc_set_destructor(conn, conn_struct_destructor); return conn; } @@ -212,7 +236,6 @@ static void conn_free_internal(connection_struct *conn) free_namearray(conn->aio_write_behind_list); ZERO_STRUCTP(conn); - talloc_destroy(conn); } /**************************************************************************** @@ -221,16 +244,7 @@ static void conn_free_internal(connection_struct *conn) void conn_free(connection_struct *conn) { - if (conn->sconn == NULL) { - conn_free_internal(conn); - return; - } - - DLIST_REMOVE(conn->sconn->connections, conn); - SMB_ASSERT(conn->sconn->num_connections > 0); - conn->sconn->num_connections--; - - conn_free_internal(conn); + TALLOC_FREE(conn); } /*