Changeset: 20ba6d492b51 for MonetDB
URL: http://dev.monetdb.org/hg/MonetDB?cmd=changeset;node=20ba6d492b51
Modified Files:
tools/merovingian/daemon/client.c
tools/merovingian/daemon/controlrunner.c
tools/merovingian/daemon/forkmserver.c
tools/merovingian/daemon/handlers.c
tools/merovingian/daemon/handlers.h
tools/merovingian/daemon/merovingian.c
Branch: Jun2016
Log Message:
Do not manipulate a shared linked list in a signal handler.
When a server exits, monetdbd gets a SIGCHLD signal. In the signal
handler, the record for that child is removed from the linked list of
children. This happens with the appropriate lock being held.
However, the locks are initialized so that if a thread tries to lock a
lock that it already holds, the second attempt succeeds.
So, if the thread executing the signal handler happens to already hold
the lock, say in logListener, the signal handler blithely ignores this
lock and manipulates the linked list that the thread (logListener)
thinks it is the sole owner of. Chaos ensues.
This hopefully finally fixed bug 4066.
diffs (271 lines):
diff --git a/tools/merovingian/daemon/client.c
b/tools/merovingian/daemon/client.c
--- a/tools/merovingian/daemon/client.c
+++ b/tools/merovingian/daemon/client.c
@@ -33,6 +33,7 @@
#include "multiplex-funnel.h"
#include "controlrunner.h"
#include "client.h"
+#include "handlers.h"
struct threads {
struct threads *next;
@@ -460,6 +461,7 @@ acceptConnections(int sock, int usock)
break;
}
}
+ childhandler();
if (retval == 0) {
/* nothing interesting has happened */
continue;
diff --git a/tools/merovingian/daemon/controlrunner.c
b/tools/merovingian/daemon/controlrunner.c
--- a/tools/merovingian/daemon/controlrunner.c
+++ b/tools/merovingian/daemon/controlrunner.c
@@ -390,13 +390,6 @@ static void ctl_handle_client(
} else {
if (*p != '\0') {
pid_t child;
- sigset_t blocksig;
- /* temporarily block SIGCHLD
signals until
- * we've waited for the child
we're about to
- * create. See bug
http://bugs.monetdb.org/3603. */
- sigemptyset(&blocksig);
- sigaddset(&blocksig, SIGCHLD);
- pthread_sigmask(SIG_BLOCK,
&blocksig, (sigset_t *) 0);
if ((child = fork()) == 0) {
FILE *secretf;
size_t len;
@@ -406,10 +399,6 @@ static void ctl_handle_client(
int setlen = 0;
char *sadbfarm;
- sigemptyset(&blocksig);
- sigaddset(&blocksig,
SIGCHLD);
-
pthread_sigmask(SIG_UNBLOCK, &blocksig, (sigset_t *) 0);
-
if ((err =
msab_getDBfarm(&sadbfarm)) != NULL) {
Mfprintf(_mero_ctlerr, "%s: internal error: %s\n",
origin, err);
@@ -465,9 +454,6 @@ static void ctl_handle_client(
Mfprintf(_mero_ctlout,
"%s: forking failed\n",
origin);
}
- sigemptyset(&blocksig);
- sigaddset(&blocksig, SIGCHLD);
- pthread_sigmask(SIG_UNBLOCK,
&blocksig, (sigset_t *) 0);
}
Mfprintf(_mero_ctlout, "%s: created
database '%s'\n",
diff --git a/tools/merovingian/daemon/forkmserver.c
b/tools/merovingian/daemon/forkmserver.c
--- a/tools/merovingian/daemon/forkmserver.c
+++ b/tools/merovingian/daemon/forkmserver.c
@@ -54,7 +54,6 @@ forkMserver(char *database, sabdb** stat
char upmax[8];
confkeyval *ckv, *kv, *list;
SABdbState state;
- sigset_t blocksig;
er = msab_getStatus(stats, database);
if (er != NULL) {
@@ -258,9 +257,6 @@ forkMserver(char *database, sabdb** stat
database, database));
}
- sigemptyset(&blocksig);
- sigaddset(&blocksig, SIGCHLD);
- pthread_sigmask(SIG_BLOCK, &blocksig, (sigset_t *) 0);
pid = fork();
if (pid == 0) {
char *sabdbfarm;
@@ -279,10 +275,6 @@ forkMserver(char *database, sabdb** stat
int c = 0;
unsigned int mport;
- sigemptyset(&blocksig);
- sigaddset(&blocksig, SIGCHLD);
- pthread_sigmask(SIG_UNBLOCK, &blocksig, (sigset_t *) 0);
-
er = msab_getDBfarm(&sabdbfarm);
if (er != NULL) {
Mfprintf(stderr, "unexpected error: %s\n", er);
@@ -454,10 +446,6 @@ forkMserver(char *database, sabdb** stat
pthread_mutex_unlock(&_mero_topdp_lock);
- sigemptyset(&blocksig);
- sigaddset(&blocksig, SIGCHLD);
- pthread_sigmask(SIG_UNBLOCK, &blocksig, (sigset_t *) 0);
-
/* wait for the child to finish starting, at some point
we
* decided that we should wait indefinitely here
because if the
* mserver needs time to start up, we shouldn't
interrupt it,
diff --git a/tools/merovingian/daemon/handlers.c
b/tools/merovingian/daemon/handlers.c
--- a/tools/merovingian/daemon/handlers.c
+++ b/tools/merovingian/daemon/handlers.c
@@ -143,83 +143,66 @@ huphandler(int sig)
}
/**
- * Handles SIGCHLD signals, that is, signals that a parent receives
- * about its children. This handler deals with terminated children, by
- * deregistering them from the internal administration (_mero_topdp)
- * with the necessary cleanup.
+ * Wait for and deal with any children that may have exited. This
+ * handler deals with terminated children, by deregistering them from
+ * the internal administration (_mero_topdp) with the necessary
+ * cleanup.
*/
void
-childhandler(int sig, siginfo_t *si, void *unused)
+childhandler(void)
{
dpair p, q;
+ pid_t pid;
+ int wstatus;
- (void)sig;
- (void)unused;
+ while ((pid = waitpid(-1, &wstatus, WNOHANG)) > 0) {
+ pthread_mutex_lock(&_mero_topdp_lock);
- /* wait for the child to get properly terminated, hopefully filling
- * in the siginfo struct on FreeBSD */
- if (waitpid(-1, NULL, WNOHANG) <= 0) {
- /* if no child has exited, we may have already waited for it
- * in e.g. ctl_handle_client() */
- return;
+ /* get the pid from the former child, and locate it in our list
*/
+ q = _mero_topdp->next;
+ p = q->next;
+ while (p != NULL) {
+ if (p->pid == pid) {
+ /* log everything that's still in the pipes */
+ logFD(p->out, "MSG", p->dbname, (long long
int)p->pid, _mero_logfile);
+ /* remove from the list */
+ q->next = p->next;
+ /* close the descriptors */
+ close(p->out);
+ close(p->err);
+ if (WIFEXITED(wstatus)) {
+ Mfprintf(stdout, "database '%s' (%lld)
has exited with "
+ "exit status %d\n",
p->dbname,
+ (long long int)p->pid,
WEXITSTATUS(wstatus));
+ } else if (WIFSIGNALED(wstatus)) {
+ if (WCOREDUMP(wstatus)) {
+ Mfprintf(stdout, "database '%s'
(%lld) has crashed "
+ "(dumped
core)\n", p->dbname,
+ (long long
int)p->pid);
+ } else {
+ const char *sigstr =
sigtostr(WTERMSIG(wstatus));
+ char signum[8];
+ if (sigstr == NULL) {
+ snprintf(signum, 8,
"%d", WTERMSIG(wstatus));
+ sigstr = signum;
+ }
+ Mfprintf(stdout, "database '%s'
(%lld) was killed by signal "
+ "%s\n",
p->dbname,
+ (long long
int)p->pid, sigstr);
+ }
+ }
+ if (p->dbname)
+ free(p->dbname);
+ free(p);
+ pthread_mutex_unlock(&_mero_topdp_lock);
+ break;
+ }
+ q = p;
+ p = q->next;
+ }
+
+ pthread_mutex_unlock(&_mero_topdp_lock);
}
-
- if (si->si_code != CLD_EXITED &&
- si->si_code != CLD_KILLED &&
- si->si_code != CLD_DUMPED)
- {
- /* ignore traps, stops and continues, we only want terminations
- * of the client process */
- return;
- }
-
- pthread_mutex_lock(&_mero_topdp_lock);
-
- /* get the pid from the former child, and locate it in our list */
- q = _mero_topdp->next;
- p = q->next;
- while (p != NULL) {
- if (p->pid == si->si_pid) {
- /* log everything that's still in the pipes */
- logFD(p->out, "MSG", p->dbname, (long long int)p->pid,
_mero_logfile);
- /* remove from the list */
- q->next = p->next;
- /* close the descriptors */
- close(p->out);
- close(p->err);
- if (si->si_code == CLD_EXITED) {
- Mfprintf(stdout, "database '%s' (%lld) has
exited with "
- "exit status %d\n", p->dbname,
- (long long int)p->pid,
si->si_status);
- } else if (si->si_code == CLD_KILLED) {
- const char *sigstr = sigtostr(si->si_status);
- char signum[8];
- if (sigstr == NULL) {
- snprintf(signum, 8, "%d",
si->si_status);
- sigstr = signum;
- }
- Mfprintf(stdout, "database '%s' (%lld) was
killed by signal "
- "%s\n", p->dbname,
- (long long int)p->pid, sigstr);
- } else if (si->si_code == CLD_DUMPED) {
- Mfprintf(stdout, "database '%s' (%lld) has
crashed "
- "(dumped core)\n", p->dbname,
- (long long int)p->pid);
- }
- if (p->dbname)
- free(p->dbname);
- free(p);
- pthread_mutex_unlock(&_mero_topdp_lock);
- return;
- }
- q = p;
- p = q->next;
- }
-
- pthread_mutex_unlock(&_mero_topdp_lock);
-
- Mfprintf(stdout, "received SIGCHLD from unknown child with pid %lld\n",
- (long long int)si->si_pid);
}
/**
diff --git a/tools/merovingian/daemon/handlers.h
b/tools/merovingian/daemon/handlers.h
--- a/tools/merovingian/daemon/handlers.h
+++ b/tools/merovingian/daemon/handlers.h
@@ -13,7 +13,7 @@
void handler(int sig);
void huphandler(int sig);
-void childhandler(int sig, siginfo_t *si, void *unused);
+void childhandler(void);
void segvhandler(int sig);
#endif
diff --git a/tools/merovingian/daemon/merovingian.c
b/tools/merovingian/daemon/merovingian.c
--- a/tools/merovingian/daemon/merovingian.c
+++ b/tools/merovingian/daemon/merovingian.c
@@ -888,15 +888,6 @@ main(int argc, char *argv[])
MERO_EXIT(1);
}
- sa.sa_flags = SA_SIGINFO;
- sigemptyset(&sa.sa_mask);
- sa.sa_sigaction = childhandler;
- if (sigaction(SIGCHLD, &sa, NULL) == -1) {
- Mfprintf(oerr, "%s: FATAL: unable to create signal handlers:
%s\n",
- argv[0], strerror(errno));
- MERO_EXIT(1);
- }
-
/* make sure we will be able to write our pid */
if ((pidfile = fopen(pidfilename, "w")) == NULL) {
Mfprintf(stderr, "unable to open '%s%s%s' for writing: %s\n",
_______________________________________________
checkin-list mailing list
[email protected]
https://www.monetdb.org/mailman/listinfo/checkin-list
