ospfd: Do not turn on write thread unless we have something in it

I am rarely seeing this crash:

r2: ospfd crashed. Core file found - Backtrace follows:
[New LWP 32748]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
Core was generated by `/usr/lib/frr/ospfd'.
Program terminated with signal SIGABRT, Aborted.
2019-08-29 15:59:36,149 ERROR: assert failed at "test_ospf_sr_topo1/test_memory_leak":

Which translates to this code:

	node = listhead(ospf->oi_write_q);
	assert(node);
	oi = listgetdata(node);
	assert(oi);

So if we get into ospf_write without anything on the oi_write_q
we are stopping the program.

This is happening because in ospf_ls_upd_queue_send we are calling
ospf_write.  Imagine that we have a interface already on the on_write_q
and then ospf_write handles the packet send for all functions.  We
are not clearing the t_write thread and we are popping and causing
a crash.

Additionally modify OSPF_ISM_WRITE_ON(O) to not just blindly
turn on the t_write thread.  Only do so if we have data.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

ospfd: Remove redundant asserts

assert(oi) is impossible all listgetdata(node) directly proceeding
it already asserts here, besides a node cannot be created
with a null pointer!

If list_isempty is called directly before the listhead call
it is impossilbe that we do not have a valid pointer here.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

2 files changed, 20 insertions, 7 deletions

diff --git a/ospfd/ospf_ism.h b/ospfd/ospf_ism.h
index 5ae99ab32..8d2140369 100644
--- a/ospfd/ospf_ism.h
+++ b/ospfd/ospf_ism.h
@@ -53,8 +53,9 @@
 			listnode_add((O)->oi_write_q, oi);                     \
 			oi->on_write_q = 1;                                    \
 		}                                                              \
-		thread_add_write(master, ospf_write, (O), (O)->fd,             \
-				 &(O)->t_write);                               \
+		if (!list_isempty((O)->oi_write_q))                            \
+			thread_add_write(master, ospf_write, (O), (O)->fd,     \
+					 &(O)->t_write);                       \
 	} while (0)
 
 /* Macro for OSPF ISM timer turn on. */
diff --git a/ospfd/ospf_packet.c b/ospfd/ospf_packet.c
index 62b044479..c69218fb5 100644
--- a/ospfd/ospf_packet.c
+++ b/ospfd/ospf_packet.c
@@ -698,12 +698,9 @@ static int ospf_write(struct thread *thread)
 		return -1;
 	}
 
-	ospf->t_write = NULL;
-
 	node = listhead(ospf->oi_write_q);
 	assert(node);
 	oi = listgetdata(node);
-	assert(oi);
 
 #ifdef WANT_OSPF_WRITE_FRAGMENT
 	/* seed ipid static with low order bits of time */
@@ -906,9 +903,7 @@ static int ospf_write(struct thread *thread)
 		/* Setup to service from the head of the queue again */
 		if (!list_isempty(ospf->oi_write_q)) {
 			node = listhead(ospf->oi_write_q);
-			assert(node);
 			oi = listgetdata(node);
-			assert(oi);
 		}
 	}
 
@@ -4062,6 +4057,23 @@ static void ospf_ls_upd_queue_send(struct ospf_interface *oi,
 			oi->on_write_q = 1;
 		}
 		ospf_write(&os_packet_thd);
+		/*
+		 * We are fake calling ospf_write with a fake
+		 * thread.  Imagine that we have oi_a already
+		 * enqueued and we have turned on the write
+		 * thread(t_write).
+		 * Now this function calls this for oi_b
+		 * so the on_write_q has oi_a and oi_b on
+		 * it, ospf_write runs and clears the packets
+		 * for both oi_a and oi_b.  Removing them from
+		 * the on_write_q.  After this thread of execution
+		 * finishes we will execute the t_write thread
+		 * with nothing in the on_write_q causing an
+		 * assert.  So just make sure that the t_write
+		 * is actually turned off.
+		 */
+		if (list_isempty(oi->ospf->oi_write_q))
+			OSPF_TIMER_OFF(oi->ospf->t_write);
 	} else {
 		/* Hook thread to write packet. */
 		OSPF_ISM_WRITE_ON(oi->ospf);