[nathelper] fix bad logic regarding the timer list

(cherry picked from commit a86d2c4)

modules/nathelper/nathelper.c

@@ -1657,26 +1657,58 @@ ping_checker_timer(unsigned int ticks, void *timer_idx)
 	ctime=now;
 
 	/* detect cells for which threshold has been exceeded */
+
+	/* something very fishy here */
 	lock_get(&table->timer_list.mutex);
 	first = last = table->timer_list.first;
 
-	while (last != table->timer_list.last
-			&& ((ctime - last->timestamp) > ping_threshold))
+	if (table->timer_list.first == NULL || table->timer_list.last == NULL) {
+		/* nothing to do here - empty list */
+		goto out_release_lock;
+	}
+
+	/* only valid elements */
+	if (table->timer_list.first == table->timer_list.last
+			&& ((ctime-last->timestamp) < ping_threshold)) {
+		goto out_release_lock;
+	}
+
+	/* at least one invalid element
+	 *
+	 */
+	prev=NULL;
+	while (last != LIST_END_CELL
+			&& ((ctime-last->timestamp)>ping_threshold)) {
+		prev = last;
 		last = last->tnext;
+	}
 
-	if (last == table->timer_list.last)
-		table->timer_list.first = table->timer_list.last = NULL;
-	else
-		table->timer_list.first = last->tnext;
 
-	/* last will be the first valid element in timer list */
-	last = table->timer_list.first;
+	if (prev != NULL) {
+		/* have at least 1 element to remove */
+		if (last == LIST_END_CELL) {
+			/* all the list contains expired elements */
+			table->timer_list.first = table->timer_list.last = NULL;
+		} else {
+			/* still have non expired elements
+			 * move list start to first valid one */
+			table->timer_list.first = last;
+		}
 
-	lock_release(&table->timer_list.mutex);
+		last = prev;
+		last->tnext = LIST_END_CELL;
+	} else {
+		/* nothing to remove - timer list remains the same */
+		goto out_release_lock;;
+	}
 
+	lock_release(&table->timer_list.mutex);
 
+	/*
+	 * getting here means we have at least one element in the list to remove
+	 */
 	cell = first;
-	while (cell &&  cell != last) {
+	do {
 		if (cell->timestamp == 0) {
 			/* ping confirmed and unlinked from hash; only free the cell */
 			prev = cell;
@@ -1703,12 +1735,12 @@ ping_checker_timer(unsigned int ticks, void *timer_idx)
 
 			remove_given_cell(cell, &table->entries[cell->hash_id]);
 
+			/* we put the lock on cell which now moved into prev */
+			unlock_hash(cell->hash_id);
+
 			prev = cell;
 			cell = cell->tnext;
 
-			/* we put the lock on cell which now moved into prev */
-			unlock_hash(prev->hash_id);
-
 			shm_free(prev);
 
 			if (ul.delete_ucontact_from_id &&
@@ -1721,12 +1753,15 @@ ping_checker_timer(unsigned int ticks, void *timer_idx)
 			cell = cell->tnext;
 
 			/* allow cell to be reintroduced in timer list */
-			prev->tnext = NULL;
+			prev->tnext = FREE_CELL;
 
 			/* we put the lock on cell which now moved into prev */
 			unlock_hash(prev->hash_id);
 		}
-	}
+	} while (cell != LIST_END_CELL);
+
+out_release_lock:
+	lock_release(&table->timer_list.mutex);
 }
 
 

modules/nathelper/sip_pinger.h

@@ -47,6 +47,8 @@
 
 #define BSTART ";branch="
 
+#define LIST_END_CELL ((struct ping_cell*)-1) /* this cell is the end of the list */
+#define FREE_CELL NULL /* this cell is not in the timer list */
 
 /* helping macros for building SIP PING ping request */
 #define append_str( _p, _s) \
@@ -208,13 +210,17 @@ build_branch(char *branch, int *size,
 {
 
 	int hash_id, ret, label;
+	time_t timestamp;
 	struct ping_cell *p_cell;
 	struct nh_table *htable;
 
 	/* we want all contact pings from a contact in one bucket*/
 	hash_id = core_hash(curi, 0, 0) & (NH_TABLE_ENTRIES-1);
 
 	if (rm_on_to) {
+		/* get the time before the lock - we may wait a little bit
+		 * on this lock */
+		timestamp=now;
 		lock_hash(hash_id);
 		if ((p_cell=get_cell(hash_id, contact_id))==NULL) {
 			if (0 == (p_cell = build_p_cell(hash_id, d, contact_id))) {
@@ -224,20 +230,23 @@ build_branch(char *branch, int *size,
 			insert_into_hash(p_cell);
 		}
 
-		p_cell->timestamp = now;
+		p_cell->timestamp = timestamp;
 		unlock_hash(hash_id);
 
 		htable = get_htable();
 
 		/* put the cell in timer list */
 		lock_get(&htable->timer_list.mutex);
-		if (!p_cell->tnext) {
+
+		if (p_cell->tnext == FREE_CELL) {
 			if (!htable->timer_list.first) {
 				htable->timer_list.first = htable->timer_list.last = p_cell;
 			} else {
 				htable->timer_list.last->tnext = p_cell;
 				htable->timer_list.last = p_cell;
 			}
+			/* this cell will be the end of the list */
+			p_cell->tnext = LIST_END_CELL;
 		}
 
 		/* we get the label that assures us that the via is unique */