From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Tue, 9 Jun 2015 05:13:34 +0000 (-0400)
Subject: Fix issue with fr_cursor_remove
X-Git-Tag: release_3_0_9~217
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dfee6bc88ac0fc186fab5e811c96aea98bf525d3;p=thirdparty%2Ffreeradius-server.git

Fix issue with fr_cursor_remove

Caused unexpected behaviour removing attributes in an fr_cursor_next loop.

cursor->current would be advanced past the VP we were deleting. Which meant fr_cursor_next would skip past the next attribute.

Now we set current to be the vp before the one we're deleting, so fr_cursor_next works as expected.
---

diff --git a/src/lib/cursor.c b/src/lib/cursor.c
index f7ed12a0921..bff895859ac 100644
--- a/src/lib/cursor.c
+++ b/src/lib/cursor.c
@@ -379,6 +379,20 @@ void fr_cursor_merge(vp_cursor_t *cursor, VALUE_PAIR *add)
  *
  * @todo this is really inefficient and should be fixed...
  *
+ * The current VP will be set to the one before the VP being removed,
+ * this is so the commonly used check and remove loop (below) works
+ * as expected.
+ @code {.c}
+   for (vp = fr_cursor_init(&cursor, head);
+        vp;
+        vp = fr_cursor_next(&cursor) {
+        if (<condition>) {
+            vp = fr_cursor_remove(&cursor);
+            talloc_free(vp);
+        }
+   }
+ @endcode
+ *
  * @addtogroup module_safe
  *
  * @param cursor to remove the current pair from.
@@ -386,30 +400,49 @@ void fr_cursor_merge(vp_cursor_t *cursor, VALUE_PAIR *add)
  */
 VALUE_PAIR *fr_cursor_remove(vp_cursor_t *cursor)
 {
-	VALUE_PAIR *vp, **last;
+	VALUE_PAIR *vp, *before;
 
 	if (!fr_assert(cursor->first)) return NULL;	/* cursor must have been initialised */
 
 	vp = cursor->current;
 	if (!vp) return NULL;
 
-	last = cursor->first;
-	while (*last != vp) last = &(*last)->next;
+	/*
+	 *	Where VP is head of the list
+	 */
+	if (*(cursor->first) == vp) {
+		*(cursor->first) = vp->next;
+		cursor->current = vp->next;
+		cursor->next = vp->next ? vp->next->next : NULL;
+		goto fixup;
+	}
 
-	fr_cursor_next(cursor);   /* Advance the cursor past the one were about to delete */
+	/*
+	 *	Where VP is not head of the list
+	 */
+	before = *(cursor->first);
+	if (!before) return NULL;
 
-	*last = vp->next;
-	vp->next = NULL;
+	/*
+	 *	Find the VP immediately preceding the one being removed
+	 */
+	while (before->next != vp) before = before->next;
+
+	cursor->next = before->next = vp->next;	/* close the gap */
+	cursor->current = before;		/* current jumps back one, but this is usually desirable */
+
+fixup:
+	vp->next = NULL;			/* limit scope of pairfree() */
 
 	/*
 	 *	Fixup cursor->found if we removed the VP it was referring to
 	 */
-	if (vp == cursor->found) cursor->found = *last;
+	if (vp == cursor->found) cursor->found = cursor->current;
 
 	/*
 	 *	Fixup cursor->last if we removed the VP it was referring to
 	 */
-	if (vp == cursor->last) cursor->last = *last;
+	if (vp == cursor->last) cursor->last = cursor->current;
 	return vp;
 }