From: jsing <jsing@openbsd.org>
Date: Wed, 24 Jan 2024 14:02:52 +0000 (+0000)
Subject: Make it safe to delete entries from an lhash doall callback.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=91a7e77456f38c971cb36ccba7505d36e1ac8b71;p=openbsd

Make it safe to delete entries from an lhash doall callback.

Currently, the callback cannot safely delete entries as it could lead to
contraction of the hash table, which in turn could lead to doall skipping
entries (and that typically leads to memory leaks). The recommended
workaround is to reach in and fiddle with the hash table internals in
order to prevent contraction, call the doall function and then restore
the internals that were changed.

Rather than just improving our documentation, actually make it safe to
delete entries from an lhash doall callback by pausing contractions prior
to starting the callback loop, then restoring the down load factor and
triggering contraction once completed. This means that callers no longer
need access to change hash table internals in order to achieve this same
behaviour.

ok tb@
---

diff --git a/lib/libcrypto/lhash/lhash.c b/lib/libcrypto/lhash/lhash.c
index 3adec71ed67..81660419c7b 100644
--- a/lib/libcrypto/lhash/lhash.c
+++ b/lib/libcrypto/lhash/lhash.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: lhash.c,v 1.20 2023/07/07 13:40:44 beck Exp $ */
+/* $OpenBSD: lhash.c,v 1.21 2024/01/24 14:02:52 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -250,12 +250,21 @@ static void
 doall_util_fn(_LHASH *lh, int use_arg, LHASH_DOALL_FN_TYPE func,
     LHASH_DOALL_ARG_FN_TYPE func_arg, void *arg)
 {
-	int i;
 	LHASH_NODE *a, *n;
+	int down_load;
+	int i;
 
 	if (lh == NULL)
 		return;
 
+	/*
+	 * Disable contraction of the hash while walking, as some consumers use
+	 * it to delete hash entries. A better option would be to snapshot the
+	 * hash, making it insert safe as well.
+	 */
+	down_load = lh->down_load;
+	lh->down_load = 0;
+
 	/* reverse the order so we search from 'top to bottom'
 	 * We were having memory leaks otherwise */
 	for (i = lh->num_nodes - 1; i >= 0; i--) {
@@ -273,6 +282,10 @@ doall_util_fn(_LHASH *lh, int use_arg, LHASH_DOALL_FN_TYPE func,
 			a = n;
 		}
 	}
+
+	/* Restore down load factor and trigger contraction. */
+	lh->down_load = down_load;
+	contract(lh);
 }
 
 void
diff --git a/lib/libcrypto/man/lh_new.3 b/lib/libcrypto/man/lh_new.3
index c848eed825e..d672b4d2d85 100644
--- a/lib/libcrypto/man/lh_new.3
+++ b/lib/libcrypto/man/lh_new.3
@@ -1,4 +1,4 @@
-.\" $OpenBSD: lh_new.3,v 1.9 2022/03/31 17:27:17 naddy Exp $
+.\" $OpenBSD: lh_new.3,v 1.10 2024/01/24 14:02:52 jsing Exp $
 .\" full merge up to:
 .\" OpenSSL doc/crypto/lhash.pod 1bc74519 May 20 08:11:46 2016 -0400
 .\" selective merge up to:
@@ -118,7 +118,7 @@
 .\" copied and put under another distribution licence
 .\" [including the GNU Public Licence.]
 .\"
-.Dd $Mdocdate: March 31 2022 $
+.Dd $Mdocdate: January 24 2024 $
 .Dt LH_NEW 3
 .Os
 .Sh NAME
@@ -342,15 +342,8 @@ lh_STUFF_doall(hashtable, LHASH_DOALL_FN(STUFF_cleanup));
 lh_STUFF_free(hashtable);
 .Ed
 .Pp
-When doing this, be careful if you delete entries from the hash table in
-your callbacks: the table may decrease in size, moving the item that you
-are currently on down lower in the hash table \(em this could cause some
-entries to be skipped during the iteration.
-The second best solution to this problem is to set hash->down_load=0
-before you start (which will stop the hash table ever decreasing in
-size).
-The best solution is probably to avoid deleting items from the hash
-table inside a doall callback!
+A callback may delete entries from the hash table, however, it is
+not safe to insert new entries.
 .Pp
 .Fn lh_<type>_doall_arg
 is the same as