From patchwork Thu Jun 29 15:01:18 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 21330 Received: (qmail 41990 invoked by alias); 29 Jun 2017 15:01:31 -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 41828 invoked by uid 89); 29 Jun 2017 15:01:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=11730 X-HELO: mail-qt0-f172.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=4nlIYCHSXdJupqX/rJg51LAnPjmwexmzaoIDFyGU3ww=; b=D5UBbm0LQ/k9bw2qwzNP1MaK5es1Ew9hik+CDCP3qiioAfQbZJ2JTcktPfxRKQfY0u Ishkvg/PIQePugosI5z0AwcYX5uhssVEV/W045KsHL1QOq+nN4lzPNQMBRiPgVcsstRX yNMBjOBBS7SAWSOD3Euq51wA8cQcuOTzcHpr/LWY9HjBrvF9hclxQxNDMhaaXWIljeUC 4JWfZ0dd9JlwEmy4p2JyUKGuDAvpCRXy34sWPDCe1vKkqpXL6WgYCBThilnO3jIFMN2n aEMJEm0JcghzjD1pgpn20jd/CsHDF6zpKOSOhEnPKVem/c9ONHj7GF9br/yvrsg5qjSC O1gg== X-Gm-Message-State: AKS2vOyRe+GV9iYYGoxmGg45oRT8KSvz4TJLWgMWdPLOFPFEavSA+RxS f3g+fJuZ7GvC9Qm32km9+Q== X-Received: by 10.237.34.39 with SMTP id n36mr19892665qtc.29.1498748481709; Thu, 29 Jun 2017 08:01:21 -0700 (PDT) Subject: Re: [PATCH 2/3] posix: Improve default posix_spawn implementation To: Andreas Schwab Cc: libc-alpha@sourceware.org References: <1494876985-21990-1-git-send-email-adhemerval.zanella@linaro.org> <1494876985-21990-2-git-send-email-adhemerval.zanella@linaro.org> From: Adhemerval Zanella Message-ID: Date: Thu, 29 Jun 2017 12:01:18 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: On 29/06/2017 11:22, Andreas Schwab wrote: > On Mai 15 2017, Adhemerval Zanella wrote: > >> + if ((ret = close_not_cancel (new_fd) != 0)) > > This assigns the wrong value to ret. Ugh, this should not be here... > >> +fail: >> + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ >> + ret = -ret; > > posix_spwan is supposed to return errno, but none of the arms going here > set ret appropriately. Fixed below. > >> + /* Generate the new process. */ >> + pid_t new_pid = __fork (); >> + >> + if (new_pid == 0) >> + __spawni_child (&args); >> + else if (new_pid > 0) >> + { >> + __close (args.pipe[1]); >> + >> + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) >> + ec = 0; >> + else >> + __waitpid (new_pid, &(int) { 0 }, 0); >> } >> - while (*p++ != '\0'); >> + else >> + ec = -new_pid; > > new_pid isn't an errno either. Since posix_spawn is not support to set errno in case of failure we will need to save/restore for fork call. And this should be fixed on Linux implementation as well since clone will also clobber errno in case of an error (I will send a fix). Ideally I think the exported clone should not set errno and just return errno as a negative number. > > Andreas. > I intended to push this change: diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c index 1979830..93767a2 100644 --- a/sysdeps/posix/spawni.c +++ b/sysdeps/posix/spawni.c @@ -117,30 +117,30 @@ __spawni_child (void *arguments) if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER)) == POSIX_SPAWN_SETSCHEDPARAM) { - if ((ret = __sched_setparam (0, &attr->__sp)) == -1) + if (__sched_setparam (0, &attr->__sp) == -1) goto fail; } else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0) { - if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1)) + if (__sched_setscheduler (0, attr->__policy, &attr->__sp) == -1) goto fail; } #endif /* Set the process session ID. */ if ((attr->__flags & POSIX_SPAWN_SETSID) != 0 - && (ret = __setsid ()) < 0) + && __setsid () < 0) goto fail; /* Set the process group ID. */ if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0 - && (ret =__setpgid (0, attr->__pgrp)) != 0) + && __setpgid (0, attr->__pgrp) != 0) goto fail; /* Set the effective user and group IDs. */ if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0 - && ((ret = local_seteuid (__getuid ())) != 0 - || (ret = local_setegid (__getgid ())) != 0)) + && (local_seteuid (__getuid ()) != 0 + || local_setegid (__getgid ())) != 0) goto fail; /* Execute the file actions. */ @@ -168,10 +168,7 @@ __spawni_child (void *arguments) /* Only signal errors for file descriptors out of range. */ if (action->action.close_action.fd < 0 || action->action.close_action.fd >= fdlimit.rlim_cur) - { - ret = -1; - goto fail; - } + goto fail; } break; @@ -190,25 +187,25 @@ __spawni_child (void *arguments) | O_LARGEFILE, action->action.open_action.mode); - if ((ret = new_fd) == -1) + if (new_fd == -1) goto fail; /* Make sure the desired file descriptor is used. */ if (new_fd != action->action.open_action.fd) { - if ((ret = __dup2 (new_fd, action->action.open_action.fd)) + if (__dup2 (new_fd, action->action.open_action.fd) != action->action.open_action.fd) goto fail; - if ((ret = close_not_cancel (new_fd) != 0)) + if (close_not_cancel (new_fd) != 0) goto fail; } } break; case spawn_do_dup2: - if ((ret = __dup2 (action->action.dup2_action.fd, - action->action.dup2_action.newfd)) + if (__dup2 (action->action.dup2_action.fd, + action->action.dup2_action.newfd) != action->action.dup2_action.newfd) goto fail; break; @@ -228,12 +225,15 @@ __spawni_child (void *arguments) (2.15). */ maybe_script_execute (args); - ret = -errno; - fail: - /* Since sizeof errno < PIPE_BUF, the write is atomic. */ - ret = -ret; + /* errno should have an appropriate non-zero value; otherwise, + there's a bug in glibc or the kernel. For lack of an error code + (EINTERNALBUG) describing that, use ECHILD. Another option would + be to set args->err to some negative sentinel and have the parent + abort(), but that seems needlessly harsh. */ + ret = errno ? : ECHILD; if (ret) + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0); _exit (SPAWN_ERROR); @@ -278,6 +278,7 @@ __spawnix (pid_t *pid, const char *file, args.xflags = xflags; /* Generate the new process. */ + int old_errno = errno; pid_t new_pid = __fork (); if (new_pid == 0) @@ -292,7 +293,8 @@ __spawnix (pid_t *pid, const char *file, __waitpid (new_pid, &(int) { 0 }, 0); } else - ec = -new_pid; + ec = errno; + errno = old_errno; __close (args.pipe[0]);