From patchwork Thu Oct 12 18:39:22 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: 23507 Received: (qmail 72461 invoked by alias); 12 Oct 2017 18:39:33 -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 71039 invoked by uid 89); 12 Oct 2017 18:39:32 -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= X-HELO: mail-qk0-f180.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=gPA4r42B8G2I733feFa78YjgtvIWCdyaQCuMy+CYtl4=; b=S0Zu+FEzcNW0tAjWwdBf7bt3+3nlbXrWHfhLzNM4vb5bom/YdJL6ykr7AU9m6PkOWa uJ80DjXPllRDGaVQ2hN5lDtizd3cKR55T3FFeX6L+selxTVnNc4WsW96zru+iiAmyLZD d6Q2uB6wo0+3OTqYXcvlv3MqqgNSVG3U6929zTqZrstVI6YBwisLRfjVEVZLxFNvpcom T5stBif576gLMi13+p5lQpKBwZnJoJJ/7DW1+DKgi6dzkc4A6XkANdMqpva+Wq8iT60I 5NJaQjx0oTbw3pa/LSDLZOnDZPN0GLCo4MIEsHVesJu1mF/Uw4qBEar8ILaWDgUqS1QA 6Y1Q== X-Gm-Message-State: AMCzsaXO4m64x7rGvYEqHOaYFnR4W8D/JWZTxxbcSDGEBHwPSHLIRI/V sBZiwJ9z8GH2tOjMRNeM+cyQkYkI3TA= X-Google-Smtp-Source: ABhQp+ScN6lomy3BiHo19+yDAG743h73KDRzuBcYU03gUQbKyHIRKNA2kyEGT81MK0Qllzdj2s8GNQ== X-Received: by 10.55.72.75 with SMTP id v72mr1873728qka.92.1507833568543; Thu, 12 Oct 2017 11:39:28 -0700 (PDT) From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH] posix: Fix improper assert in Linux posix_spawn (BZ#22273) Date: Thu, 12 Oct 2017 15:39:22 -0300 Message-Id: <1507833562-6945-1-git-send-email-adhemerval.zanella@linaro.org> As noted by Florian Weimer, current Linux posix_spawn implementation can trigger an assert if the auxiliary process is terminated before actually setting the err member: 340 /* Child must set args.err to something non-negative - we rely on 341 the parent and child sharing VM. */ 342 args.err = -1; [...] 362 new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size, 363 CLONE_VM | CLONE_VFORK | SIGCHLD, &args); 364 365 if (new_pid > 0) 366 { 367 ec = args.err; 368 assert (ec >= 0); Another possible issue is killing the child between setting the err and actually calling execve. In this case the process will not ran, but posix_spawn also will not report any error: 269 270 args->err = 0; 271 args->exec (args->file, args->argv, args->envp); As suggested by Andreas Schwab, this patch removes the faulty assert and also handles any signal that happens before fork and execve as the spawn was successful (and thus relaying the handling to the caller to figure this out). Different than Florian, I can not see why using atomics to set err would help here, essentially the code runs sequentially (due CLONE_VFORK) and I think it would not be legal the compiler evaluate ec without checking for new_pid result (thus there is no need to compiler barrier). Checked on x86_64-linux-gnu. [BZ #22273] * sysdeps/unix/sysv/linux/spawni.c (__spawnix): Handle the case where the auxiliary process is terminated by a signal before calling _exit or execve. --- ChangeLog | 6 ++++++ sysdeps/unix/sysv/linux/spawni.c | 12 ++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index dea1650..d6acc1e 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -365,9 +365,17 @@ __spawnix (pid_t * pid, const char *file, if (new_pid > 0) { ec = args.err; - assert (ec >= 0); if (ec != 0) - __waitpid (new_pid, NULL, 0); + { + /* It handles the unlikely case where the auxiliary process is + terminated by a signal before calling _exit or execve. For + this case the signal is relayed to the called, as the spawn + was successful. */ + int status; + __waitpid (new_pid, &status, WNOHANG); + if (WIFSIGNALED (status)) + ec = 0; + } } else ec = -new_pid;