Merge branch '4030-fix-UAF-in-isc_httpd' into 'main'

Fix potential UAF when shutting down isc_httpd

Closes #4030

See merge request isc-projects/bind9!7865

.clang-format

@@ -78,4 +78,4 @@ PenaltyBreakString: 80
 PenaltyExcessCharacter: 100
 Standard: Cpp11
 ContinuationIndentWidth: 8
-ForEachMacros: [ 'cds_lfs_for_each', 'cds_lfs_for_each_safe', 'cds_list_for_each_entry_safe' ]
+ForEachMacros: [ 'cds_lfs_for_each', 'cds_lfs_for_each_safe', 'cds_list_for_each_entry_safe', 'ISC_LIST_FOREACH', 'ISC_LIST_FOREACH_SAFE' ]

CHANGES

@@ -1,3 +1,8 @@
+6158.	[func]		Add ISC_LIST_FOREACH() and ISC_LIST_FOREACH_SAFE()
+			to walk the ISC_LIST() in a unified manner and use
+			the safe macro to fix the potential UAF when shutting
+			down the isc_httpd. [GL #4031]
+
 6157.	[bug]		When removing delegations in an OPTOUT range
 			empty-non-terminal NSEC3 records generated by
 			those delegations where not removed. [GL #4027]

lib/isc/httpd.c

@@ -20,6 +20,7 @@
 
 #include <isc/buffer.h>
 #include <isc/httpd.h>
+#include <isc/list.h>
 #include <isc/mem.h>
 #include <isc/netmgr.h>
 #include <isc/refcount.h>
@@ -278,8 +279,6 @@ httpdmgr_detach(isc_httpdmgr_t **httpdmgrp) {
 
 static void
 destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) {
-	isc_httpdurl_t *url;
-
 	isc_refcount_destroy(&httpdmgr->references);
 
 	LOCK(&httpdmgr->lock);
@@ -297,12 +296,11 @@ destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) {
 	 * Clear out the list of all actions we know about.  Just free the
 	 * memory.
 	 */
-	url = ISC_LIST_HEAD(httpdmgr->urls);
+	isc_httpdurl_t *url, *next;
-	while (url != NULL) {
+	ISC_LIST_FOREACH_SAFE (httpdmgr->urls, url, link, next) {
 		isc_mem_free(httpdmgr->mctx, url->url);
 		ISC_LIST_UNLINK(httpdmgr->urls, url, link);
 		isc_mem_put(httpdmgr->mctx, url, sizeof(isc_httpdurl_t));
-		url = ISC_LIST_HEAD(httpdmgr->urls);
 	}
 
 	UNLOCK(&httpdmgr->lock);
@@ -746,12 +744,10 @@ prepare_response(isc_httpdmgr_t *mgr, isc_httpd_t *httpd,
 	}
 
 	LOCK(&mgr->lock);
-	url = ISC_LIST_HEAD(mgr->urls);
+	ISC_LIST_FOREACH (mgr->urls, url, link) {
-	while (url != NULL) {
 		if (strncmp(path, url->url, path_len) == 0) {
 			break;
 		}
-		url = ISC_LIST_NEXT(url, link);
 	}
 	UNLOCK(&mgr->lock);
 
@@ -934,7 +930,6 @@ close_readhandle:
 void
 isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) {
 	isc_httpdmgr_t *httpdmgr = NULL;
-	isc_httpd_t *httpd = NULL;
 
 	REQUIRE(httpdmgrp != NULL);
 	REQUIRE(VALID_HTTPDMGR(*httpdmgrp));
@@ -947,13 +942,12 @@ isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) {
 	LOCK(&httpdmgr->lock);
 	httpdmgr->flags |= ISC_HTTPDMGR_SHUTTINGDOWN;
 
-	httpd = ISC_LIST_HEAD(httpdmgr->running);
+	isc_httpd_t *httpd, *next;
-	while (httpd != NULL) {
+	ISC_LIST_FOREACH_SAFE (httpdmgr->running, httpd, link, next) {
 		if (httpd->handle != NULL) {
 			httpd_request(httpd->handle, ISC_R_SUCCESS, NULL,
 				      httpd);
 		}
-		httpd = ISC_LIST_NEXT(httpd, link);
 	}
 	UNLOCK(&httpdmgr->lock);
 

lib/isc/include/isc/list.h

@@ -227,3 +227,17 @@
 		INSIST(ISC_LIST_EMPTY(dest));   \
 		ISC_LIST_MOVEUNSAFE(dest, src); \
 	}
+
+/* clang-format off */
+#define ISC_LIST_FOREACH(list, elt, link)	\
+	for (elt = ISC_LIST_HEAD(list);		\
+	     elt != NULL;			\
+	     elt = ISC_LIST_NEXT(elt, link))
+/* clang-format on */
+
+/* clang-format off */
+#define ISC_LIST_FOREACH_SAFE(list, elt, link, next)						\
+	for (elt = ISC_LIST_HEAD(list), next = (elt != NULL) ? ISC_LIST_NEXT(elt, link) : NULL;	\
+	     elt != NULL;									\
+	     elt = next, next = (elt != NULL) ? ISC_LIST_NEXT(elt, link) : NULL)
+/* clang-format on */