This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] [BZ #18433] Check file access/existence before forking.
- From: navid Rahimi <rahimi dot nv at gmail dot com>
- To: "Carlos O'Donell" <carlos at redhat dot com>
- Cc: Szabolcs Nagy <szabolcs dot nagy at arm dot com>, Phil Blundell <pb at pbcl dot net>, "libc-alpha at sourceware dot org" <libc-alpha at sourceware dot org>
- Date: Fri, 18 Sep 2015 19:24:10 +0430
- Subject: Re: [PATCH] [BZ #18433] Check file access/existence before forking.
- Authentication-results: sourceware.org; auth=none
- References: <55F19819 dot 3010601 at gmail dot com> <55F19B66 dot 9050001 at arm dot com> <55F19C50 dot 3010502 at gmail dot com> <1441909606 dot 2948 dot 25 dot camel at pbcl dot net> <CAOUBrm2Kjbk3q+QJACYG24=p1dz60JXimxYG3oRRz2ehpocwkQ at mail dot gmail dot com> <55F299F4 dot 6030907 at arm dot com> <55FC235E dot 6030608 at redhat dot com>
I developed v3 of patch which uses pipe2 syscall and pipe in fallback
mode , but I didn't get any response for a while. I chose to use pipe2
apprach becasue :
1.most of platform nowdays have pipe2 implementation.
2.if they does not have , then pipe would be enough.
3.people seems have so many problem with vfork.
p.s. I publish it in new thread (since new version should be in new
thread) in mailing list but nobody seems interested.(bugzilla too)
best wishes,
-navid
On Fri, Sep 18, 2015 at 7:14 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 09/11/2015 05:08 AM, Szabolcs Nagy wrote:
>> On 10/09/15 22:45, navid Rahimi wrote:
>>> On Thu, Sep 10, 2015 at 10:56 PM, Phil Blundell <pb@pbcl.net> wrote:
>>>> On Thu, 2015-09-10 at 19:35 +0430, Navid Rahimi wrote:
>>>>> I think our main objection here is to avoid forking when there is no
>>>>> file.There is so many other variable for checking if execve is going to
>>>>> success or not.
>>>>
>>>> On the other hand, your patch will add an extra system call and
>>>> directory lookup to every successful posix_spawn() call, i.e. you are
>>>> optimising the failure case at the expense of the successful case. It's
>>>> not at all obvious to me that this is a sensible thing to do. Can you
>>>> explain your reasoning in a bit more detail?
>>>>
>>>> p.
>>>>
>>>>
>>>
>>> I agree with you completely about overhead and extra syscall , but
>>> there is no other option if we want to check fork. About optimizing
>>
>> there are other options.
>>
>> for example you can correctly check if execve returned
>> and report an error then instead of doing approximations
>> of the right check.
>>
>> you can report the error through a cloexec pipe to the parent.
>>
>> note that a correct posix_spawn implementation never uses
>> fork for QoI reasons (that's the point of posix_spawn, so
>> it works on nommu, large multi-thread applications etc.)
>> and vfork has the wrong semantics for c code so there are
>> other issues here.
>
> Note that it is possible to use vfork in certain conditions,
> and we do in glibc. So one should not entirely dismiss vfork,
> but that's slightly off topic.
>
> c.
>
diff --git a/ChangeLog b/ChangeLog
index eb731cc..b8a1efb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-09-13 Navid Rahimi <rahimi.nv@gmail.com>
+
+ [BZ #18433]
+ * sysdeps/posix/spawni.c (__spawni): Check child status.
+
2015-09-12 Rasmus Villemoes <rv@rasmusvillemoes.dk>
* sysdeps/unix/sysv/linux/getsysstats.c (__get_phys_pages):
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
index eee9331..8994216 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -89,6 +89,24 @@ __spawni (pid_t *pid, const char *file,
char *path, *p, *name;
size_t len;
size_t pathlen;
+ int pipefd[2];
+
+ errno = 0;
+ /* Open Read/Write pipe for parent/child communication. */
+#ifdef __ASSUME_PIPE2
+ if (__pipe2 (pipefd, O_CLOEXEC))
+ return errno;
+#else
+ if (__pipe (pipefd))
+ return errno;
+ if (__fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) == -1
+ || __fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) == -1)
+ {
+ close(pipefd[0]);
+ close(pipefd[1]);
+ return errno;
+ }
+#endif
/* Do this once. */
short int flags = attrp == NULL ? 0 : attrp->__flags;
@@ -109,20 +127,26 @@ __spawni (pid_t *pid, const char *file,
if (new_pid != 0)
{
- if (new_pid < 0)
+ __close (pipefd[1]);
+ if (new_pid < 0){
+ __close (pipefd[0]);
return errno;
-
+ }
/* The call was successful. Store the PID if necessary. */
if (pid != NULL)
*pid = new_pid;
- return 0;
+ __read (pipefd[0], &errno, sizeof errno);
+ __close (pipefd[0]);
+ return errno;
}
+ else
+ __close (pipefd[0]);
/* Set signal mask. */
if ((flags & POSIX_SPAWN_SETSIGMASK) != 0
&& __sigprocmask (SIG_SETMASK, &attrp->__ss, NULL) != 0)
- _exit (SPAWN_ERROR);
+ goto fail;
/* Set signal default action. */
if ((flags & POSIX_SPAWN_SETSIGDEF) != 0)
@@ -140,7 +164,7 @@ __spawni (pid_t *pid, const char *file,
for (sig = 1; sig <= _NSIG; ++sig)
if (__sigismember (&attrp->__sd, sig) != 0
&& __sigaction (sig, &sa, NULL) != 0)
- _exit (SPAWN_ERROR);
+ goto fail;
}
@@ -150,25 +174,25 @@ __spawni (pid_t *pid, const char *file,
== POSIX_SPAWN_SETSCHEDPARAM)
{
if (__sched_setparam (0, &attrp->__sp) == -1)
- _exit (SPAWN_ERROR);
+ goto fail;
}
else if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0)
{
if (__sched_setscheduler (0, attrp->__policy, &attrp->__sp) == -1)
- _exit (SPAWN_ERROR);
+ goto fail;
}
#endif
/* Set the process group ID. */
if ((flags & POSIX_SPAWN_SETPGROUP) != 0
&& __setpgid (0, attrp->__pgrp) != 0)
- _exit (SPAWN_ERROR);
+ goto fail;
/* Set the effective user and group IDs. */
if ((flags & POSIX_SPAWN_RESETIDS) != 0
&& (local_seteuid (__getuid ()) != 0
|| local_setegid (__getgid ()) != 0))
- _exit (SPAWN_ERROR);
+ goto fail;
/* Execute the file actions. */
if (file_actions != NULL)
@@ -196,7 +220,7 @@ __spawni (pid_t *pid, const char *file,
if (action->action.close_action.fd < 0
|| action->action.close_action.fd >= fdlimit.rlim_cur)
/* Signal the error. */
- _exit (SPAWN_ERROR);
+ goto fail;
}
break;
@@ -209,7 +233,7 @@ __spawni (pid_t *pid, const char *file,
if (new_fd == -1)
/* The `open' call failed. */
- _exit (SPAWN_ERROR);
+ goto fail;
/* Make sure the desired file descriptor is used. */
if (new_fd != action->action.open_action.fd)
@@ -217,11 +241,11 @@ __spawni (pid_t *pid, const char *file,
if (__dup2 (new_fd, action->action.open_action.fd)
!= action->action.open_action.fd)
/* The `dup2' call failed. */
- _exit (SPAWN_ERROR);
+ goto fail;
if (close_not_cancel (new_fd) != 0)
/* The `close' call failed. */
- _exit (SPAWN_ERROR);
+ goto fail;
}
}
break;
@@ -231,7 +255,7 @@ __spawni (pid_t *pid, const char *file,
action->action.dup2_action.newfd)
!= action->action.dup2_action.newfd)
/* The `dup2' call failed. */
- _exit (SPAWN_ERROR);
+ goto fail;
break;
}
}
@@ -245,7 +269,7 @@ __spawni (pid_t *pid, const char *file,
maybe_script_execute (file, argv, envp, xflags);
/* Oh, oh. `execve' returns. This is bad. */
- _exit (SPAWN_ERROR);
+ goto fail;
}
/* We have to search for FILE on the path. */
@@ -304,11 +328,15 @@ __spawni (pid_t *pid, const char *file,
/* Some other error means we found an executable file, but
something went wrong executing it; return the error to our
caller. */
- _exit (SPAWN_ERROR);
+ goto fail;
}
}
while (*p++ != '\0');
+ fail:
+ /* Send parent what was the reason of failure. */
+ __write (pipefd[1], &errno, sizeof errno);
+ __close (pipefd[1]);
/* Return with an error. */
_exit (SPAWN_ERROR);
}