From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72c.google.com (mail-qk1-x72c.google.com [IPv6:2607:f8b0:4864:20::72c]) by sourceware.org (Postfix) with ESMTPS id 371B23861001 for ; Tue, 9 Mar 2021 19:07:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 371B23861001 Received: by mail-qk1-x72c.google.com with SMTP id 130so14178149qkh.11 for ; Tue, 09 Mar 2021 11:07:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:references:from:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=7U7pHcIPZPvVEzXXDX4wu2Qr4D7h15ihRXm+/R9AZ+E=; b=Wyz4mdhUKXufYEJTRzBGKUH23j7MsRxSvFd/J+ob9KAohH6RvsKKEAqKgXF0Y/SpVd Fskk40ziR6aAuj0Am3Lk5H5UFIF1Zk4xmhX6emDsO2Dvm2BfOjrojOlOqISIFaM8yOxC kfmHD+Dv9qOsTuV3qK01s75GtF2uLarUy+TKFMA+/UJoMugA8aMTS/kpJswCziv+VVBe HpCLIbdK3pODoMVxKbpW6NhkJi1KMqR4C5gOLdQE5M1vvu41CaJdqZZXWOrwOoi1yjxM NOXdA9skmYSMVZGFG+bhyfcnN2vh1R384xGmvUDjjQh91uRa4Qwx6GTMdasCtjMEfEDY qWFA== X-Gm-Message-State: AOAM533QRwtL6Ww/HAek3LOsFwD/5/mpRpIAGH21uXeFVSyRAjW9juy3 NT+uU+LVgvQqbSbZCIxuZlo+eZZZWTqkMw== X-Google-Smtp-Source: ABdhPJzNz9c6kQIvIp+FckTA9Z4LKZgxwsFrS6RJInTGTEoxZ+1+c0GuCVS+8Ez43ntK0f4pL3d6Kw== X-Received: by 2002:ae9:eb57:: with SMTP id b84mr26856286qkg.271.1615316876439; Tue, 09 Mar 2021 11:07:56 -0800 (PST) Received: from [192.168.1.4] ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id d70sm11061960qkg.30.2021.03.09.11.07.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 09 Mar 2021 11:07:56 -0800 (PST) To: Florian Weimer , Adhemerval Zanella via Libc-alpha References: <20201223163651.2634504-1-adhemerval.zanella@linaro.org> <20201223163651.2634504-3-adhemerval.zanella@linaro.org> <877dmg62x0.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Subject: Re: [PATCH v3 3/4] io: Add closefrom [BZ #10353] Message-ID: <55c9201c-3bc9-4f96-3a83-210638a2936c@linaro.org> Date: Tue, 9 Mar 2021 16:07:53 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <877dmg62x0.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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 19:07:59 -0000 On 09/03/2021 07:23, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/io/Versions b/io/Versions >> index 49c4d2d40a..fa33452881 100644 >> --- a/io/Versions >> +++ b/io/Versions >> @@ -135,6 +135,7 @@ libc { >> GLIBC_2.33 { >> stat; stat64; fstat; fstat64; lstat; lstat64; fstatat; fstatat64; >> mknod; mknodat; >> + closefrom; >> } > > Needs to be GLIBC_2.34 now. Copyright years need adjusting, too. Ack. > >> diff --git a/io/closefrom.c b/io/closefrom.c >> new file mode 100644 >> index 0000000000..7833935b16 >> --- /dev/null >> +++ b/io/closefrom.c > >> +void >> +__closefrom (int lowfd) >> +{ >> + int maxfd = __getdtablesize (); >> + if (maxfd == -1) >> + __fortify_fail ("closefrom failed to get the file descriptor table size"); >> + >> + for (int i = 0; i < maxfd; i++) >> + if (i >= lowfd) >> + __close_nocancel_nostatus (i); >> +} > > Still okay for Hurd. > >> +weak_alias (__closefrom, closefrom) >> diff --git a/io/tst-closefrom.c b/io/tst-closefrom.c >> new file mode 100644 >> index 0000000000..6f6fbd270f >> --- /dev/null >> +++ b/io/tst-closefrom.c > >> +#define NFDS 100 >> + >> +static int >> +open_multiple_temp_files (void) >> +{ >> + /* Check if the temporary file descriptor has no no gaps. */ >> + int lowfd = xopen ("/dev/null", O_RDONLY, 0600); >> + for (int i = 1; i <= NFDS; i++) >> + TEST_COMPARE (xopen ("/dev/null", O_RDONLY, 0600), >> + lowfd + i); >> + return lowfd; >> +} > > Spurious line wrap. Ack. > >> + >> +static int >> +closefrom_test (void) >> +{ >> + struct support_descriptors *descrs = support_descriptors_list (); >> + >> + int lowfd = open_multiple_temp_files (); >> + >> + const int maximum_fd = lowfd + NFDS; >> + const int half_fd = maximum_fd / 2; >> + const int gap = maximum_fd / 4; > > See the other message about the half_fd initialization. Ack. > >> +/* Check if closefrom works even when no new file descriptors can be >> + created. */ >> +static int >> +closefrom_test_file_desc_limit (void) >> +{ >> + int max_fd = NFDS; >> + { >> + struct rlimit rl; >> + if (getrlimit (RLIMIT_NOFILE, &rl) == -1) >> + FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m"); >> + >> + max_fd = (rl.rlim_cur < max_fd ? rl.rlim_cur : max_fd); >> + rl.rlim_cur = max_fd; >> + >> + if (setrlimit (RLIMIT_NOFILE, &rl) == 1) >> + FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m"); >> + } >> + >> + /* Exhauste the file descriptor limit. */ >> + int lowfd = xopen ("/dev/null", O_RDONLY, 0600); >> + for (;;) >> + { >> + int fd = open ("/dev/null", O_RDONLY, 0600); >> + if (fd == -1) >> + { >> + if (errno != EMFILE) >> + FAIL_EXIT1 ("create_temp_file: %m"); > > Wrong error message. > > Maybe add TEST_VERIFY_EXIT (fd < max_fd) to the loop? I believe the > setrlimit call will ensure that. Ack. > >> diff --git a/manual/llio.texi b/manual/llio.texi >> index ceb18ac89a..777993d207 100644 >> --- a/manual/llio.texi >> +++ b/manual/llio.texi >> @@ -321,6 +321,14 @@ The maximum number of file descriptors is controlled by the >> @end table >> @end deftypefun >> >> +@deftypefun void closefrom (int @var{lowfd}) >> +@standards{GNU, unistd.h} >> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{@acsfd{}}} >> + >> +The function @code{closefrom} closes all file descriptors large or equal >> +then @var{lowfd}. This function is similar to call @code{close} in specified >> +file descriptor range. >> +@end deftypefun > > “larger than or equal to @var{lowfd}” > > “@code{close} applied to the specified file descriptor range” > > Please also mention that already-closed descriptors are ignored. Ack. > >> diff --git a/sysdeps/unix/sysv/linux/closefrom.c b/sysdeps/unix/sysv/linux/closefrom.c >> new file mode 100644 >> index 0000000000..ba98fccd39 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/closefrom.c >> +void >> +__closefrom (int lowfd) >> +{ >> + int l = MAX (0, lowfd); >> + >> + int r = __close_range (l, ~0U, 0); >> + if (r == 0) >> + return; >> + >> + if (!__closefrom_fallback (l)) >> + __fortify_fail ("closefrom failed to close a file descriptor"); >> +} > > This ignores EPERM. I guess that's okay. Ack. > >> diff --git a/sysdeps/unix/sysv/linux/closefrom_fallback.c b/sysdeps/unix/sysv/linux/closefrom_fallback.c >> new file mode 100644 >> index 0000000000..78182bc5f0 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/closefrom_fallback.c >> @@ -0,0 +1,93 @@ > >> +/* Fallback code: iterates over /proc/self/fd, closing each file descriptor >> + that fall on the criteria. */ >> +_Bool >> +__closefrom_fallback (int from) >> +{ >> + bool ret = false; >> + >> + int dirfd = __open_nocancel (FD_TO_FILENAME_PREFIX, O_RDONLY | O_DIRECTORY, >> + 0); >> + if (dirfd == -1) >> + { >> + /* The closefrom should work even when process can't open new files. >> + In this case it loops over RLIMIT_NOFILE / rlim_cur until it frees >> + a file descriptor to iterate over /proc. */ >> + if (errno == ENOENT) >> + goto err; >> + >> + int maxfd = __getdtablesize (); >> + for (int i = from; i < maxfd; i++) >> + if (__close_nocancel (i) == 0) >> + break; > > This should check for errno != EBADF. EINTR, EIO etc. are okay as well > because a descriptor has been released. Ack. > > I wouldn't mind iterating up to INT_MAX, removing the __getdtablesize > call. It may take a minute or two, but it removes some of the error > scenarios. Ok, I think it should be ok (the __getdtablesize call should be issue only when open fails). > >> + >> + dirfd = __open_nocancel (FD_TO_FILENAME_PREFIX, O_RDONLY | O_DIRECTORY, >> + 0); >> + if (dirfd == -1) >> + goto err; >> + } >> + >> + char buffer[1024]; >> + while (true) >> + { >> + ssize_t ret = __getdents64 (dirfd, buffer, sizeof (buffer)); >> + if (ret == -1) >> + goto err; >> + else if (ret == 0) >> + break; >> + >> + bool closed = false; > > Maybe add a comment here about restarting on close? Ack. > >> + char *begin = buffer, *end = buffer + ret; >> + while (begin != end) >> + { >> + unsigned short int d_reclen; >> + memcpy (&d_reclen, begin + offsetof (struct dirent64, d_reclen), >> + sizeof (d_reclen)); >> + const char *dname = begin + offsetof (struct dirent64, d_name); >> + begin += d_reclen; >> + >> + if (dname[0] == '.') >> + continue; >> + >> + int fd = 0; >> + for (const char *s = dname; (unsigned int) (*s) - '0' < 10; s++) >> + fd = 10 * fd + (*s - '0'); > > Hmm. I had to think a bit about it, but it seems okay whether *s is > signed or not.> >> + if (fd == dirfd || fd < from) >> + continue; >> + >> + __close_nocancel (fd); > > Maybe add a comment why we aren't adding an error here? Ack. > >> + closed = true; >> + } >> + >> + if (closed) >> + __lseek (dirfd, 0, SEEK_SET); >> + } > > Missing error check? Ack, lseek failure should be reported. > >> + ret = true; >> +err: >> + __close_nocancel (dirfd); >> + return ret; >> +} > > The algorithm looks fine to me. Thanks for adding the restart-on-close > part. > > Thanks, > Florian >