You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by wr...@apache.org on 2007/09/28 23:04:48 UTC
svn commit: r580486 - in /apr/apr/trunk: include/apr_thread_proc.h
threadproc/unix/proc.c
Author: wrowe
Date: Fri Sep 28 14:04:47 2007
New Revision: 580486
URL: http://svn.apache.org/viewvc?rev=580486&view=rev
Log:
Introduce APR_NO_FILE as an option for any of the three stdio streams
to cause the specified streams to be closed to the child process,
when the caller has chosen that flag via apr_procattr_io_set().
ALSO; solve a serious flaw where we attempted to dup2 to a non existant
file if the user had not already called apr_procattr_io_set()!
The Unix implementation.
Modified:
apr/apr/trunk/include/apr_thread_proc.h
apr/apr/trunk/threadproc/unix/proc.c
Modified: apr/apr/trunk/include/apr_thread_proc.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_thread_proc.h?rev=580486&r1=580485&r2=580486&view=diff
==============================================================================
--- apr/apr/trunk/include/apr_thread_proc.h (original)
+++ apr/apr/trunk/include/apr_thread_proc.h Fri Sep 28 14:04:47 2007
@@ -88,11 +88,7 @@
#define APR_CHILD_BLOCK 4
/** @see apr_procattr_io_set
- * @note introduced strictly for Win32 to apr revision 1.2.12 (to restore
- * the non-portable default behavior of 1.2.9 and prior versions on Win32).
- * This becomes portable to all platforms effective revision 1.3.0, ensuring
- * the standard files specified in the call to apr_procattr_io_set are not
- * open in the created process (on Win32 as INVALID_HANDLE_VALUEs).
+ * @note Win32 only effective with version 1.2.12, portably introduced in 1.3.0
*/
#define APR_NO_FILE 8
@@ -401,6 +397,11 @@
* @param in Should stdin be a pipe back to the parent?
* @param out Should stdout be a pipe back to the parent?
* @param err Should stderr be a pipe back to the parent?
+ * @note If APR_NO_PIPE, there will be no special channel, the child
+ * inherit's the parent's stdio stream. If APR_NO_FILE is specified,
+ * that stdio stream is closed in the child (and will be INVALID_HANDLE_VALUE
+ * if inspected on Win32); warning this can have the ugly side effect
+ * that the next file opened may fall into the stdio stream role on Unix.
*/
APR_DECLARE(apr_status_t) apr_procattr_io_set(apr_procattr_t *attr,
apr_int32_t in, apr_int32_t out,
Modified: apr/apr/trunk/threadproc/unix/proc.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/unix/proc.c?rev=580486&r1=580485&r2=580486&view=diff
==============================================================================
--- apr/apr/trunk/threadproc/unix/proc.c (original)
+++ apr/apr/trunk/threadproc/unix/proc.c Fri Sep 28 14:04:47 2007
@@ -20,6 +20,11 @@
#include "apr_signal.h"
#include "apr_random.h"
+/* Heavy on no'ops, here's what we want to pass if there is APR_NO_FILE
+ * requested for a specific child handle;
+ */
+static apr_file_t no_file = { NULL, -1, };
+
APR_DECLARE(apr_status_t) apr_procattr_create(apr_procattr_t **new,
apr_pool_t *pool)
{
@@ -40,7 +45,7 @@
apr_int32_t err)
{
apr_status_t status;
- if (in != 0) {
+ if ((in != APR_NO_PIPE) && (in != APR_NO_FILE)) {
if ((status = apr_file_pipe_create(&attr->child_in, &attr->parent_in,
attr->pool)) != APR_SUCCESS) {
return status;
@@ -60,8 +65,11 @@
apr_file_pipe_timeout_set(attr->parent_in, 0);
}
}
+ else if (in == APR_NO_FILE) {
+ attr->child_in = &no_file;
+ }
- if (out) {
+ if ((out != APR_NO_PIPE) && (out != APR_NO_FILE)) {
if ((status = apr_file_pipe_create(&attr->parent_out, &attr->child_out,
attr->pool)) != APR_SUCCESS) {
return status;
@@ -81,8 +89,11 @@
apr_file_pipe_timeout_set(attr->parent_out, 0);
}
}
+ else if (out == APR_NO_FILE) {
+ attr->child_out = &no_file;
+ }
- if (err) {
+ if ((err != APR_NO_PIPE) && (err != APR_NO_FILE)) {
if ((status = apr_file_pipe_create(&attr->parent_err, &attr->child_err,
attr->pool)) != APR_SUCCESS) {
return status;
@@ -102,6 +113,9 @@
apr_file_pipe_timeout_set(attr->parent_err, 0);
}
}
+ else if (err == APR_NO_FILE) {
+ attr->child_err = &no_file;
+ }
return APR_SUCCESS;
}
@@ -116,11 +130,19 @@
if (attr->child_in == NULL && attr->parent_in == NULL)
rv = apr_file_pipe_create(&attr->child_in, &attr->parent_in, attr->pool);
- if (child_in != NULL && rv == APR_SUCCESS)
- rv = apr_file_dup2(attr->child_in, child_in, attr->pool);
+ if (child_in != NULL && rv == APR_SUCCESS) {
+ if (!attr->child_in || attr->child_in->filedes == -1)
+ rv = apr_file_dup(&attr->child_in, child_in, attr->pool);
+ else
+ rv = apr_file_dup2(attr->child_in, child_in, attr->pool);
+ }
- if (parent_in != NULL && rv == APR_SUCCESS)
- rv = apr_file_dup2(attr->parent_in, parent_in, attr->pool);
+ if (parent_in != NULL && rv == APR_SUCCESS) {
+ if (!attr->parent_in || attr->parent_in->filedes == -1)
+ rv = apr_file_dup(&attr->parent_in, parent_in, attr->pool);
+ else
+ rv = apr_file_dup2(attr->parent_in, parent_in, attr->pool);
+ }
return rv;
}
@@ -135,11 +157,19 @@
if (attr->child_out == NULL && attr->parent_out == NULL)
rv = apr_file_pipe_create(&attr->child_out, &attr->parent_out, attr->pool);
- if (child_out != NULL && rv == APR_SUCCESS)
- rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
+ if (child_out != NULL && rv == APR_SUCCESS) {
+ if (!attr->child_out || attr->child_out->filedes == -1)
+ rv = apr_file_dup(&attr->child_out, child_out, attr->pool);
+ else
+ rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
+ }
- if (parent_out != NULL && rv == APR_SUCCESS)
- rv = apr_file_dup2(attr->parent_out, parent_out, attr->pool);
+ if (parent_out != NULL && rv == APR_SUCCESS) {
+ if (!attr->parent_out || attr->parent_out->filedes == -1)
+ rv = apr_file_dup(&attr->parent_out, parent_out, attr->pool);
+ else
+ rv = apr_file_dup2(attr->parent_out, parent_out, attr->pool);
+ }
return rv;
}
@@ -154,11 +184,19 @@
if (attr->child_err == NULL && attr->parent_err == NULL)
rv = apr_file_pipe_create(&attr->child_err, &attr->parent_err, attr->pool);
- if (child_err != NULL && rv == APR_SUCCESS)
- rv = apr_file_dup2(attr->child_err, child_err, attr->pool);
+ if (child_out != NULL && rv == APR_SUCCESS) {
+ if (!attr->child_out || attr->child_out->filedes == -1)
+ rv = apr_file_dup(&attr->child_out, child_out, attr->pool);
+ else
+ rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
+ }
- if (parent_err != NULL && rv == APR_SUCCESS)
- rv = apr_file_dup2(attr->parent_err, parent_err, attr->pool);
+ if (parent_out != NULL && rv == APR_SUCCESS) {
+ if (!attr->parent_out || attr->parent_out->filedes == -1)
+ rv = apr_file_dup(&attr->parent_out, parent_out, attr->pool);
+ else
+ rv = apr_file_dup2(attr->parent_out, parent_out, attr->pool);
+ }
return rv;
}
@@ -402,19 +440,28 @@
apr_pool_cleanup_for_exec();
- if (attr->child_in) {
+ if ((attr->child_in) && (attr->child_in->filedes == -1) {
+ close(STDIN_FILENO);
+ }
+ else if (attr->child_in) {
apr_file_close(attr->parent_in);
dup2(attr->child_in->filedes, STDIN_FILENO);
apr_file_close(attr->child_in);
}
- if (attr->child_out) {
+ if ((attr->child_out) && (attr->child_out->filedes == -1) {
+ close(STDOUT_FILENO);
+ }
+ else if (attr->child_out) {
apr_file_close(attr->parent_out);
dup2(attr->child_out->filedes, STDOUT_FILENO);
apr_file_close(attr->child_out);
}
- if (attr->child_err) {
+ if ((attr->child_err) && (attr->child_err->filedes == -1) {
+ close(STDERR_FILENO);
+ }
+ else if (attr->child_err) {
apr_file_close(attr->parent_err);
dup2(attr->child_err->filedes, STDERR_FILENO);
apr_file_close(attr->child_err);
@@ -551,15 +598,15 @@
}
/* Parent process */
- if (attr->child_in) {
+ if ((attr->child_in && (attr->child_in->filedes == -1)) {
apr_file_close(attr->child_in);
}
- if (attr->child_out) {
+ if ((attr->child_out) && (attr->child_out->filedes == -1) {
apr_file_close(attr->child_out);
}
- if (attr->child_err) {
+ if ((attr->child_err) && (attr->child_err->filedes == -1) {
apr_file_close(attr->child_err);
}
Re: svn commit: r580486 - in /apr/apr/trunk: include/apr_thread_proc.h
threadproc/unix/proc.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 09/29/2007 12:53 AM, Ruediger Pluem wrote:
>
> On 09/28/2007 11:04 PM, wrowe@apache.org wrote:
>> Author: wrowe
>> Date: Fri Sep 28 14:04:47 2007
>> New Revision: 580486
>>
>> URL: http://svn.apache.org/viewvc?rev=580486&view=rev
>> Log:
>> Introduce APR_NO_FILE as an option for any of the three stdio streams
>> to cause the specified streams to be closed to the child process,
>> when the caller has chosen that flag via apr_procattr_io_set().
>>
>> ALSO; solve a serious flaw where we attempted to dup2 to a non existant
>> file if the user had not already called apr_procattr_io_set()!
>>
>> The Unix implementation.
>>
>> Modified:
>> apr/apr/trunk/include/apr_thread_proc.h
>> apr/apr/trunk/threadproc/unix/proc.c
>>
>
>> Modified: apr/apr/trunk/threadproc/unix/proc.c
>> URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/unix/proc.c?rev=580486&r1=580485&r2=580486&view=diff
>> ==============================================================================
>> --- apr/apr/trunk/threadproc/unix/proc.c (original)
>> +++ apr/apr/trunk/threadproc/unix/proc.c Fri Sep 28 14:04:47 2007
>
>> @@ -551,15 +598,15 @@
>> }
>>
>> /* Parent process */
>> - if (attr->child_in) {
>> + if ((attr->child_in && (attr->child_in->filedes == -1)) {
>> apr_file_close(attr->child_in);
>> }
>>
>> - if (attr->child_out) {
>> + if ((attr->child_out) && (attr->child_out->filedes == -1) {
>> apr_file_close(attr->child_out);
>> }
>>
>> - if (attr->child_err) {
>> + if ((attr->child_err) && (attr->child_err->filedes == -1) {
>> apr_file_close(attr->child_err);
>> }
>
>
> Shouldn't this be filedes != -1 instead?
> I see no sense in closing invalid fds while keeping valid ones open that
> would have been closed before the patch.
Ping?
Regards
Rüdiger
Re: svn commit: r580486 - in /apr/apr/trunk: include/apr_thread_proc.h
threadproc/unix/proc.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 09/28/2007 11:04 PM, wrowe@apache.org wrote:
> Author: wrowe
> Date: Fri Sep 28 14:04:47 2007
> New Revision: 580486
>
> URL: http://svn.apache.org/viewvc?rev=580486&view=rev
> Log:
> Introduce APR_NO_FILE as an option for any of the three stdio streams
> to cause the specified streams to be closed to the child process,
> when the caller has chosen that flag via apr_procattr_io_set().
>
> ALSO; solve a serious flaw where we attempted to dup2 to a non existant
> file if the user had not already called apr_procattr_io_set()!
>
> The Unix implementation.
>
> Modified:
> apr/apr/trunk/include/apr_thread_proc.h
> apr/apr/trunk/threadproc/unix/proc.c
>
> Modified: apr/apr/trunk/threadproc/unix/proc.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/unix/proc.c?rev=580486&r1=580485&r2=580486&view=diff
> ==============================================================================
> --- apr/apr/trunk/threadproc/unix/proc.c (original)
> +++ apr/apr/trunk/threadproc/unix/proc.c Fri Sep 28 14:04:47 2007
> @@ -551,15 +598,15 @@
> }
>
> /* Parent process */
> - if (attr->child_in) {
> + if ((attr->child_in && (attr->child_in->filedes == -1)) {
> apr_file_close(attr->child_in);
> }
>
> - if (attr->child_out) {
> + if ((attr->child_out) && (attr->child_out->filedes == -1) {
> apr_file_close(attr->child_out);
> }
>
> - if (attr->child_err) {
> + if ((attr->child_err) && (attr->child_err->filedes == -1) {
> apr_file_close(attr->child_err);
> }
Shouldn't this be filedes != -1 instead?
I see no sense in closing invalid fds while keeping valid ones open that
would have been closed before the patch.
Regards
Rüdiger