[BZ,#18433] Check file access/existence before forking.
Commit Message
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.
>
Comments
There are various places where you're missing the space before the '(' of
a function call.
POSIX specifies that no library function sets errno to 0.
Typically when using a fallback from __ASSUME_PIPE2 the first fallback
would be to try calling __pipe2 even if !__ASSUME_PIPE2 - and if it fails
with an errno setting other than ENOSYS, then treat that as an error from
which you don't try to fall back, but if it fails with ENOSYS, fall back
to __pipe. In addition, such code in the !__ASSUME_PIPE2 case uses the
variable __have_pipe2 (0 if pipe2 availability unknown, 1 if it's been
tried and didn't fail with ENOSYS, -1 if it's been tried and did fail with
ENOSYS. See various existing examples.
@@ -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):
@@ -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);
}