From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id B3092386F446 for ; Tue, 9 Mar 2021 10:45:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B3092386F446 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-337-2mHLGiW7NTKRihaCV9FXyA-1; Tue, 09 Mar 2021 05:45:38 -0500 X-MC-Unique: 2mHLGiW7NTKRihaCV9FXyA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5204557; Tue, 9 Mar 2021 10:45:37 +0000 (UTC) Received: from oldenburg.str.redhat.com (ovpn-112-77.ams2.redhat.com [10.36.112.77]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3C76060C04; Tue, 9 Mar 2021 10:45:35 +0000 (UTC) From: Florian Weimer To: Adhemerval Zanella via Libc-alpha Subject: Re: [PATCH v3 4/4] posix: Add posix_spawn_file_actions_closefrom References: <20201223163651.2634504-1-adhemerval.zanella@linaro.org> <20201223163651.2634504-4-adhemerval.zanella@linaro.org> Date: Tue, 09 Mar 2021 11:45:42 +0100 In-Reply-To: <20201223163651.2634504-4-adhemerval.zanella@linaro.org> (Adhemerval Zanella via Libc-alpha's message of "Wed, 23 Dec 2020 13:36:51 -0300") Message-ID: <87y2ew4nc9.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Mar 2021 10:45:42 -0000 * Adhemerval Zanella via Libc-alpha: > This patch adds a way to close a range of file descriptors on > posix_spawn as a new file action. The API is similar to the one > provided by Solaris 11 [1], where the file action causes the all open > file descriptors greater than or equal to input on to be closed when > the new process is spawned. Commit subject should say posix_spawn_file_actions_closefrom_np. > The function The function posix_spawn_file_actions_closefrom_np is safe > to be implemented by interacting over /proc/self/fd, since the Linux > spawni.c helper process does not use CLONE_FILES, so its has own file > descriptor table and any failure (in /proc operation) aborts the process > creation and returns an error to the caller. Duplicate =E2=80=9CThe function=E2=80=9D, =E2=80=9Diterating=E2=80=9D inste= ad of =E2=80=9Cinteracting=E2=80=9D. > > diff --git a/NEWS b/NEWS > index e6446f0548..60a13edcbe 100644 > --- a/NEWS > +++ b/NEWS > @@ -34,6 +34,11 @@ Major new features: > greater than given integer. This function is a GNU extension, althoug= h it > also present in other systems. > =20 > +* The posix_spawn_file_actions_closefrom_np has been added, enabling > + posix_spawn and posix_spawnp to close all file descriptors greater tha= n > + a giver integer. This function is a GNU extension, although Solaris a= lso > + provides a similar function. Missing =E2=80=9Cfunction=E2=80=9D. =E2=80=9Cgreat than or equal to=E2=80= =9D. > diff --git a/posix/Versions b/posix/Versions > index 7d06a6d0c0..95d7b01126 100644 > --- a/posix/Versions > +++ b/posix/Versions > @@ -147,6 +147,9 @@ libc { > } > GLIBC_2.30 { > } > + GLIBC_2.33 { > + posix_spawn_file_actions_addclosefrom_np; > + } Needs to be GLIBC_2.34 now. Copyright years need adjusting as well. > diff --git a/posix/spawn.h b/posix/spawn.h > index be6bd591a3..e81b4a4e90 100644 > --- a/posix/spawn.h > +++ b/posix/spawn.h > @@ -213,6 +213,14 @@ extern int posix_spawn_file_actions_addchdir_np (pos= ix_spawn_file_actions_t * > extern int posix_spawn_file_actions_addfchdir_np (posix_spawn_file_actio= ns_t *, > =09=09=09=09=09=09 int __fd) > __THROW __nonnull ((1)); > + > +/* Add an action to close all file descriptor greater than FROM during > + spawn. This affects the subsequent file actions. */ > +extern int > +posix_spawn_file_actions_addclosefrom_np (posix_spawn_file_actions_t *, > +=09=09=09=09=09 int __from) > + __THROW __nonnull ((1)); > + > #endif Likewise, =E2=80=9Cgreater than or equal to=E2=80=9D. > diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c > new file mode 100644 > index 0000000000..a20e9c9ac7 > --- /dev/null > +++ b/posix/tst-spawn5.c > @@ -0,0 +1,283 @@ > +/* Nonzero if the program gets called via `exec'. */ > +static int restart; =E2=80=9Cwas called=E2=80=9D? > +/* Hold the four initial argument used to respawn the process. */ > +static char *initial_argv[7]; four !=3D 7? > +/* Called on process re-execution. The arguments are the expected opene= d > + file descriptors. */ > +_Noreturn static void > +handle_restart (int argc, char *argv[]) > +{ > + char *endptr; > + long int fd =3D strtol (e->d_name, &endptr, 10); > + if (*endptr !=3D '\0' || fd < 0 || fd > INT_MAX) > + FAIL_EXIT1 ("readdir: invalid file descriptor name: /proc/self/f= d/%s", > + e->d_name); Maybe use strtoul here? > +static void > +do_test_closefrom (void) > +{ > + /* Close a range and add some file actions. */ > + { > + posix_spawn_file_actions_t fa; > + TEST_COMPARE (posix_spawn_file_actions_init (&fa), 0); > + > + TEST_COMPARE (posix_spawn_file_actions_addclosefrom_np (&fa, lowfd += 1), 0); > + TEST_COMPARE (posix_spawn_file_actions_addopen (&fa, lowfd, "/dev/nu= ll", > +=09=09=09=09=09=09 0666, O_RDONLY), 0); > + TEST_COMPARE (posix_spawn_file_actions_adddup2 (&fa, lowfd, lowfd + = 1), 0); > + TEST_COMPARE (posix_spawn_file_actions_addopen (&fa, lowfd, "/dev/nu= ll", > +=09=09=09=09=09=09 0666, O_RDONLY), 0); > + > + spawn_closefrom_test (&fa, lowfd, lowfd, (int[]){lowfd, lowfd + 1}, = 2); I believe the closefrom should start add lowfd here, to check that the closing is ordered with regards to the other operations. > diff --git a/sysdeps/generic/spawn_int_abi.h b/sysdeps/generic/spawn_int_= abi.h > new file mode 100644 > index 0000000000..bfc8961598 > --- /dev/null > +++ b/sysdeps/generic/spawn_int_abi.h > @@ -0,0 +1,24 @@ > +/* Internal ABI specific for posix_spawn functionality. Generic version= . > +#ifndef _SPAWN_INT_ABI_H > +#define _SPAWN_INT_ABI_H > + > +#define __SPAWN_SUPPORT_CLOSEFROM 0 > + > +#endif /* _SPAWN_INT_H */ Final comment is wrong. The header is misnamed because the macro does not actually impact ABI. > diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/s= pawni.c > index f157bfffd2..ea87d8b5e6 100644 > --- a/sysdeps/unix/sysv/linux/spawni.c > +++ b/sysdeps/unix/sysv/linux/spawni.c > /* The Linux implementation of posix_spawn{p} uses the clone syscall dir= ectly > with CLONE_VM and CLONE_VFORK flags and an allocated stack. The new = stack > @@ -280,6 +274,14 @@ __spawni_child (void *arguments) > =09 if (__fchdir (action->action.fchdir_action.fd) !=3D 0) > =09=09goto fail; > =09 break; > + > +=09 case spawn_do_closefrom: > +=09 { > +=09=09int lowfd =3D action->action.closefrom_action.from; > +=09 int r =3D INLINE_SYSCALL_CALL (close_range, lowfd, ~0U, 0); > +=09=09if (r !=3D 0 && !__closefrom_fallback (lowfd)) > +=09=09 goto fail; > +=09 } break; If you implement the agressive up-to-INT_MAX closing in __closefrom_fallback, this probably needs a flag that it disables it when called from here. Thanks, Florian