From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76076 invoked by alias); 14 Sep 2016 19:37:42 -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 75265 invoked by uid 89); 14 Sep 2016 19:37:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=conforms, UD:php, austin X-HELO: mail-yb0-f170.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=Qv59L4nxobVpAaMsCZddOlQli/5W259VPGHQ/PKGlEY=; b=jHasrPWaIjnbbgRCZOdn9RdLcuIK1ik9gqki1DJYhA2udbXI4no/DD9Z5IhJguSYax I5LG/IS+O6QzfWpoCJv7chdCJcz+VQ/nXmuOwfQ3ivzDupbrHkTUAiuYJGDjT6w1JR2D CIqkjhvYYmDrOh3lUF/y7AsjnBoNHv/mVrIocADzD+C1rCxsVHedk4i7i7amxLY1Zho+ y4nsFelG403f1dGBxzCNyZNWqigulsrZJF2TLZdsjGTAMko9FQJPMofGsCKkmtxi9IP0 EULE5sV+hGIeGsHE3/hH2IJ7T2u0ft4434/wA2ihyzVctKBJVIY/R1i+wZICAvl4Kp/M cdoQ== X-Gm-Message-State: AE9vXwOj+eQCEO2oa3pFN01E8PqSTDCqtJSNFJf3HocNwcFjvHVwggg7PBfn1TXx2/AQtqr3 X-Received: by 10.37.19.132 with SMTP id 126mr4881669ybt.113.1473881849618; Wed, 14 Sep 2016 12:37:29 -0700 (PDT) Subject: Re: [PATCH v2 3/3] posix: New Linux posix_spawn{p} implementation To: Rasmus Villemoes , libc-alpha@sourceware.org References: <1454343665-1706-1-git-send-email-adhemerval.zanella@linaro.org> <1454343665-1706-4-git-send-email-adhemerval.zanella@linaro.org> <87mvjsprqb.fsf@rasmusvillemoes.dk> From: Adhemerval Zanella Message-ID: <3ec3af54-b733-a894-cb74-d83a947b20d8@linaro.org> Date: Wed, 14 Sep 2016 19:37:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <87mvjsprqb.fsf@rasmusvillemoes.dk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-09/txt/msg00252.txt.bz2 On 31/08/2016 18:13, Rasmus Villemoes wrote: >> + 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 I haven't joined the original discussion also (if any) for old implementation behaviour, but I think this conforms to austin group issue #370 [1] where it changed: fails for any of the reasons that would cause close( ), dup2( ), or open( ) to fail, an error value shall be returned to: fails for any of the reasons that would cause close( ), dup2( ), or open( ) to fail, other than attempting a close( ) on a file descriptor that is in range but already closed, an error value shall be returned The documentation at [2] seems to not have this updates issues description. [1] http://austingroupbugs.net/view.php?id=370 [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn.html > >> + 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 I think this should be an issue iff the program tries call posix_spawn with a total number of file descriptors equal to RLIMIT_NOFILE. > > * 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. Right, this should more problematic to handle. I think an option is to add a 'fcntl(fd, F_GETFD) != -1 || errno != EBADF' check before the open syscall and close the file descriptor in this case.