From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68656 invoked by alias); 31 Aug 2016 21:13:14 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 68643 invoked by uid 89); 31 Aug 2016 21:13:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=H*F:D*dk, fcntl, cnt, pov X-HELO: mail-wm0-f50.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:organization:references:date :in-reply-to:message-id:user-agent:mime-version; bh=+bXySh1pgyNR2BB6AIYXV+f0L/EEdF7u8G/nKJcIg7w=; b=kqn5Smi3BiIHoQRpVK6bnepK0if81vpp6OCEe+avsT3i0QCj2CReKwP8k4td/2Y/OW CErY/UIbRVany8BXStQsp4KneLyGLTgSM/5FFQ34pn9oCvPu57kZ4QNlAH0W1miZ3n03 4GLKLDzvpYCMmfeXVrBhjR/V539XtUaFvV56ZY1Q827CqKSrRKXZBNWCoLPLiMEThMOl BWs7/UP8oE8fJx3UkgXE/VB0R83SMQuXdwFmPpSDBOMezXYgoVlDhwF9FK5j3L3nFwig /s2zpe+o4RgTHXxkzeydrxsNYI2w/oNXp48VLiJjqRefsVIUDmiXUKK6fKWwy2ygM8mJ B4MA== X-Gm-Message-State: AE9vXwOo7ISqGwBDKpQCHvH6aW7WyxlGRYkBpbK7c3xOfkFMV/CroP/K2EKHN81Nr26CBg== X-Received: by 10.194.44.229 with SMTP id h5mr10597484wjm.191.1472677981857; Wed, 31 Aug 2016 14:13:01 -0700 (PDT) X-Hashcash: 1:20:160831:libc-alpha@sourceware.org::ILkGnelTApdBjkvB:00000000000000000000000000000000000037fg X-Hashcash: 1:20:160831:adhemerval.zanella@linaro.org::oF2mlAaiJeuZubKS:000000000000000000000000000000003Hdq From: Rasmus Villemoes To: libc-alpha@sourceware.org Cc: Adhemerval Zanella Subject: Re: [PATCH v2 3/3] posix: New Linux posix_spawn{p} implementation References: <1454343665-1706-1-git-send-email-adhemerval.zanella@linaro.org> <1454343665-1706-4-git-send-email-adhemerval.zanella@linaro.org> Date: Wed, 31 Aug 2016 21:13:00 -0000 In-Reply-To: <1454343665-1706-4-git-send-email-adhemerval.zanella@linaro.org> (Adhemerval Zanella's message of "Mon, 1 Feb 2016 14:21:05 -0200") Message-ID: <87mvjsprqb.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2016-08/txt/msg00935.txt.bz2 On Mon, Feb 01 2016, Adhemerval Zanella wrote: > + for (cnt = 0; cnt < file_actions->__used; ++cnt) > + { > + struct __spawn_action *action = &file_actions->__actions[cnt]; > + > + /* Dup the pipe fd onto an unoccupied one to avoid any file > + operation to clobber it. */ > + if ((action->action.close_action.fd == p) > + || (action->action.open_action.fd == p) > + || (action->action.dup2_action.fd == p)) > + { > + if ((ret = __dup (p)) < 0) > + goto fail; > + p = ret; > + } > + Rather late to the party, but I think there's a few bugs here. Most importantly, dup() doesn't preserve the CLOEXEC flag, so if we do move the write end around like this, the fd will not automatically be closed during the exec, and hence the parent won't receive EOF and will block in read() call until the child finally exits. That's easily fixable with fcntl(p, F_DUPFD_CLOEXEC, 0). Pretty annoying to add a test for, though. Then there's the condition. First of all I think it's a little ugly reading the fd member from the "irrelevant" union members, even if gcc probably optimizes this to a single comparison. But more importantly, for the purpose of preventing clobbering our back channel to our parent, in the case of dup2_action the target aka newfd is much more relevant. That doesn't mean we don't also have to do some checking of the source of a dup2 action: If we allow dup2'ing it, we'd end up with the same problem as above; some copy of the write end of the pipe not being closed by exec(). Arguably, from the application's POV, p is not an open file descriptor, so we could fail the action (and hence the entire spawn) with EBADF. In fact, I can't really see what else we could do. For the close action, we can get by with just dupping the fd away and letting the action close the original (effectively making the action a no-op), but I also think that's an application bug. So I think the pipe clobber checking should be done in each action individually, since there's different logic that needs to be applied in each case. > + switch (action->tag) > + { > + case spawn_do_close: > + if ((ret = > + close_not_cancel (action->action.close_action.fd)) != 0) > + { > + if (!have_fdlimit) > + { > + __getrlimit64 (RLIMIT_NOFILE, &fdlimit); > + have_fdlimit = true; > + } > + > + /* Signal errors only for file descriptors out of range. */ > + if (action->action.close_action.fd < 0 > + || action->action.close_action.fd >= fdlimit.rlim_cur) > + goto fail; > + } I may have missed it in the original discussion, but what is the rationale for this? POSIX says If the file_actions argument is not NULL, and specifies any close, dup2, or open actions to be performed, and if posix_spawn() or posix_spawnp() fails for any of the reasons that would cause close(), dup2(), or open() to fail, an error value shall be returned as described by close(), dup2(), and open(), respectively > + break; > + > + case spawn_do_open: > + { > + ret = open_not_cancel (action->action.open_action.path, > + action->action. > + open_action.oflag | O_LARGEFILE, > + action->action.open_action.mode); > + > + if (ret == -1) > + goto fail; > + > + int new_fd = ret; > + > + /* Make sure the desired file descriptor is used. */ > + if (ret != action->action.open_action.fd) > + { > + if ((ret = __dup2 (new_fd, action->action.open_action.fd)) > + != action->action.open_action.fd) > + goto fail; > + > + if ((ret = close_not_cancel (new_fd)) != 0) > + goto fail; > + } > + } This is also how I'd have implemented it, but POSIX explicitly says If fildes was already an open file descriptor, it shall be closed before the new file is opened. Some, if slightly pathological, examples of how the difference could be observed: * ENFILE/EMFILE * Some single-open special device; following POSIX, 'action_open("/dev/watchdog", 47); action_open("/dev/watchdog", 47);' should both succeed if the first does, whereas the second will fail with the current code. > + break; > + > + case spawn_do_dup2: > + if ((ret = __dup2 (action->action.dup2_action.fd, > + action->action.dup2_action.newfd)) > + != action->action.dup2_action.newfd) > + goto fail; > + break; > + } > + } > + } > + > + /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK > + is set, otherwise restore the previous one. */ > + __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK) > + ? &attr->__ss : &args->oldmask, 0); > + > + args->exec (args->file, args->argv, args->envp); > + > + /* This is compatibility function required to enable posix_spawn run > + script without shebang definition for older posix_spawn versions > + (2.15). */ > + maybe_script_execute (args); > + > + ret = -errno; > + > +fail: > + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ > + ret = -ret; > + if (ret) > + while (write_not_cancel (p, &ret, sizeof ret) < 0) > + continue; This also seems wrong, at least if the intention is to write the proper errno value back. That only happens if we reach fail: after failing to exec - in most or all other cases, ret has the value -1, so we'd be writing EPERM to our parent. So why not just pull the value from errno in all cases? Rasmus