From patchwork Fri Sep 18 14:54:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Navid Rahimi X-Patchwork-Id: 8783 Received: (qmail 26774 invoked by alias); 18 Sep 2015 14:54:16 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 26761 invoked by uid 89); 18 Sep 2015 14:54:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-lb0-f171.google.com MIME-Version: 1.0 X-Received: by 10.112.36.196 with SMTP id s4mr3340263lbj.59.1442588050328; Fri, 18 Sep 2015 07:54:10 -0700 (PDT) In-Reply-To: <55FC235E.6030608@redhat.com> References: <55F19819.3010601@gmail.com> <55F19B66.9050001@arm.com> <55F19C50.3010502@gmail.com> <1441909606.2948.25.camel@pbcl.net> <55F299F4.6030907@arm.com> <55FC235E.6030608@redhat.com> Date: Fri, 18 Sep 2015 19:24:10 +0430 Message-ID: Subject: Re: [PATCH] [BZ #18433] Check file access/existence before forking. From: navid Rahimi To: "Carlos O'Donell" Cc: Szabolcs Nagy , Phil Blundell , "libc-alpha@sourceware.org" 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 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 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 + + [BZ #18433] + * sysdeps/posix/spawni.c (__spawni): Check child status. + 2015-09-12 Rasmus Villemoes * 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); }