You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by mt...@apache.org on 2009/02/21 21:59:47 UTC
svn commit: r746589 - in /apr/apr/trunk: ./ include/ include/arch/unix/
include/arch/win32/ misc/unix/ misc/win32/ threadproc/beos/
threadproc/netware/ threadproc/os2/ threadproc/unix/ threadproc/win32/
Author: mturk
Date: Sat Feb 21 20:59:46 2009
New Revision: 746589
URL: http://svn.apache.org/viewvc?rev=746589&view=rev
Log:
Add simple parent/child data exchange for APR processes
Modified:
apr/apr/trunk/CHANGES
apr/apr/trunk/include/apr_thread_proc.h
apr/apr/trunk/include/arch/unix/apr_arch_threadproc.h
apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h
apr/apr/trunk/misc/unix/start.c
apr/apr/trunk/misc/win32/start.c
apr/apr/trunk/threadproc/beos/proc.c
apr/apr/trunk/threadproc/netware/proc.c
apr/apr/trunk/threadproc/os2/proc.c
apr/apr/trunk/threadproc/unix/proc.c
apr/apr/trunk/threadproc/win32/proc.c
Modified: apr/apr/trunk/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/CHANGES [utf-8] (original)
+++ apr/apr/trunk/CHANGES [utf-8] Sat Feb 21 20:59:46 2009
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes for APR 2.0.0
+ *) Added simple interprocess data exchange API between
+ parent and child APR based processes.
+ [Mladen Turk]
+
*) Added Unix domain socket support.
[Mladen Turk]
Modified: apr/apr/trunk/include/apr_thread_proc.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_thread_proc.h?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/include/apr_thread_proc.h (original)
+++ apr/apr/trunk/include/apr_thread_proc.h Sat Feb 21 20:59:46 2009
@@ -590,6 +590,41 @@
void *data,
apr_fileperms_t perms);
+/**
+ * Set the ipc data for child process
+ * @param attr The procattr we care about.
+ * @param data Data to pass to child process
+ * @param size Data size
+ */
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_set(apr_procattr_t *attr,
+ const void *data,
+ apr_size_t size);
+
+/**
+ * Set the shared memory ipc data for child process
+ * @param attr The procattr we care about.
+ * @param data Shared memory data pointer
+ * @param size Shared memory data size
+ * @remark data points to the shared memory segment created by the
+ * parent process and initially contains the copy of the data set by
+ * apr_procattr_ipc_data_set function. The data is valid only after
+ * the child process was created.
+ */
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_get(apr_procattr_t *attr,
+ void **data,
+ apr_size_t *size);
+
+/**
+ * Get the shared memory ipc data segment pointer
+ * @param data Shared memory data pointer
+ * @param size Shared memory data size
+ * @remark data points to the shared memory segment created by the
+ * parent process. If the parent process didn't create ipc data
+ * the function returns APR_ENOMEM.
+ */
+APR_DECLARE(apr_status_t) apr_proc_parent_ipc_data_get(void **data,
+ apr_size_t *size);
+
#if APR_HAS_FORK
/**
* This is currently the only non-portable call in APR. This executes
Modified: apr/apr/trunk/include/arch/unix/apr_arch_threadproc.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/arch/unix/apr_arch_threadproc.h?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/include/arch/unix/apr_arch_threadproc.h (original)
+++ apr/apr/trunk/include/arch/unix/apr_arch_threadproc.h Sat Feb 21 20:59:46 2009
@@ -19,6 +19,8 @@
#include "apr_thread_proc.h"
#include "apr_file_io.h"
#include "apr_arch_file_io.h"
+#include "apr_arch_proc_mutex.h"
+#include "apr_arch_shm.h"
#include "apr_perms_set.h"
/* System headers required for thread/process library */
@@ -113,7 +115,13 @@
apr_uid_t uid;
apr_gid_t gid;
apr_procattr_pscb_t *perms_set_callbacks;
+
+ apr_shm_t *ipc_shm;
+ const void *ipc_data;
+ apr_size_t ipc_size;
};
+apr_status_t apr_proc_ipc_init(apr_pool_t *pool);
+
#endif /* ! THREAD_PROC_H */
Modified: apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h (original)
+++ apr/apr/trunk/include/arch/win32/apr_arch_threadproc.h Sat Feb 21 20:59:46 2009
@@ -17,6 +17,8 @@
#include "apr_private.h"
#include "apr_thread_proc.h"
#include "apr_file_io.h"
+#include "apr_arch_shm.h"
+#include "apr_perms_set.h"
#ifndef THREAD_PROC_H
#define THREAD_PROC_H
@@ -62,6 +64,9 @@
LPSECURITY_ATTRIBUTES sa;
LPVOID sd;
#endif
+ apr_shm_t *ipc_shm;
+ const void *ipc_data;
+ apr_size_t ipc_size;
};
struct apr_thread_once_t {
@@ -69,6 +74,7 @@
};
extern apr_status_t apr_threadproc_init(apr_pool_t *pool);
+extern apr_status_t apr_proc_ipc_init(apr_pool_t *pool);
#endif /* ! THREAD_PROC_H */
Modified: apr/apr/trunk/misc/unix/start.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/misc/unix/start.c?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/misc/unix/start.c (original)
+++ apr/apr/trunk/misc/unix/start.c Sat Feb 21 20:59:46 2009
@@ -22,6 +22,7 @@
#include "apr_arch_proc_mutex.h" /* for apr_proc_mutex_unix_setup_lock() */
#include "apr_arch_internal_time.h"
+#include "apr_arch_threadproc.h" /* for apr_procattr_ipc_after_init() */
APR_DECLARE(apr_status_t) apr_app_initialize(int *argc,
@@ -70,6 +71,9 @@
apr_signal_init(pool);
+ /* Try to initialize the shared memory if this is a child process.
+ */
+ apr_proc_ipc_init(pool);
return APR_SUCCESS;
}
Modified: apr/apr/trunk/misc/win32/start.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/misc/win32/start.c?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/misc/win32/start.c (original)
+++ apr/apr/trunk/misc/win32/start.c Sat Feb 21 20:59:46 2009
@@ -206,6 +206,7 @@
apr_signal_init(pool);
apr_threadproc_init(pool);
+ apr_proc_ipc_init(pool);
return APR_SUCCESS;
}
Modified: apr/apr/trunk/threadproc/beos/proc.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/beos/proc.c?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/threadproc/beos/proc.c (original)
+++ apr/apr/trunk/threadproc/beos/proc.c Sat Feb 21 20:59:46 2009
@@ -451,3 +451,28 @@
{
return APR_ENOTIMPL;
}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_set(apr_procattr_t *attr,
+ const void *data,
+ apr_size_t size)
+{
+ return APR_ENOTIMPL;
+}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_get(apr_procattr_t *attr,
+ void **data,
+ apr_size_t *size)
+{
+ return APR_ENOTIMPL;
+}
+
+apr_status_t apr_proc_ipc_init(apr_pool_t *pool)
+{
+ return APR_ENOTIMPL;
+}
+
+APR_DECLARE(apr_status_t) apr_proc_parent_ipc_data_get(void **data,
+ apr_size_t *size)
+{
+ return APR_ENOTIMPL;
+}
Modified: apr/apr/trunk/threadproc/netware/proc.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/netware/proc.c?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/threadproc/netware/proc.c (original)
+++ apr/apr/trunk/threadproc/netware/proc.c Sat Feb 21 20:59:46 2009
@@ -499,3 +499,23 @@
{
return APR_ENOTIMPL;
}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_set(apr_procattr_t *attr,
+ const void *data,
+ apr_size_t size)
+{
+ return APR_ENOTIMPL;
+}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_get(apr_procattr_t *attr,
+ void **data,
+ apr_size_t *size)
+{
+ return APR_ENOTIMPL;
+}
+
+APR_DECLARE(apr_status_t) apr_proc_parent_ipc_data_get(void **data,
+ apr_size_t *size)
+{
+ return APR_ENOTIMPL;
+}
Modified: apr/apr/trunk/threadproc/os2/proc.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/os2/proc.c?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/threadproc/os2/proc.c (original)
+++ apr/apr/trunk/threadproc/os2/proc.c Sat Feb 21 20:59:46 2009
@@ -670,3 +670,28 @@
{
return APR_ENOTIMPL;
}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_set(apr_procattr_t *attr,
+ const void *data,
+ apr_size_t size)
+{
+ return APR_ENOTIMPL;
+}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_get(apr_procattr_t *attr,
+ void **data,
+ apr_size_t *size)
+{
+ return APR_ENOTIMPL;
+}
+
+apr_status_t apr_proc_ipc_init(apr_pool_t *pool)
+{
+ return APR_ENOTIMPL;
+}
+
+APR_DECLARE(apr_status_t) apr_proc_parent_ipc_data_get(void **data,
+ apr_size_t *size)
+{
+ return APR_ENOTIMPL;
+}
Modified: apr/apr/trunk/threadproc/unix/proc.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/unix/proc.c?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/threadproc/unix/proc.c (original)
+++ apr/apr/trunk/threadproc/unix/proc.c Sat Feb 21 20:59:46 2009
@@ -19,6 +19,7 @@
#include "apr_portable.h"
#include "apr_signal.h"
#include "apr_random.h"
+#include "apr_arch_proc_mutex.h"
/* Heavy on no'ops, here's what we want to pass if there is APR_NO_FILE
* requested for a specific child handle;
@@ -216,6 +217,69 @@
return APR_SUCCESS;
}
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_set(apr_procattr_t *attr,
+ const void *data,
+ apr_size_t size)
+{
+ attr->ipc_data = data;
+ attr->ipc_size = size;
+ return APR_SUCCESS;
+}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_get(apr_procattr_t *attr,
+ void **data,
+ apr_size_t *size)
+{
+ if (attr->ipc_shm && attr->ipc_size) {
+ *data = apr_shm_baseaddr_get(attr->ipc_shm);
+ *size = attr->ipc_size;
+ return APR_SUCCESS;
+ }
+ else
+ return APR_EINIT;
+}
+
+/* Global ipc data setup if we are child
+ * of the process that used child_data_set
+ * TODO: This can probably be some struct.
+ */
+static void *ipc_shm_data = NULL;
+static apr_size_t ipc_shm_size = 0;
+static int ipc_shm_init = 0;
+
+apr_status_t apr_proc_ipc_init(apr_pool_t *pool)
+{
+ apr_status_t rv;
+ char shmname[64];
+ const char *tmpdir = NULL;
+ apr_shm_t *shm;
+
+ if (ipc_shm_init++)
+ return APR_SUCCESS;
+ apr_temp_dir_get(&tmpdir, pool);
+ apr_snprintf(shmname, sizeof(shmname), "%s/.apripc.%" APR_PID_T_FMT,
+ tmpdir, getpid());
+
+ if ((rv = apr_shm_attach(&shm, shmname, pool)) != APR_SUCCESS)
+ return rv;
+ ipc_shm_data = apr_shm_baseaddr_get(shm);
+ ipc_shm_size = apr_shm_size_get(shm);
+ return APR_SUCCESS;
+}
+
+APR_DECLARE(apr_status_t) apr_proc_parent_ipc_data_get(void **data,
+ apr_size_t *size)
+{
+ if (!ipc_shm_init)
+ return APR_EINIT;
+ if (!ipc_shm_data)
+ return APR_ENOMEM;
+
+ *data = ipc_shm_data;
+ *size = ipc_shm_size;
+ return APR_SUCCESS;
+}
+
APR_DECLARE(apr_status_t) apr_proc_fork(apr_proc_t *proc, apr_pool_t *pool)
{
int pid;
@@ -353,7 +417,9 @@
apr_pool_t *pool)
{
int i;
+ apr_status_t rc;
const char * const empty_envp[] = {NULL};
+ apr_proc_mutex_t *ipc_mutex = NULL;
if (!env) { /* Specs require an empty array instead of NULL;
* Purify will trigger a failure, even if many
@@ -392,6 +458,22 @@
}
}
+ if (attr->ipc_data && attr->ipc_size) {
+ /* Create startup ipc mutex.
+ * The purpose of this mutex is to enable
+ * the 'start suspended' process behaviour.
+ * We need to create the shared memory after
+ * fork() and we don't know in advance what
+ * the child pid will be. Mutex locks the child
+ * until we create the shared memory which
+ * can be then attached at child's apr_initialize.
+ */
+ if ((rc = apr_proc_mutex_create(&ipc_mutex, NULL,
+ APR_LOCK_DEFAULT, attr->pool)) != APR_SUCCESS) {
+ return rc;
+ }
+ apr_proc_mutex_lock(ipc_mutex);
+ }
if ((new->pid = fork()) < 0) {
return errno;
}
@@ -399,6 +481,33 @@
int status;
/* child process */
+ if (ipc_mutex) {
+ if ((rc = apr_proc_mutex_child_init(&ipc_mutex,
+ NULL, pool)) != APR_SUCCESS) {
+ /* This should never happen.
+ * We cannot continue.
+ */
+ if (attr->errfn) {
+ attr->errfn(pool, rc, "ipc mutex init failed");
+ }
+ _exit(-1);
+ }
+ /* Wait util the parent initializes shared memory.
+ */
+ if ((rc = apr_proc_mutex_lock(ipc_mutex)) != APR_SUCCESS) {
+ /* Locking failed.
+ * Probably the parent ended prematurely
+ */
+ if (attr->errfn) {
+ attr->errfn(pool, rc, "ipc mutex lock failed");
+ }
+ _exit(-1);
+ }
+ /* By now we should have the ipc shared memory set up.
+ * We don't need the mutex any more.
+ */
+ apr_proc_mutex_destroy(ipc_mutex);
+ }
/*
* If we do exec cleanup before the dup2() calls to set up pipes
* on 0-2, we accidentally close the pipes used by programs like
@@ -455,6 +564,7 @@
apr_signal(SIGCHLD, SIG_DFL); /* not sure if this is needed or not */
+
if (attr->currdir != NULL) {
if (chdir(attr->currdir) == -1) {
if (attr->errfn) {
@@ -594,6 +704,35 @@
_exit(-1); /* if we get here, there is a problem, so exit with an
* error code. */
}
+ if (ipc_mutex) {
+ char shmname[64];
+ const char *tmpdir = NULL;
+
+ apr_temp_dir_get(&tmpdir, pool);
+ apr_snprintf(shmname, sizeof(shmname), "%s/.apripc.%" APR_PID_T_FMT,
+ tmpdir, new->pid);
+ rc = apr_shm_create(&attr->ipc_shm, attr->ipc_size,
+ shmname, attr->pool);
+ if (rc == APR_SUCCESS) {
+ if (!geteuid()) {
+ /* Set ipc shared memory permissions for
+ * our child process.
+ */
+ APR_PERMS_SET_FN(shm)(attr->ipc_shm,
+ APR_FPROT_UREAD | APR_FPROT_UWRITE,
+ attr->uid, attr->gid);
+ }
+ /* Fill in shared memory with ipc data */
+ memcpy(apr_shm_baseaddr_get(attr->ipc_shm),
+ attr->ipc_data, attr->ipc_size);
+ }
+ /* Unlock the ipc startup mutex.
+ * This mutex is destroyed when attr->pool is cleared
+ * or destroyed because it might not yet be initialised
+ * inside the child.
+ */
+ apr_proc_mutex_unlock(ipc_mutex);
+ }
/* Parent process */
if (attr->child_in && (attr->child_in->filedes != -1)) {
Modified: apr/apr/trunk/threadproc/win32/proc.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/win32/proc.c?rev=746589&r1=746588&r2=746589&view=diff
==============================================================================
--- apr/apr/trunk/threadproc/win32/proc.c (original)
+++ apr/apr/trunk/threadproc/win32/proc.c Sat Feb 21 20:59:46 2009
@@ -341,6 +341,69 @@
return APR_SUCCESS;
}
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_set(apr_procattr_t *attr,
+ const void *data,
+ apr_size_t size)
+{
+ attr->ipc_data = data;
+ attr->ipc_size = size;
+ return APR_SUCCESS;
+}
+
+APR_DECLARE(apr_status_t) apr_procattr_ipc_data_get(apr_procattr_t *attr,
+ void **data,
+ apr_size_t *size)
+{
+ if (attr->ipc_shm && attr->ipc_size) {
+ *data = apr_shm_baseaddr_get(attr->ipc_shm);
+ *size = attr->ipc_size;
+ return APR_SUCCESS;
+ }
+ else
+ return APR_EINIT;
+}
+
+/* Global ipc data setup if we are child
+ * of the process that used child_data_set
+ * TODO: This can probably be some struct.
+ */
+static void *ipc_shm_data = NULL;
+static apr_size_t ipc_shm_size = 0;
+static int ipc_shm_init = 0;
+
+apr_status_t apr_proc_ipc_init(apr_pool_t *pool)
+{
+ apr_status_t rv;
+ char shmname[APR_PATH_MAX];
+ const char *tmpdir = NULL;
+ apr_shm_t *shm;
+
+ if (ipc_shm_init++)
+ return APR_SUCCESS;
+ apr_temp_dir_get(&tmpdir, pool);
+ apr_snprintf(shmname, sizeof(shmname), "%s\\APRIPC_%d",
+ tmpdir, GetCurrentProcessId());
+
+ if ((rv = apr_shm_attach(&shm, shmname, pool)) != APR_SUCCESS)
+ return rv;
+ ipc_shm_data = apr_shm_baseaddr_get(shm);
+ ipc_shm_size = apr_shm_size_get(shm);
+ return APR_SUCCESS;
+}
+
+APR_DECLARE(apr_status_t) apr_proc_parent_ipc_data_get(void **data,
+ apr_size_t *size)
+{
+ if (!ipc_shm_init)
+ return APR_EINIT;
+ if (!ipc_shm_data)
+ return APR_ENOMEM;
+
+ *data = ipc_shm_data;
+ *size = ipc_shm_size;
+ return APR_SUCCESS;
+}
+
static const char* has_space(const char *str)
{
const char *ch;
@@ -453,6 +516,7 @@
apr_pool_t *pool)
{
apr_status_t rv;
+ apr_status_t rc;
apr_size_t i;
const char *argv0;
char *cmdline;
@@ -782,6 +846,12 @@
if (attr->detached) {
si.dwFlags |= STARTF_USESHOWWINDOW;
si.wShowWindow = SW_HIDE;
+ }
+ if (attr->ipc_data && attr->ipc_size) {
+ /* Child will be suspended until
+ * we create the shared memory
+ */
+ dwCreationFlags |= CREATE_SUSPENDED;
}
#ifndef _WIN32_WCE
@@ -908,6 +978,23 @@
NULL, /* STARTUPINFO not supported */
&pi);
#endif
+
+ if (rv && attr->ipc_data, attr->ipc_size) {
+ char shmname[APR_PATH_MAX];
+ const char *tmpdir = NULL;
+
+ apr_temp_dir_get(&tmpdir, pool);
+ apr_snprintf(shmname, sizeof(shmname), "%s\\APRIPC_%d",
+ tmpdir, pi.dwProcessId);
+ rc = apr_shm_create(&attr->ipc_shm, attr->ipc_size,
+ shmname, attr->pool);
+ if (rc == APR_SUCCESS) {
+ /* Fill in shared memory with ipc data */
+ memcpy(apr_shm_baseaddr_get(attr->ipc_shm),
+ attr->ipc_data, attr->ipc_size);
+ }
+ ResumeThread(pi.hThread);
+ }
}
#endif /* APR_HAS_UNICODE_FS */
#if APR_HAS_ANSI_FS
Re: svn commit: r746589 - in /apr/apr/trunk: ./ include/
include/arch/unix/ include/arch/win32/ misc/unix/ misc/win32/
threadproc/beos/ threadproc/netware/ threadproc/os2/ threadproc/unix/
threadproc/win32/
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Feb 23, 2009 at 13:16, Mladen Turk <mt...@apache.org> wrote:
> Joe Orton wrote:
>...
>> -1
>
> I announced that twice on this list during the last year,
> so you should spoke then thought.
And people miss email. Or they thought it might be implemented
differently. Or WHATEVER.
It doesn't matter. There is no time limit on standing against a
change. It is possible to veto something that was checked in *months*
ago. That change would then have to be backed out before release, or
some solution/compromise reached instead.
I'm also with Joe: stuff like this can be built *on top of* APR,
rather than being part of it. Create a shared memory segment, and pass
a cmdline switch. Or leave a descriptor open with the name of the
segment which the parent/child can write/read. These solutions could
be done as part of apr-util, no?
Cheers,
-g
Re: svn commit: r746589 - in /apr/apr/trunk: ./ include/
include/arch/unix/ include/arch/win32/ misc/unix/ misc/win32/
threadproc/beos/ threadproc/netware/ threadproc/os2/
threadproc/unix/ threadproc/win32/
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Feb 23, 2009 at 04:05:26PM +0100, Mladen Turk wrote:
> Joe Orton wrote:
>>
>>> Also shm name is not predictable because the child pid is not
>>> known in advance (well one can make all 1...PID_MAX shm's, sure),
>>
>> tmp races are a security issue regardless of pid predicatibility (and
>> in fact, pids are predictable on many platforms).
>>
>
> I made it unique. It can still fail in theory during the
> transition from two consequtive api calls
> (file_close and shm_create)
So the race is still there, and if an attacker wins that race, he can
substitute a shm segment containing arbitrary data which the child
process will pick up. -1 stands on that basis.
Using an env var is also completely bizarre. I can't imagine why on
earth an application would want to work like this, rather than e.g.
creating a shm segment in the parent and passing the name as a
command-line option to the exec'ed child - or using a common config
file. Or using hard-coded paths. But trying to shove this all into
apr_proc_create() just seems like a disaster.
Regards, Joe
Re: svn commit: r746589 - in /apr/apr/trunk: ./ include/ include/arch/unix/
include/arch/win32/ misc/unix/ misc/win32/ threadproc/beos/ threadproc/netware/
threadproc/os2/ threadproc/unix/ threadproc/win32/
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
>
>> Also shm name is not predictable because the child pid is not
>> known in advance (well one can make all 1...PID_MAX shm's, sure),
>
> tmp races are a security issue regardless of pid predicatibility (and in
> fact, pids are predictable on many platforms).
>
I made it unique. It can still fail in theory during the
transition from two consequtive api calls
(file_close and shm_create)
If you still insist on -1, I'll have to remove it :(
Regards
--
^(TM)
Re: svn commit: r746589 - in /apr/apr/trunk: ./ include/ include/arch/unix/
include/arch/win32/ misc/unix/ misc/win32/ threadproc/beos/ threadproc/netware/
threadproc/os2/ threadproc/unix/ threadproc/win32/
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
> On Mon, Feb 23, 2009 at 01:16:22PM +0100, Mladen Turk wrote:
>> Joe Orton wrote:
>>> On Sat, Feb 21, 2009 at 08:59:47PM -0000, Mladen Turk wrote:
>>>> Author: mturk
>>>> Date: Sat Feb 21 20:59:46 2009
>>>> New Revision: 746589
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=746589&view=rev
>>>> Log:
>>>> Add simple parent/child data exchange for APR processes
>>> 1) another addition to the procattr API which is entirely orthogonal to
>>> the API's purpose. ick.
>>>
>> This can always be made using some different solutions.
>> However this one IMHO is the most portable one.
>> If you think the apr_procattr is the wrong group I can
>> rename it to something else, but it's related to
>> process, process attributes and ipc, so there's not
>> very much space for something meaningful.
>
> "this HTTP server I just wrote is related to sockets, files, and
> processes, so I'm going to put it in APR"
>
:)
> I don't see why it needs to be in APR at all. APR should not become a
> random dumping ground for utility functions that someone once found
> useful; APR-util has to-date served that function quite well.
>
It cannot be part of apr-util cause it's platform specific.
It is created when we know the child pid but before the child starts.
>
> The parent doesn't fail on the apr_shm_create() error path, it carries
> on regardless. Changing it to fail would downgrade the tmp race from a
> potential escalation of privileges to a DoS; still a security issue.
>
Right it should fail if it cannot create the shm.
>> Also shm name is not predictable because the child pid is not
>> known in advance (well one can make all 1...PID_MAX shm's, sure),
>
> tmp races are a security issue regardless of pid predicatibility (and in
> fact, pids are predictable on many platforms).
>
Well, this can be easily fixed by making unique tmpfile
and adding apripc.<PID>=tempfile to the env.
Regards
--
^(TM)
Re: svn commit: r746589 - in /apr/apr/trunk: ./ include/
include/arch/unix/ include/arch/win32/ misc/unix/ misc/win32/
threadproc/beos/ threadproc/netware/ threadproc/os2/
threadproc/unix/ threadproc/win32/
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Feb 23, 2009 at 01:16:22PM +0100, Mladen Turk wrote:
> Joe Orton wrote:
> > On Sat, Feb 21, 2009 at 08:59:47PM -0000, Mladen Turk wrote:
> >> Author: mturk
> >> Date: Sat Feb 21 20:59:46 2009
> >> New Revision: 746589
> >>
> >> URL: http://svn.apache.org/viewvc?rev=746589&view=rev
> >> Log:
> >> Add simple parent/child data exchange for APR processes
> >
> > 1) another addition to the procattr API which is entirely orthogonal to
> > the API's purpose. ick.
> >
>
> This can always be made using some different solutions.
> However this one IMHO is the most portable one.
> If you think the apr_procattr is the wrong group I can
> rename it to something else, but it's related to
> process, process attributes and ipc, so there's not
> very much space for something meaningful.
"this HTTP server I just wrote is related to sockets, files, and
processes, so I'm going to put it in APR"
I don't see why it needs to be in APR at all. APR should not become a
random dumping ground for utility functions that someone once found
useful; APR-util has to-date served that function quite well.
> > 3) fundamental /tmp race (the temp file name is predictable and hence
> > another local user could create the shm file and populate the data
> > segment which would be picked up by the child) and hence is huge
> > security hole
>
> Hmm, that's the shared memory, not the actual file.
Not sure what you mean there? e.g. with APR_USE_SHMEM_MMAP_TMP they are
one and the same thing
> Nevertheless the parent creates the shm so it will fail
> if it cannot create shm, so the child won't get started.
The parent doesn't fail on the apr_shm_create() error path, it carries
on regardless. Changing it to fail would downgrade the tmp race from a
potential escalation of privileges to a DoS; still a security issue.
> Also shm name is not predictable because the child pid is not
> known in advance (well one can make all 1...PID_MAX shm's, sure),
tmp races are a security issue regardless of pid predicatibility (and in
fact, pids are predictable on many platforms).
Regards, Joe
Re: svn commit: r746589 - in /apr/apr/trunk: ./ include/ include/arch/unix/
include/arch/win32/ misc/unix/ misc/win32/ threadproc/beos/ threadproc/netware/
threadproc/os2/ threadproc/unix/ threadproc/win32/
Posted by Mladen Turk <mt...@apache.org>.
Joe Orton wrote:
> On Sat, Feb 21, 2009 at 08:59:47PM -0000, Mladen Turk wrote:
>> Author: mturk
>> Date: Sat Feb 21 20:59:46 2009
>> New Revision: 746589
>>
>> URL: http://svn.apache.org/viewvc?rev=746589&view=rev
>> Log:
>> Add simple parent/child data exchange for APR processes
>
> 1) another addition to the procattr API which is entirely orthogonal to
> the API's purpose. ick.
>
This can always be made using some different solutions.
However this one IMHO is the most portable one.
If you think the apr_procattr is the wrong group I can
rename it to something else, but it's related to
process, process attributes and ipc, so there's not
very much space for something meaningful.
> 2) every apr_initialize() call for every app in the universe now tries
> to open some random file in /tmp regardless of whether they use this API
>
I suppose the late open on explicit
apr_proc_parent_ipc_data_get would be smarter solution
and wouldn't impose any performance degradation.
> 3) fundamental /tmp race (the temp file name is predictable and hence
> another local user could create the shm file and populate the data
> segment which would be picked up by the child) and hence is huge
> security hole
>
Hmm, that's the shared memory, not the actual file.
Nevertheless the parent creates the shm so it will fail
if it cannot create shm, so the child won't get started.
Also shm name is not predictable because the child pid is not
known in advance (well one can make all 1...PID_MAX shm's, sure),
but then again the child using the api 'knows' it was called
by the parent and data is always valid.
> -1
>
I announced that twice on this list during the last year,
so you should spoke then thought.
Regards
--
^(TM)
Re: svn commit: r746589 - in /apr/apr/trunk: ./ include/
include/arch/unix/ include/arch/win32/ misc/unix/ misc/win32/
threadproc/beos/ threadproc/netware/ threadproc/os2/
threadproc/unix/ threadproc/win32/
Posted by Joe Orton <jo...@redhat.com>.
On Sat, Feb 21, 2009 at 08:59:47PM -0000, Mladen Turk wrote:
> Author: mturk
> Date: Sat Feb 21 20:59:46 2009
> New Revision: 746589
>
> URL: http://svn.apache.org/viewvc?rev=746589&view=rev
> Log:
> Add simple parent/child data exchange for APR processes
1) another addition to the procattr API which is entirely orthogonal to
the API's purpose. ick.
2) every apr_initialize() call for every app in the universe now tries
to open some random file in /tmp regardless of whether they use this API
3) fundamental /tmp race (the temp file name is predictable and hence
another local user could create the shm file and populate the data
segment which would be picked up by the child) and hence is huge
security hole
-1