dhcp: Fix REQUIRE(ctx->running) assertion triggered on SIGTERM/SIGINT

Closed a small window of time between the installation of graceful
shutdown signal handlers and application context startup, during which
the receipt of shutdown signal would cause a REQUIRE() assertion to
occur.  Note this issue is only visible when compiling with
ENABLE_GENTLE_SHUTDOWN defined.

Reference:
https://gitlab.isc.org/isc-projects/dhcp/issues/53

Upstream patches:
ce117de7a1
dbd36dfa82
95944cab60

(From OE-Core rev: 7235c62727e48415c4e81f852607311ec31b6e41)

Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
This commit is contained in:
Ovidiu Panait
2020-02-27 13:45:49 +02:00
committed by Richard Purdie
parent f4f272f72c
commit ae1001ab3a
4 changed files with 228 additions and 0 deletions

View File

@@ -0,0 +1,165 @@
From f369dbb9e67eb5ef336944af63039b6d8f838384 Mon Sep 17 00:00:00 2001
From: Thomas Markwalder <tmark@isc.org>
Date: Thu, 12 Sep 2019 10:35:46 -0400
Subject: [PATCH 1/3] Ensure context is running prior to calling
isc_app_ctxsuspend
Add a release note.
includes/omapip/isclib.h
Added actx_running flag to global context, dhcp_gbl_ctx
omapip/isclib.c
set_ctx_running() - new function used as the ctxonrun callback
dhcp_context_create() - installs set_ctx_running callback
dhcp_signal_handler() - modified to use act_running flag to
determine is context is running and should be suspended
Upstream-Status: Backport [https://gitlab.isc.org/isc-projects/dhcp.git]
Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
---
RELNOTES | 7 +++++
includes/omapip/isclib.h | 3 ++-
omapip/isclib.c | 57 +++++++++++++++++++++++++++++++++-------
3 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/RELNOTES b/RELNOTES
index f10305d..1730473 100644
--- a/RELNOTES
+++ b/RELNOTES
@@ -6,6 +6,13 @@
NEW FEATURES
+- Closed a small window of time between the installation of graceful
+ shutdown signal handlers and application context startup, during which
+ the receipt of shutdown signal would cause a REQUIRE() assertion to
+ occur. Note this issue is only visible when compiling with
+ ENABLE_GENTLE_SHUTDOWN defined.
+ [Gitlab #53,!18 git TBD]
+
Please note that that ISC DHCP is now licensed under the Mozilla Public License,
MPL 2.0. Please see https://www.mozilla.org/en-US/MPL/2.0/ to read the MPL 2.0
license terms.
diff --git a/includes/omapip/isclib.h b/includes/omapip/isclib.h
index 6c20584..af6a6fc 100644
--- a/includes/omapip/isclib.h
+++ b/includes/omapip/isclib.h
@@ -94,7 +94,8 @@
typedef struct dhcp_context {
isc_mem_t *mctx;
isc_appctx_t *actx;
- int actx_started;
+ int actx_started; // ISC_TRUE if ctxstart has been called
+ int actx_running; // ISC_TRUE if ctxrun has been called
isc_taskmgr_t *taskmgr;
isc_task_t *task;
isc_socketmgr_t *socketmgr;
diff --git a/omapip/isclib.c b/omapip/isclib.c
index ce4b4a1..73e017c 100644
--- a/omapip/isclib.c
+++ b/omapip/isclib.c
@@ -134,6 +134,35 @@ handle_signal(int sig, void (*handler)(int)) {
}
}
+/* Callback passed to isc_app_ctxonrun
+ *
+ * BIND9 context code will invoke this handler once the context has
+ * entered the running state. We use it to set a global marker so that
+ * we can tell if the context is running. Several of the isc_app_
+ * calls REQUIRE that the context is running and we need a way to
+ * know that.
+ *
+ * We also check to see if we received a shutdown signal prior to
+ * the context entering the run state. If we did, then we can just
+ * simply shut the context down now. This closes the relatively
+ * small window between start up and entering run via the call
+ * to dispatch().
+ *
+ */
+static void
+set_ctx_running(isc_task_t *task, isc_event_t *event) {
+ task = task; // unused;
+ dhcp_gbl_ctx.actx_running = ISC_TRUE;
+
+ if (shutdown_signal) {
+ // We got signaled shutdown before we entered running state.
+ // Now that we've reached running state, shut'er down.
+ isc_app_ctxsuspend(dhcp_gbl_ctx.actx);
+ }
+
+ isc_event_free(&event);
+}
+
isc_result_t
dhcp_context_create(int flags,
struct in_addr *local4,
@@ -141,6 +170,9 @@ dhcp_context_create(int flags,
isc_result_t result;
if ((flags & DHCP_CONTEXT_PRE_DB) != 0) {
+ dhcp_gbl_ctx.actx_started = ISC_FALSE;
+ dhcp_gbl_ctx.actx_running = ISC_FALSE;
+
/*
* Set up the error messages, this isn't the right place
* for this call but it is convienent for now.
@@ -204,15 +236,24 @@ dhcp_context_create(int flags,
if (result != ISC_R_SUCCESS)
goto cleanup;
- result = isc_task_create(dhcp_gbl_ctx.taskmgr, 0, &dhcp_gbl_ctx.task);
+ result = isc_task_create(dhcp_gbl_ctx.taskmgr, 0,
+ &dhcp_gbl_ctx.task);
if (result != ISC_R_SUCCESS)
goto cleanup;
result = isc_app_ctxstart(dhcp_gbl_ctx.actx);
if (result != ISC_R_SUCCESS)
- return (result);
+ goto cleanup;
+
dhcp_gbl_ctx.actx_started = ISC_TRUE;
+ // Install the onrun callback.
+ result = isc_app_ctxonrun(dhcp_gbl_ctx.actx, dhcp_gbl_ctx.mctx,
+ dhcp_gbl_ctx.task, set_ctx_running,
+ dhcp_gbl_ctx.actx);
+ if (result != ISC_R_SUCCESS)
+ goto cleanup;
+
/* Not all OSs support suppressing SIGPIPE through socket
* options, so set the sigal action to be ignore. This allows
* broken connections to fail gracefully with EPIPE on writes */
@@ -335,19 +376,17 @@ isclib_make_dst_key(char *inname,
* @param signal signal code that we received
*/
void dhcp_signal_handler(int signal) {
- isc_appctx_t *ctx = dhcp_gbl_ctx.actx;
- int prev = shutdown_signal;
-
- if (prev != 0) {
+ if (shutdown_signal != 0) {
/* Already in shutdown. */
return;
}
+
/* Possible race but does it matter? */
shutdown_signal = signal;
- /* Use reload (aka suspend) for easier dispatch() reenter. */
- if (ctx && ctx->methods && ctx->methods->ctxsuspend) {
- (void) isc_app_ctxsuspend(ctx);
+ /* If the application context is running tell it to shut down */
+ if (dhcp_gbl_ctx.actx_running == ISC_TRUE) {
+ (void) isc_app_ctxsuspend(dhcp_gbl_ctx.actx);
}
}
--
2.23.0

View File

@@ -0,0 +1,29 @@
From adcd34ae1f56b16d7e9696d980332b4cf6c7ce91 Mon Sep 17 00:00:00 2001
From: Thomas Markwalder <tmark@isc.org>
Date: Fri, 13 Sep 2019 15:03:31 -0400
Subject: [PATCH 2/3] Added shutdown log statment to dhcrelay
Upstream-Status: Backport [https://gitlab.isc.org/isc-projects/dhcp.git]
Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
---
relay/dhcrelay.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/relay/dhcrelay.c b/relay/dhcrelay.c
index d8caaaf..4bd1d47 100644
--- a/relay/dhcrelay.c
+++ b/relay/dhcrelay.c
@@ -2076,6 +2076,9 @@ dhcp_set_control_state(control_object_state_t oldstate,
if (newstate != server_shutdown)
return ISC_R_SUCCESS;
+ /* Log shutdown on signal. */
+ log_info("Received signal %d, initiating shutdown.", shutdown_signal);
+
if (no_pid_file == ISC_FALSE)
(void) unlink(path_dhcrelay_pid);
--
2.23.0

View File

@@ -0,0 +1,31 @@
From e4b54b4d676783152d487103714cba2913661ef8 Mon Sep 17 00:00:00 2001
From: Thomas Markwalder <tmark@isc.org>
Date: Wed, 6 Nov 2019 15:53:50 -0500
Subject: [PATCH 3/3] Addressed review comment.
omapip/isclib.c
Added use of IGNORE_UNUSED()
Upstream-Status: Backport [https://gitlab.isc.org/isc-projects/dhcp.git]
Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
---
omapip/isclib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/omapip/isclib.c b/omapip/isclib.c
index 73e017c..1d52463 100644
--- a/omapip/isclib.c
+++ b/omapip/isclib.c
@@ -151,7 +151,7 @@ handle_signal(int sig, void (*handler)(int)) {
*/
static void
set_ctx_running(isc_task_t *task, isc_event_t *event) {
- task = task; // unused;
+ IGNORE_UNUSED(task);
dhcp_gbl_ctx.actx_running = ISC_TRUE;
if (shutdown_signal) {
--
2.23.0

View File

@@ -11,6 +11,9 @@ SRC_URI += "file://0001-define-macro-_PATH_DHCPD_CONF-and-_PATH_DHCLIENT_CON.pat
file://0013-fixup_use_libbind.patch \
file://0001-master-Added-includes-of-new-BIND9-compatibility-hea.patch \
file://0001-Fix-a-NSUPDATE-compiling-issue.patch \
file://0001-Ensure-context-is-running-prior-to-calling-isc_app_c.patch \
file://0002-Added-shutdown-log-statment-to-dhcrelay.patch \
file://0003-Addressed-review-comment.patch \
"
SRC_URI[md5sum] = "18c7f4dcbb0a63df25098216d47b1ede"