code calls rad_fork(1) (such as rad_check_ts()) This means that no entry is stored in thread_pool.waiters This means that rad_waitpid() calls reap_children() which means that subsequent calls to waitpid() -- since there is no thread_pool.waiters entry for the pid -- return an ECHILD, which leads rad_check_ts() to print this message radlog(L_ERR, "Check-TS: unknown error in waitpid()"); So basically rad_fork(1) and rad_waitpid() and waitpid() are now unreliable. Furthermore, using rad_fork(0) without rad_waitpid(pid,...,...) will eventualy result in a full thread_pool.waiters hashtable and calls to rad_fork(0) will fail. So waitpid(0,...,...) in reap_children() buys nothing except elimination of zombie processes by trading it in for causing calls of rad_fork(0) to fail. (why 0 and not -1 main waitpid..... -1 meaning wait for any child process. 0 meaning wait for any child process whose process group ID is equal to that of the calling process. ) I am getting errors forking my scripts off with rlm_exec -- apparently thread_pool.waiters is getting full somehow, since this is logged. Error: Couldn't fork /usr/local/sbin/radius-dns-update.sh: No child processes Which isnt a documented errno for fork() grep "No child" /usr/include/ -r /usr/include/asm-generic/errno-base.h:#define ECHILD 10 /* No child processes */ The fix? Here are some ideas. 1) reap_children() should only call waitpid() for entries in thread_pool.waiters -- this was the old way, should be fairly easy to revert to. This was changed due to zombies being created by creative forking in rlm_perl. I think that problem should have properly been resolved by using rad_fork() or by building its own long lived fork/wait mechanism or by properly daemonizing the child process. -- rad_fork(1)s or fork()s that are not followed by waitpid()s will result in zombies (which is a bug owned by the code doing the call to fork) IMHO this makes the most sense -- freeradius should not try to fix other code bugs by introducing all the breakage as descibed above. 2) reap_children() should create a thread_pool.waiters entry for pids that have exited for subsequent retrieval of status by rad_waitpid -- rad_fork(1)s or fork()s that are not followed by waitpid()s will result in thread_pool.waiters running out of elements. -- waitpid() is still unreliable -- rad_fork(1) is still (nearly) meaningless This can be handled by rad_fork() behaving differently, such as deleting all thread_pool.waiters elements that created by reap_children() if the thread_pool.waiters reaches its limit. This is a series of nasty kludges and workarounds for a problem that should exist in the first place. Attached find proof-of-concept patch 3) calls to rad_fork(1) should also result in creation of thread_pool.waiters element -- This doesnt completely solve anything and 2 is still needed. -- rad_fork(1) is meaningless 4) rad_fork() should mark elements in the hash table as deletable when the table is full -- after all, this is what exec_wait == 0 means, right? Perhaps to make it as correct as possible, it should only delete a percentage of the table, sorted by oldest entry. That might be hard. Why have a waiters pool anyway when calling waitpid(0 ? exec_wait == 1 processes get waited upon and their status discarded exec_wait == 0 processes get waited upon and their status kept until an explicit call to rad_waitpid() with the pid. #! /bin/sh /usr/share/dpatch/dpatch-run ## 360-rad_check_ts_waitpid.dpatch by <joe@nameserver3.ttec.com> ## ## All lines beginning with `## DP:' are a description of the patch. ## DP: No description. @DPATCH@ diff -urNad freeradius-2.0.0~/src/main/threads.c freeradius-2.0.0/src/main/threads.c --- freeradius-2.0.0~/src/main/threads.c 2007-01-17 09:00:25.000000000 -0500 +++ freeradius-2.0.0/src/main/threads.c 2007-01-17 09:02:37.000000000 -0500 @@ -113,10 +113,12 @@ RAD_REQUEST_FUNP fun; } request_queue_t; +#define MAX_WAITERS 1024 typedef struct thread_fork_t { pid_t pid; int status; int exited; + int reaper_created; } thread_fork_t; @@ -257,10 +259,10 @@ pid_t pid; int status; thread_fork_t mytf, *tf; - - if (lrad_hash_table_num_elements(thread_pool.waiters) == 0) return; - - pthread_mutex_lock(&thread_pool.wait_mutex); + int num_waiters = lrad_hash_table_num_elements(thread_pool.waiters); + + if (!num_waiters) + return; while (1) { pid = waitpid(0, &status, WNOHANG); @@ -268,12 +270,26 @@ mytf.pid = pid; tf = lrad_hash_table_finddata(thread_pool.waiters, &mytf); - if (!tf) continue; - - tf->status = status; + if (!tf) + { + tf = rad_malloc(sizeof(*tf)); + memset(tf, 0, sizeof(*tf)); + tf->pid = pid; + tf->status = status; + tf->exited = 1; + tf->reaper_created = 1; + if (!lrad_hash_table_insert(thread_pool.waiters, tf)) + { + free(tf); + tf = NULL; + } + if (++num_waiters >= MAX_WAITERS) + break; + continue; + } tf->exited = 1; + tf->status = status; } - pthread_mutex_unlock(&thread_pool.wait_mutex); } @@ -1049,16 +1065,29 @@ /* * Thread wrapper for fork(). */ +static int reap_reaper_children(void * context, void * data) +{ + thread_fork_t *tf = data; + if (tf->reaper_created) + lrad_hash_table_delete(thread_pool.waiters, tf); + return 0; +} + pid_t rad_fork(int exec_wait) { pid_t child_pid; + int num_waiters; if (exec_wait) return fork(); reap_children(); /* be nice to non-wait thingies */ - if (lrad_hash_table_num_elements(thread_pool.waiters) >= 1024) { - return -1; + if (lrad_hash_table_num_elements(thread_pool.waiters) >= MAX_WAITERS) { + lrad_hash_table_walk(thread_pool.waiters, reap_reaper_children, NULL); + if ((num_waiters = lrad_hash_table_num_elements(thread_pool.waiters)) >= MAX_WAITERS) { + radlog(L_ERR, "too many child waiters: %d", num_waiters); + return -1; + } } /* @@ -1104,8 +1133,8 @@ pid_t rad_waitpid(pid_t pid, int *status, int options) { thread_fork_t mytf, *tf; + pid_t child_pid = 0; - reap_children(); /* be nice to non-wait thingies */ if (pid <= 0) return -1; @@ -1117,22 +1146,21 @@ tf = lrad_hash_table_finddata(thread_pool.waiters, &mytf); if (!tf) { /* not found. It's a problem... */ - pthread_mutex_unlock(&thread_pool.wait_mutex); - return waitpid(pid, status, options); + child_pid = waitpid(pid, status, options); } - - if (tf->exited) { + else if (tf->exited) { *status = tf->status; lrad_hash_table_delete(thread_pool.waiters, &mytf); - pthread_mutex_unlock(&thread_pool.wait_mutex); - return pid; + child_pid = pid; } - /* - * Don't wait, and it hasn't exited. Return. + * else { + * Don't wait, and it hasn't exited. Return 0. + * } */ + reap_children(); /* be nice to non-wait thingies */ pthread_mutex_unlock(&thread_pool.wait_mutex); - return 0; + return child_pid; } #else /* HAVE_PTHREAD_H */