Blame SOURCES/0015-Ticket-49174-nunc-stans-can-not-use-negative-timeout.patch

6f51e1
From 4f90e73538f1faf101733fcd95392bb77ba9467c Mon Sep 17 00:00:00 2001
6f51e1
From: William Brown <firstyear@redhat.com>
6f51e1
Date: Wed, 22 Mar 2017 14:10:11 +1000
6f51e1
Subject: [PATCH] Ticket 49174 - nunc-stans can not use negative timeout
6f51e1
6f51e1
Bug Description:  FreeIPA regularly sets up service accounts with
6f51e1
an nsIdleTimeout of -1. As a result of an issue with NS and libevent
6f51e1
this would cause an instant timeout and disconnect of the service
6f51e1
account.
6f51e1
6f51e1
Fix Description:  Correctly check that jobs are registered to NS.
6f51e1
Add validation to NS for negative timeouts. During the job registration,
6f51e1
we force the timeout to be a valid value.
6f51e1
6f51e1
https://pagure.io/389-ds-base/issue/49174
6f51e1
6f51e1
Author: wibrown
6f51e1
6f51e1
Review by: mreynolds(Thanks!!!)
6f51e1
6f51e1
Signed-off-by: Mark Reynolds <mreynolds@redhat.com>
6f51e1
---
6f51e1
 ldap/servers/slapd/daemon.c           | 39 ++++++++++++++++++++++++++++-------
6f51e1
 src/nunc-stans/ns/ns_event_fw_event.c |  8 -------
6f51e1
 src/nunc-stans/ns/ns_thrpool.c        | 16 ++++++++++++++
6f51e1
 src/nunc-stans/test/test_nuncstans.c  | 20 ++++++++++++++++++
6f51e1
 4 files changed, 68 insertions(+), 15 deletions(-)
6f51e1
6f51e1
diff --git a/ldap/servers/slapd/daemon.c b/ldap/servers/slapd/daemon.c
6f51e1
index e17a858..a4ea4c0 100644
6f51e1
--- a/ldap/servers/slapd/daemon.c
6f51e1
+++ b/ldap/servers/slapd/daemon.c
6f51e1
@@ -1891,15 +1891,32 @@ ns_connection_post_io_or_closing(Connection *conn)
6f51e1
 			tv.tv_usec = slapd_wakeup_timer * 1000;
6f51e1
 			conn->c_ns_close_jobs++; /* now 1 active closure job */
6f51e1
 			connection_acquire_nolock_ext(conn, 1 /* allow acquire even when closing */); /* event framework now has a reference */
6f51e1
-			ns_add_timeout_job(conn->c_tp, &tv, NS_JOB_TIMER,
6f51e1
+			PRStatus job_result = ns_add_timeout_job(conn->c_tp, &tv, NS_JOB_TIMER,
6f51e1
 					   ns_handle_closure, conn, NULL);
6f51e1
-			slapi_log_err(SLAPI_LOG_CONNS, "ns_connection_post_io_or_closing", "post closure job "
6f51e1
-				"for conn %" NSPRIu64 " for fd=%d\n", conn->c_connid, conn->c_sd);
6f51e1
+#ifdef DEBUG
6f51e1
+			PR_ASSERT(job_result == PR_SUCCESS);
6f51e1
+#endif
6f51e1
+			if (job_result != PR_SUCCESS) {
6f51e1
+				slapi_log_err(SLAPI_LOG_WARNING, "ns_connection_post_io_or_closing", "post closure job "
6f51e1
+					"for conn %" NSPRIu64 " for fd=%d failed to be added to event queue\n", conn->c_connid, conn->c_sd);
6f51e1
+			} else {
6f51e1
+				slapi_log_err(SLAPI_LOG_CONNS, "ns_connection_post_io_or_closing", "post closure job "
6f51e1
+					"for conn %" NSPRIu64 " for fd=%d\n", conn->c_connid, conn->c_sd);
6f51e1
+			}
6f51e1
 			
6f51e1
 		}
6f51e1
 	} else {
6f51e1
 		/* process event normally - wait for I/O until idletimeout */
6f51e1
-		tv.tv_sec = conn->c_idletimeout;
6f51e1
+		/* With nunc-stans there is a quirk. When we have idleTimeout of -1
6f51e1
+		 * which is set on some IPA bind dns for infinite, this causes libevent 
6f51e1
+		 * to *instantly* timeout. So if we detect < 0, we set 0 to this timeout, to
6f51e1
+		 * catch all possible times that an admin could set.
6f51e1
+		 */
6f51e1
+		if (conn->c_idletimeout < 0) {
6f51e1
+			tv.tv_sec = 0;
6f51e1
+		} else {
6f51e1
+			tv.tv_sec = conn->c_idletimeout;
6f51e1
+		}
6f51e1
 		tv.tv_usec = 0;
6f51e1
 #ifdef DEBUG
6f51e1
 		PR_ASSERT(0 == connection_acquire_nolock(conn));
6f51e1
@@ -1913,11 +1930,19 @@ ns_connection_post_io_or_closing(Connection *conn)
6f51e1
 			return;
6f51e1
 		}
6f51e1
 #endif
6f51e1
-		ns_add_io_timeout_job(conn->c_tp, conn->c_prfd, &tv,
6f51e1
+		PRStatus job_result = ns_add_io_timeout_job(conn->c_tp, conn->c_prfd, &tv,
6f51e1
 				      NS_JOB_READ|NS_JOB_PRESERVE_FD,
6f51e1
 				      ns_handle_pr_read_ready, conn, NULL);
6f51e1
-		slapi_log_err(SLAPI_LOG_CONNS, "ns_connection_post_io_or_closing", "post I/O job for "
6f51e1
-			"conn %" NSPRIu64 " for fd=%d\n", conn->c_connid, conn->c_sd);
6f51e1
+#ifdef DEBUG
6f51e1
+		PR_ASSERT(job_result == PR_SUCCESS);
6f51e1
+#endif
6f51e1
+		if (job_result != PR_SUCCESS) {
6f51e1
+			slapi_log_err(SLAPI_LOG_WARNING, "ns_connection_post_io_or_closing", "post I/O job for "
6f51e1
+				"conn %" NSPRIu64 " for fd=%d failed to be added to event queue\n", conn->c_connid, conn->c_sd);
6f51e1
+		} else {
6f51e1
+			slapi_log_err(SLAPI_LOG_CONNS, "ns_connection_post_io_or_closing", "post I/O job for "
6f51e1
+				"conn %" NSPRIu64 " for fd=%d\n", conn->c_connid, conn->c_sd);
6f51e1
+		}
6f51e1
 	}
6f51e1
 #endif
6f51e1
 }
6f51e1
diff --git a/src/nunc-stans/ns/ns_event_fw_event.c b/src/nunc-stans/ns/ns_event_fw_event.c
6f51e1
index 3acbaf7..76936de 100644
6f51e1
--- a/src/nunc-stans/ns/ns_event_fw_event.c
6f51e1
+++ b/src/nunc-stans/ns/ns_event_fw_event.c
6f51e1
@@ -48,7 +48,6 @@ typedef struct event ns_event_fw_sig_t;
6f51e1
 #include "ns_event_fw.h"
6f51e1
 #include <syslog.h>
6f51e1
 
6f51e1
-
6f51e1
 static void
6f51e1
 event_logger_cb(int severity, const char *msg)
6f51e1
 {
6f51e1
@@ -248,13 +247,6 @@ ns_event_fw_mod_io(
6f51e1
     }
6f51e1
     if (events) {
6f51e1
         job->ns_event_fw_fd->ev_events = events;
6f51e1
-
6f51e1
-#ifdef DEBUG_FSM
6f51e1
-        /* REALLY make sure that we aren't being re-added */
6f51e1
-        if (event_pending(job->ns_event_fw_fd, events, tv)) {
6f51e1
-            abort();
6f51e1
-        }
6f51e1
-#endif
6f51e1
         event_add(job->ns_event_fw_fd, tv);
6f51e1
     } else {
6f51e1
         /* setting the job_type to remove IO events will remove it from the event system */
6f51e1
diff --git a/src/nunc-stans/ns/ns_thrpool.c b/src/nunc-stans/ns/ns_thrpool.c
6f51e1
index a867b39..9d87384 100644
6f51e1
--- a/src/nunc-stans/ns/ns_thrpool.c
6f51e1
+++ b/src/nunc-stans/ns/ns_thrpool.c
6f51e1
@@ -180,6 +180,14 @@ ns_thrpool_is_event_shutdown(struct ns_thrpool_t *tp)
6f51e1
     return result;
6f51e1
 }
6f51e1
 
6f51e1
+static int32_t
6f51e1
+validate_event_timeout(struct timeval *tv) {
6f51e1
+    if (tv->tv_sec < 0 || tv->tv_usec < 0) {
6f51e1
+        /* If we get here, you have done something WRONG */
6f51e1
+        return 1;
6f51e1
+    }
6f51e1
+    return 0;
6f51e1
+}
6f51e1
 
6f51e1
 static void
6f51e1
 job_queue_cleanup(void *arg) {
6f51e1
@@ -864,6 +872,10 @@ ns_add_timeout_job(ns_thrpool_t *tp, struct timeval *tv, ns_job_type_t job_type,
6f51e1
         return PR_FAILURE;
6f51e1
     }
6f51e1
 
6f51e1
+    if (validate_event_timeout(tv)) {
6f51e1
+        return PR_FAILURE;
6f51e1
+    }
6f51e1
+
6f51e1
     /* get an event context for a timer job */
6f51e1
     _job = alloc_timeout_context(tp, tv, job_type, func, data);
6f51e1
     if (!_job) {
6f51e1
@@ -900,6 +912,10 @@ ns_add_io_timeout_job(ns_thrpool_t *tp, PRFileDesc *fd, struct timeval *tv,
6f51e1
         return PR_FAILURE;
6f51e1
     }
6f51e1
 
6f51e1
+    if (validate_event_timeout(tv)) {
6f51e1
+        return PR_FAILURE;
6f51e1
+    }
6f51e1
+
6f51e1
     /* Don't allow an accept job to be run outside of the event thread.
6f51e1
      * We do this so a listener job won't shut down while still processing
6f51e1
      * current connections in other threads.
6f51e1
diff --git a/src/nunc-stans/test/test_nuncstans.c b/src/nunc-stans/test/test_nuncstans.c
6f51e1
index 8eef9e6..2795302 100644
6f51e1
--- a/src/nunc-stans/test/test_nuncstans.c
6f51e1
+++ b/src/nunc-stans/test/test_nuncstans.c
6f51e1
@@ -385,6 +385,23 @@ ns_job_signal_cb_test(void **state)
6f51e1
     assert_int_equal(ns_job_done(job), 0);
6f51e1
 }
6f51e1
 
6f51e1
+/*
6f51e1
+ * Test that given a timeout of -1, we fail to create a job.
6f51e1
+ */
6f51e1
+
6f51e1
+static void
6f51e1
+ns_job_neg_timeout_test(void **state)
6f51e1
+{
6f51e1
+    struct ns_thrpool_t *tp = *state;
6f51e1
+
6f51e1
+    struct timeval tv = { -1, 0 };
6f51e1
+
6f51e1
+    PR_ASSERT(PR_FAILURE == ns_add_io_timeout_job(tp, 0, &tv, NS_JOB_THREAD, ns_init_do_nothing_cb, NULL, NULL));
6f51e1
+
6f51e1
+    PR_ASSERT(PR_FAILURE == ns_add_timeout_job(tp, &tv, NS_JOB_THREAD, ns_init_do_nothing_cb, NULL, NULL));
6f51e1
+
6f51e1
+}
6f51e1
+
6f51e1
 int
6f51e1
 main(void)
6f51e1
 {
6f51e1
@@ -410,6 +427,9 @@ main(void)
6f51e1
         cmocka_unit_test_setup_teardown(ns_job_signal_cb_test,
6f51e1
                                         ns_test_setup,
6f51e1
                                         ns_test_teardown),
6f51e1
+        cmocka_unit_test_setup_teardown(ns_job_neg_timeout_test,
6f51e1
+                                        ns_test_setup,
6f51e1
+                                        ns_test_teardown),
6f51e1
     };
6f51e1
     return cmocka_run_group_tests(tests, NULL, NULL);
6f51e1
 }
6f51e1
-- 
6f51e1
2.9.3
6f51e1