[PATCH] Fix winbind child process exit bug
Volker Lendecke
Volker.Lendecke at SerNet.DE
Tue Feb 27 07:57:23 UTC 2018
Hi!
Attached find a patchset that solves an issue when a winbind helper
process dies. It's patch 10/11. To make the winbind parent/child
relationships easier to understand (at least to me), the 9 patches
before remove the "winbindd_children" linked list.
Review appreciated!
Thanks, Volker
--
Besuchen Sie die verinice.XP 2018 in Berlin,
Anwenderkonferenz für Informationssicherheit
vom 21.-23.03.2018 im Sofitel Kurfürstendamm
Info & Anmeldung hier: http://veriniceXP.org
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From bfb60674a955c3c2298a470c2056e88db58d6089 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 12:55:31 +0100
Subject: [PATCH 01/11] winbind: Implement forall_children()
Step 0 in removing winbindd_children as a variable: We have access to all
children via our domain list and the two explicit children. There's no need to
separately maintain a list of winbind children. Maintaining child->pid!=0 is
sufficient to make sure we only walk active children.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_dual.c | 86 +++++++++++++++++++++++++++++++++++-----
1 file changed, 75 insertions(+), 11 deletions(-)
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index fa34e7363ef..82a4a4f200e 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -48,6 +48,57 @@ extern bool override_logfile;
static struct winbindd_child *winbindd_children = NULL;
+static void forall_domain_children(bool (*fn)(struct winbindd_child *c,
+ void *private_data),
+ void *private_data)
+{
+ struct winbindd_domain *d;
+
+ for (d = domain_list(); d != NULL; d = d->next) {
+ int i;
+
+ for (i=0; i<lp_winbind_max_domain_connections(); i++) {
+ struct winbindd_child *c = &d->children[i];
+ bool ok;
+
+ if (c->pid == 0) {
+ continue;
+ }
+
+ ok = fn(c, private_data);
+ if (!ok) {
+ return;
+ }
+ }
+ }
+}
+
+static void forall_children(bool (*fn)(struct winbindd_child *c,
+ void *private_data),
+ void *private_data)
+{
+ struct winbindd_child *c;
+ bool ok;
+
+ c = idmap_child();
+ if (c->pid != 0) {
+ ok = fn(c, private_data);
+ if (!ok) {
+ return;
+ }
+ }
+
+ c = locator_child();
+ if (c->pid != 0) {
+ ok = fn(c, private_data);
+ if (!ok) {
+ return;
+ }
+ }
+
+ forall_domain_children(fn, private_data);
+}
+
/* Read some data from a client connection */
static NTSTATUS child_read_request(int sock, struct winbindd_request *wreq)
@@ -709,6 +760,7 @@ void setup_child(struct winbindd_domain *domain, struct winbindd_child *child,
"logname == NULL");
}
+ child->pid = 0;
child->sock = -1;
child->domain = domain;
child->table = table;
@@ -762,28 +814,40 @@ void winbindd_flush_negative_conn_cache(struct winbindd_domain *domain)
* level to that of parents.
*/
+struct winbind_msg_relay_state {
+ struct messaging_context *msg_ctx;
+ uint32_t msg_type;
+ DATA_BLOB *data;
+};
+
+static bool winbind_msg_relay_fn(struct winbindd_child *child,
+ void *private_data)
+{
+ struct winbind_msg_relay_state *state = private_data;
+
+ DBG_DEBUG("sending message to pid %u.\n",
+ (unsigned int)child->pid);
+
+ messaging_send(state->msg_ctx, pid_to_procid(child->pid),
+ state->msg_type, state->data);
+ return true;
+}
+
void winbind_msg_debug(struct messaging_context *msg_ctx,
void *private_data,
uint32_t msg_type,
struct server_id server_id,
DATA_BLOB *data)
{
- struct winbindd_child *child;
+ struct winbind_msg_relay_state state = {
+ .msg_ctx = msg_ctx, .msg_type = msg_type, .data = data
+ };
DEBUG(10,("winbind_msg_debug: got debug message.\n"));
debug_message(msg_ctx, private_data, MSG_DEBUG, server_id, data);
- for (child = winbindd_children; child != NULL; child = child->next) {
-
- DEBUG(10,("winbind_msg_debug: sending message to pid %u.\n",
- (unsigned int)child->pid));
-
- messaging_send_buf(msg_ctx, pid_to_procid(child->pid),
- MSG_DEBUG,
- data->data,
- strlen((char *) data->data) + 1);
- }
+ forall_children(winbind_msg_relay_fn, &state);
}
/* Set our domains as offline and forward the offline message to our children. */
--
2.11.0
From c6b2f4ab9ac708d8313694d8f24b7d1f57e01a64 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 12:59:06 +0100
Subject: [PATCH 02/11] winbind: Use forall_children in winbind_child_died()
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_dual.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index 82a4a4f200e..d289abe48e0 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -772,29 +772,42 @@ void setup_child(struct winbindd_domain *domain, struct winbindd_child *child,
}
}
-void winbind_child_died(pid_t pid)
-{
+struct winbind_child_died_state {
+ pid_t pid;
struct winbindd_child *child;
+};
- for (child = winbindd_children; child != NULL; child = child->next) {
- if (child->pid == pid) {
- break;
- }
+static bool winbind_child_died_fn(struct winbindd_child *child,
+ void *private_data)
+{
+ struct winbind_child_died_state *state = private_data;
+
+ if (child->pid == state->pid) {
+ state->child = child;
+ return false;
}
+ return true;
+}
- if (child == NULL) {
+void winbind_child_died(pid_t pid)
+{
+ struct winbind_child_died_state state = { .pid = pid };
+
+ forall_children(winbind_child_died_fn, &state);
+
+ if (state.child == NULL) {
DEBUG(5, ("Already reaped child %u died\n", (unsigned int)pid));
return;
}
/* This will be re-added in fork_domain_child() */
- DLIST_REMOVE(winbindd_children, child);
- child->pid = 0;
+ DLIST_REMOVE(winbindd_children, state.child);
+ state.child->pid = 0;
- if (child->sock != -1) {
- close(child->sock);
- child->sock = -1;
+ if (state.child->sock != -1) {
+ close(state.child->sock);
+ state.child->sock = -1;
}
}
--
2.11.0
From d32b49fff30b6b09e703a6313ee321a1ad528a0d Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 13:14:21 +0100
Subject: [PATCH 03/11] winbind: "internal" children never have a domain set
Look at setup_domain_child(): There we always set child->domain. The only other
two children are the idmap and locator children, which don't have a domain set.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_dual.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index d289abe48e0..3696250cb84 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -897,9 +897,8 @@ void winbind_msg_offline(struct messaging_context *msg_ctx,
}
for (child = winbindd_children; child != NULL; child = child->next) {
- /* Don't send message to internal children. We've already
- done so above. */
- if (!child->domain || winbindd_internal_child(child)) {
+ /* Only set domain children offline */
+ if (child->domain == NULL) {
continue;
}
@@ -972,8 +971,8 @@ void winbind_msg_online(struct messaging_context *msg_ctx,
}
for (child = winbindd_children; child != NULL; child = child->next) {
- /* Don't send message to internal childs. */
- if (!child->domain || winbindd_internal_child(child)) {
+ /* Only set domain children online */
+ if (child->domain == NULL) {
continue;
}
--
2.11.0
From b3bdc138a4e61b03091adae5c6517fe2e8f328c9 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 13:15:14 +0100
Subject: [PATCH 04/11] winbind: Remove unused winbindd_internal_child()
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_proto.h | 1 -
source3/winbindd/winbindd_util.c | 12 ------------
2 files changed, 13 deletions(-)
diff --git a/source3/winbindd/winbindd_proto.h b/source3/winbindd/winbindd_proto.h
index 704a8dd2924..b70227c9b91 100644
--- a/source3/winbindd/winbindd_proto.h
+++ b/source3/winbindd/winbindd_proto.h
@@ -503,7 +503,6 @@ NTSTATUS resolve_alias_to_username(TALLOC_CTX *mem_ctx,
const char *alias, char **name);
bool winbindd_can_contact_domain(struct winbindd_domain *domain);
-bool winbindd_internal_child(struct winbindd_child *child);
void winbindd_set_locator_kdc_envs(const struct winbindd_domain *domain);
void winbindd_unset_locator_kdc_env(const struct winbindd_domain *domain);
void winbindd_set_locator_kdc_envs(const struct winbindd_domain *domain);
diff --git a/source3/winbindd/winbindd_util.c b/source3/winbindd/winbindd_util.c
index 73e6b76ec73..3ec84d73020 100644
--- a/source3/winbindd/winbindd_util.c
+++ b/source3/winbindd/winbindd_util.c
@@ -1968,18 +1968,6 @@ done:
return ret;
}
-/*********************************************************************
- ********************************************************************/
-
-bool winbindd_internal_child(struct winbindd_child *child)
-{
- if ((child == idmap_child()) || (child == locator_child())) {
- return True;
- }
-
- return False;
-}
-
#ifdef HAVE_KRB5_LOCATE_PLUGIN_H
/*********************************************************************
--
2.11.0
From b2615fa9c35078c8828b99f386de3cbcd5a2add0 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 13:20:25 +0100
Subject: [PATCH 05/11] winbind: Use forall_domain_children in
winbind_msg_offline()
Note that we only walk the domain children, which all have
child->domain!=NULL. So we don't need that check anymore.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_dual.c | 56 +++++++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 23 deletions(-)
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index 3696250cb84..21424b9e590 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -865,13 +865,44 @@ void winbind_msg_debug(struct messaging_context *msg_ctx,
/* Set our domains as offline and forward the offline message to our children. */
+struct winbind_msg_on_offline_state {
+ struct messaging_context *msg_ctx;
+ uint32_t msg_type;
+};
+
+static bool winbind_msg_on_offline_fn(struct winbindd_child *child,
+ void *private_data)
+{
+ struct winbind_msg_on_offline_state *state = private_data;
+
+ if (child->domain->internal) {
+ return true;
+ }
+
+ /* Each winbindd child should only process requests for one
+ * domain - make sure we only set it online / offline for that
+ * domain. */
+
+ DBG_DEBUG("sending message to pid %u for domain %s.\n",
+ (unsigned int)child->pid, child->domain->name);
+
+ messaging_send_buf(state->msg_ctx, pid_to_procid(child->pid),
+ state->msg_type,
+ (const uint8_t *)child->domain->name,
+ strlen(child->domain->name)+1);
+
+ return true;
+}
+
void winbind_msg_offline(struct messaging_context *msg_ctx,
void *private_data,
uint32_t msg_type,
struct server_id server_id,
DATA_BLOB *data)
{
- struct winbindd_child *child;
+ struct winbind_msg_on_offline_state state = {
+ .msg_ctx = msg_ctx, .msg_type = MSG_WINBIND_OFFLINE
+ };
struct winbindd_domain *domain;
DEBUG(10,("winbind_msg_offline: got offline message.\n"));
@@ -896,28 +927,7 @@ void winbind_msg_offline(struct messaging_context *msg_ctx,
set_domain_offline(domain);
}
- for (child = winbindd_children; child != NULL; child = child->next) {
- /* Only set domain children offline */
- if (child->domain == NULL) {
- continue;
- }
-
- /* Or internal domains (this should not be possible....) */
- if (child->domain->internal) {
- continue;
- }
-
- /* Each winbindd child should only process requests for one domain - make sure
- we only set it online / offline for that domain. */
-
- DEBUG(10,("winbind_msg_offline: sending message to pid %u for domain %s.\n",
- (unsigned int)child->pid, child->domain->name ));
-
- messaging_send_buf(msg_ctx, pid_to_procid(child->pid),
- MSG_WINBIND_OFFLINE,
- (const uint8_t *)child->domain->name,
- strlen(child->domain->name)+1);
- }
+ forall_domain_children(winbind_msg_on_offline_fn, &state);
}
/* Set our domains as online and forward the online message to our children. */
--
2.11.0
From c32e3cc6a941bb577af1393babf2097d1998388f Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 13:24:50 +0100
Subject: [PATCH 06/11] winbind: Use forall_domain_children in
winbind_msg_online
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_dual.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index 21424b9e590..688196c91ed 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -938,7 +938,9 @@ void winbind_msg_online(struct messaging_context *msg_ctx,
struct server_id server_id,
DATA_BLOB *data)
{
- struct winbindd_child *child;
+ struct winbind_msg_on_offline_state state = {
+ .msg_ctx = msg_ctx, .msg_type = MSG_WINBIND_ONLINE
+ };
struct winbindd_domain *domain;
DEBUG(10,("winbind_msg_online: got online message.\n"));
@@ -980,28 +982,7 @@ void winbind_msg_online(struct messaging_context *msg_ctx,
}
}
- for (child = winbindd_children; child != NULL; child = child->next) {
- /* Only set domain children online */
- if (child->domain == NULL) {
- continue;
- }
-
- /* Or internal domains (this should not be possible....) */
- if (child->domain->internal) {
- continue;
- }
-
- /* Each winbindd child should only process requests for one domain - make sure
- we only set it online / offline for that domain. */
-
- DEBUG(10,("winbind_msg_online: sending message to pid %u for domain %s.\n",
- (unsigned int)child->pid, child->domain->name ));
-
- messaging_send_buf(msg_ctx, pid_to_procid(child->pid),
- MSG_WINBIND_ONLINE,
- (const uint8_t *)child->domain->name,
- strlen(child->domain->name)+1);
- }
+ forall_domain_children(winbind_msg_on_offline_fn, &state);
}
static const char *collect_onlinestatus(TALLOC_CTX *mem_ctx)
--
2.11.0
From 54963f7d9f77bcf14b5da933fca44f04441f561f Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 13:37:05 +0100
Subject: [PATCH 07/11] winbind: Use forall_children in
winbind_msg_ip_dropped_parent()
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_dual.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index 688196c91ed..be69c9a453d 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -1789,14 +1789,12 @@ void winbind_msg_ip_dropped_parent(struct messaging_context *msg_ctx,
struct server_id server_id,
DATA_BLOB *data)
{
- struct winbindd_child *child;
+ struct winbind_msg_relay_state state = {
+ .msg_ctx = msg_ctx, .msg_type = msg_type, .data = data
+ };
winbind_msg_ip_dropped(msg_ctx, private_data, msg_type,
server_id, data);
-
- for (child = winbindd_children; child != NULL; child = child->next) {
- messaging_send_buf(msg_ctx, pid_to_procid(child->pid),
- msg_type, data->data, data->length);
- }
+ forall_children(winbind_msg_relay_fn, &state);
}
--
2.11.0
From 390b8c5a1a89f49499a85d44311483d71d1d0a8e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 13:45:01 +0100
Subject: [PATCH 08/11] winbind: Use forall_children in reinit_after_fork()
This removes the special handling for idmap_child() after the "This is
a little tricky" comment. I believe this was not required at all, the
idmap_child is part of the winbindd_children list.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_dual.c | 75 ++++++++++++++++++++--------------------
1 file changed, 37 insertions(+), 38 deletions(-)
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index be69c9a453d..f2f9d8d424b 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -1416,11 +1416,46 @@ static void child_msg_online(struct messaging_context *msg,
}
}
+struct winbindd_reinit_after_fork_state {
+ const struct winbindd_child *myself;
+};
+
+static bool winbindd_reinit_after_fork_fn(struct winbindd_child *child,
+ void *private_data)
+{
+ struct winbindd_reinit_after_fork_state *state = private_data;
+
+ if (child == state->myself) {
+ return true;
+ }
+
+ /* Destroy all possible events in child list. */
+
+ TALLOC_FREE(child->lockout_policy_event);
+ TALLOC_FREE(child->machine_password_change_event);
+
+ /*
+ * Children should never be able to send each other messages,
+ * all messages must go through the parent.
+ */
+ child->pid = (pid_t)0;
+
+ /*
+ * Close service sockets to all other children
+ */
+ if (child->sock != -1) {
+ close(child->sock);
+ child->sock = -1;
+ }
+
+ return true;
+}
+
NTSTATUS winbindd_reinit_after_fork(const struct winbindd_child *myself,
const char *logfilename)
{
+ struct winbindd_reinit_after_fork_state state = { .myself = myself };
struct winbindd_domain *domain;
- struct winbindd_child *cl;
NTSTATUS status;
status = reinit_after_fork(
@@ -1484,43 +1519,7 @@ NTSTATUS winbindd_reinit_after_fork(const struct winbindd_child *myself,
ccache_remove_all_after_fork();
- /* Destroy all possible events in child list. */
- for (cl = winbindd_children; cl != NULL; cl = cl->next) {
- TALLOC_FREE(cl->lockout_policy_event);
- TALLOC_FREE(cl->machine_password_change_event);
-
- /* Children should never be able to send
- * each other messages, all messages must
- * go through the parent.
- */
- cl->pid = (pid_t)0;
-
- /*
- * Close service sockets to all other children
- */
- if ((cl != myself) && (cl->sock != -1)) {
- close(cl->sock);
- cl->sock = -1;
- }
- }
- /*
- * This is a little tricky, children must not
- * send an MSG_WINBIND_ONLINE message to idmap_child().
- * If we are in a child of our primary domain or
- * in the process created by fork_child_dc_connect(),
- * and the primary domain cannot go online,
- * fork_child_dc_connection() sends MSG_WINBIND_ONLINE
- * periodically to idmap_child().
- *
- * The sequence is, fork_child_dc_connect() ---> getdcs() --->
- * get_dc_name_via_netlogon() ---> cm_connect_netlogon()
- * ---> init_dc_connection() ---> cm_open_connection --->
- * set_domain_online(), sends MSG_WINBIND_ONLINE to
- * idmap_child(). Disallow children sending messages
- * to each other, all messages must go through the parent.
- */
- cl = idmap_child();
- cl->pid = (pid_t)0;
+ forall_children(winbindd_reinit_after_fork_fn, &state);
return NT_STATUS_OK;
}
--
2.11.0
From b520353c334f70a9283c95278df0ca32f9ad77f1 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 13:48:24 +0100
Subject: [PATCH 09/11] winbind: Remove the "winbindd_children" global
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd.h | 2 --
source3/winbindd/winbindd_dual.c | 8 --------
source3/winbindd/winbindd_ndr.c | 2 --
3 files changed, 12 deletions(-)
diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h
index 081722f6a90..6aa57f2e704 100644
--- a/source3/winbindd/winbindd.h
+++ b/source3/winbindd/winbindd.h
@@ -115,8 +115,6 @@ struct winbindd_child_dispatch_table {
};
struct winbindd_child {
- struct winbindd_child *next, *prev;
-
pid_t pid;
struct winbindd_domain *domain;
char *logfilename;
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index f2f9d8d424b..69df2ddce10 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -46,8 +46,6 @@
extern bool override_logfile;
-static struct winbindd_child *winbindd_children = NULL;
-
static void forall_domain_children(bool (*fn)(struct winbindd_child *c,
void *private_data),
void *private_data)
@@ -363,7 +361,6 @@ static void wb_child_request_cleanup(struct tevent_req *req,
*/
close(state->child->sock);
state->child->sock = -1;
- DLIST_REMOVE(winbindd_children, state->child);
}
static struct winbindd_child *choose_domain_child(struct winbindd_domain *domain)
@@ -800,9 +797,6 @@ void winbind_child_died(pid_t pid)
return;
}
- /* This will be re-added in fork_domain_child() */
-
- DLIST_REMOVE(winbindd_children, state.child);
state.child->pid = 0;
if (state.child->sock != -1) {
@@ -1633,8 +1627,6 @@ static bool fork_domain_child(struct winbindd_child *child)
return false;
}
- child->next = child->prev = NULL;
- DLIST_ADD(winbindd_children, child);
child->sock = fdpair[1];
return True;
}
diff --git a/source3/winbindd/winbindd_ndr.c b/source3/winbindd/winbindd_ndr.c
index 383de4e8116..74a03d0bc33 100644
--- a/source3/winbindd/winbindd_ndr.c
+++ b/source3/winbindd/winbindd_ndr.c
@@ -36,8 +36,6 @@ void ndr_print_winbindd_child(struct ndr_print *ndr,
{
ndr_print_struct(ndr, name, "winbindd_child");
ndr->depth++;
- ndr_print_ptr(ndr, "next", r->next);
- ndr_print_ptr(ndr, "prev", r->prev);
ndr_print_uint32(ndr, "pid", (uint32_t)r->pid);
#if 0
ndr_print_winbindd_domain(ndr, "domain", r->domain);
--
2.11.0
From 0f81f9c643042c28f5f5c856ead2428c68185c93 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 15:12:14 +0100
Subject: [PATCH 10/11] winbind: Fix a race between the sigchld and 0-sized
socket read
Fix a bug when a child dies when a request is pending in the child. If the
signal handler fires before epoll finds out the other end of the parent-child
socket is closed, we close the socket on our side without taking care of the
pending request. This causes two problems: First, that one pending request
never is replied to properly, and secondly, we might end up with EPOLL_DEL on a
wrong file descriptor. This causes all sorts of trouble if we hit an active
one.
The fix for this problem is not to close the socket in winbind_child_died().
This however stops an idle child that dies hard from being properly cleaned up.
The fix for that is to add the child->monitor_fde that is set pending only when
no child request is active. This way we can remove the close(sock) in the
signal handler.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd.h | 1 +
source3/winbindd/winbindd_dual.c | 47 +++++++++++++++++++++++++++++++++++-----
2 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h
index 6aa57f2e704..f496c41ccbd 100644
--- a/source3/winbindd/winbindd.h
+++ b/source3/winbindd/winbindd.h
@@ -120,6 +120,7 @@ struct winbindd_child {
char *logfilename;
int sock;
+ struct tevent_fd *monitor_fde; /* Watch for dead children/sockets */
struct tevent_queue *queue;
struct dcerpc_binding_handle *binding_handle;
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index 69df2ddce10..70e9e302b5d 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -239,6 +239,8 @@ static void wb_child_request_waited(struct tevent_req *subreq)
return;
}
+ tevent_fd_set_flags(state->child->monitor_fde, 0);
+
subreq = wb_simple_trans_send(state, server_event_context(), NULL,
state->child->sock, state->request);
if (tevent_req_nomem(subreq, req)) {
@@ -338,6 +340,8 @@ static void wb_child_request_cleanup(struct tevent_req *req,
TALLOC_FREE(state->subreq);
TALLOC_FREE(state->queue_subreq);
+ tevent_fd_set_flags(state->child->monitor_fde, TEVENT_FD_READ);
+
if (state->child->domain != NULL) {
/*
* If the child is attached to a domain,
@@ -359,10 +363,36 @@ static void wb_child_request_cleanup(struct tevent_req *req,
* The basic parent/child communication broke, close
* our socket
*/
+ TALLOC_FREE(state->child->monitor_fde);
close(state->child->sock);
state->child->sock = -1;
}
+static void child_socket_readable(struct tevent_context *ev,
+ struct tevent_fd *fde,
+ uint16_t flags,
+ void *private_data)
+{
+ struct winbindd_child *child = private_data;
+
+ if ((flags & TEVENT_FD_READ) == 0) {
+ return;
+ }
+
+ TALLOC_FREE(child->monitor_fde);
+
+ /*
+ * We're only active when there is no outstanding child
+ * request. Arriving here means the child closed its socket,
+ * it died. Do the same here.
+ */
+
+ SMB_ASSERT(child->sock != -1);
+
+ close(child->sock);
+ child->sock = -1;
+}
+
static struct winbindd_child *choose_domain_child(struct winbindd_domain *domain)
{
struct winbindd_child *shortest = &domain->children[0];
@@ -798,11 +828,6 @@ void winbind_child_died(pid_t pid)
}
state.child->pid = 0;
-
- if (state.child->sock != -1) {
- close(state.child->sock);
- state.child->sock = -1;
- }
}
/* Ensure any negative cache entries with the netbios or realm names are removed. */
@@ -1627,6 +1652,18 @@ static bool fork_domain_child(struct winbindd_child *child)
return false;
}
+ child->monitor_fde = tevent_add_fd(server_event_context(),
+ server_event_context(),
+ fdpair[1],
+ TEVENT_FD_READ,
+ child_socket_readable,
+ child);
+ if (child->monitor_fde == NULL) {
+ DBG_WARNING("tevent_add_fd failed\n");
+ close(fdpair[1]);
+ return false;
+ }
+
child->sock = fdpair[1];
return True;
}
--
2.11.0
From 919611cda62a8107ebd230b6362bef98c59e2f42 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 26 Feb 2018 15:32:05 +0100
Subject: [PATCH 11/11] winbind: Fix --ping-dc error handling
If the child dies at the wrong moment, we get an error in the "req" itself.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/winbindd/winbindd_ping_dc.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/source3/winbindd/winbindd_ping_dc.c b/source3/winbindd/winbindd_ping_dc.c
index a6cea79ee2a..c1a0a7b0ec6 100644
--- a/source3/winbindd/winbindd_ping_dc.c
+++ b/source3/winbindd/winbindd_ping_dc.c
@@ -115,6 +115,11 @@ NTSTATUS winbindd_ping_dc_recv(struct tevent_req *req,
{
struct winbindd_ping_dc_state *state = tevent_req_data(
req, struct winbindd_ping_dc_state);
+ NTSTATUS status;
+
+ if (tevent_req_is_nterror(req, &status)) {
+ return status;
+ }
if (!NT_STATUS_IS_OK(state->result)) {
set_auth_errors(presp, state->result);
--
2.11.0
More information about the samba-technical
mailing list