[retry] Process at most one timer's expiry in each call to retry_step()

Calling a timer's expiry method may cause arbitrary consequences,
including arbitrary modifications of the list of retry timers.
list_for_each_entry_safe() guards against only deletion of the current
list entry; it provides no protection against other list
modifications.  In particular, if a timer's expiry method causes the
subsequent timer in the list to be deleted, then the next loop
iteration will access a timer that may no longer exist.

This is a particularly nasty bug, since absolutely none of the
list-manipulation or reference-counting assertion checks will be
triggered.  (The first assertion failure happens on the next iteration
through list_for_each_entry(), showing that the list has become
corrupted but providing no clue as to when this happened.)

Fix by stopping traversal of the list of retry timers as soon as we
hit an expired timer.

Signed-off-by: Michael Brown <mcb30@ipxe.org>

1 files changed, 9 insertions, 3 deletions

diff --git a/src/net/retry.c b/src/net/retry.c
index b65bb57e..082be39b 100644
--- a/src/net/retry.c
+++ b/src/net/retry.c
@@ -180,14 +180,20 @@ static void timer_expired ( struct retry_timer *timer ) {
  */
 static void retry_step ( struct process *process __unused ) {
 	struct retry_timer *timer;
-	struct retry_timer *tmp;
 	unsigned long now = currticks();
 	unsigned long used;
 
-	list_for_each_entry_safe ( timer, tmp, &timers, list ) {
+	/* Process at most one timer expiry.  We cannot process
+	 * multiple expiries in one pass, because one timer expiring
+	 * may end up triggering another timer's deletion from the
+	 * list.
+	 */
+	list_for_each_entry ( timer, &timers, list ) {
 		used = ( now - timer->start );
-		if ( used >= timer->timeout )
+		if ( used >= timer->timeout ) {
 			timer_expired ( timer );
+			break;
+		}
 	}
 }