Skip to content

Commit

Permalink
Optimize and unite setting CLOEXEC on fds
Browse files Browse the repository at this point in the history
In case maximum number of open files limit is set too high, both
luaext/Pexec() and lib/doScriptExec() spend way too much time
trying to set FD_CLOEXEC flag for all those file descriptors,
resulting in severe increase of time it takes to execute rpm or dnf.

This becomes increasingly noticeable when running with e.g. under
Docker because:

> $ docker run fedora ulimit -n
> 1048576

One obvious fix is to use procfs to get the actual list of opened fds
and iterate over it. My quick-n-dirty benchmark shows the /proc approach
is about 10x faster than iterating through a list of 1024 fds.

Note that the old method is still used in case /proc is not available.

While at it, unite the two implementations, and future* proof it by
making sure we modify the existing flags.

* - currently the only known flag is FD_CLOEXEC.

This should fix:
- moby/moby#23137
- https://bugzilla.redhat.com/show_bug.cgi?id=1537564

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
  • Loading branch information
kolyshkin committed May 17, 2018
1 parent a104c9e commit fee7297
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 28 deletions.
17 changes: 1 addition & 16 deletions lib/rpmscript.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#include <sys/types.h>
#include <sys/wait.h>
#include <errno.h>
#include <unistd.h>

#include <rpm/rpmfileutil.h>
#include <rpm/rpmmacro.h>
Expand Down Expand Up @@ -170,26 +169,12 @@ static const char * const SCRIPT_PATH = "PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr
static void doScriptExec(ARGV_const_t argv, ARGV_const_t prefixes,
FD_t scriptFd, FD_t out)
{
int flag;
int fdno;
int xx;
int open_max;

/* SIGPIPE is ignored in rpm, reset to default for the scriptlet */
(void) signal(SIGPIPE, SIG_DFL);

/* XXX Force FD_CLOEXEC on all inherited fdno's. */
open_max = sysconf(_SC_OPEN_MAX);
if (open_max == -1) {
open_max = 1024;
}
for (fdno = 3; fdno < open_max; fdno++) {
flag = fcntl(fdno, F_GETFD);
if (flag == -1 || (flag & FD_CLOEXEC))
continue;
xx = fcntl(fdno, F_SETFD, FD_CLOEXEC);
/* XXX W2DO? debug msg for inheirited fdno w/o FD_CLOEXEC */
}
rpmSetCloseOnExec();

if (scriptFd != NULL) {
int sfdno = Fileno(scriptFd);
Expand Down
12 changes: 1 addition & 11 deletions luaext/lposix.c
Original file line number Diff line number Diff line change
Expand Up @@ -335,21 +335,11 @@ static int Pexec(lua_State *L) /** exec(path,[args]) */
const char *path = luaL_checkstring(L, 1);
int i,n=lua_gettop(L);
char **argv;
int flag, fdno, open_max;

if (!have_forked)
return luaL_error(L, "exec not permitted in this context");

open_max = sysconf(_SC_OPEN_MAX);
if (open_max == -1) {
open_max = 1024;
}
for (fdno = 3; fdno < open_max; fdno++) {
flag = fcntl(fdno, F_GETFD);
if (flag == -1 || (flag & FD_CLOEXEC))
continue;
fcntl(fdno, F_SETFD, FD_CLOEXEC);
}
rpmSetCloseOnExec();

argv = malloc((n+1)*sizeof(char*));
if (argv==NULL) return luaL_error(L,"not enough memory");
Expand Down
2 changes: 1 addition & 1 deletion rpmio/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ AM_CPPFLAGS += -DLOCALSTATEDIR="\"$(localstatedir)\""
usrlibdir = $(libdir)
usrlib_LTLIBRARIES = librpmio.la
librpmio_la_SOURCES = \
argv.c base64.c digest.h digest.c macro.c \
argv.c base64.c cloexec.c digest.h digest.c macro.c \
rpmhook.c rpmio.c rpmlog.c rpmmalloc.c \
rpmpgp.c rpmsq.c rpmsw.c url.c \
rpmio_internal.h rpmhook.h \
Expand Down
51 changes: 51 additions & 0 deletions rpmio/cloexec.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/types.h>
#include <dirent.h>
#include "rpmutil.h"

static void set_cloexec(int fd)
{
int flags = fcntl(fd, F_GETFD);

if (flags == -1 || (flags & FD_CLOEXEC))
return;

fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
}

void rpmSetCloseOnExec(void)
{
const int min_fd = STDERR_FILENO; // don't touch stdin/out/err
int fd, open_max;

// iterate over fds obtained from /proc
DIR *dir = opendir("/proc/self/fd");
if (dir == NULL) {
// no /proc mounted
goto fallback;
}

struct dirent *entry;
while ((entry = readdir(dir)) != NULL) {
fd = atoi(entry->d_name);
if (fd > min_fd)
set_cloexec(fd);
}
closedir(dir);

return;

fallback:
// iterate over all possible fds
open_max = sysconf(_SC_OPEN_MAX);
if (open_max == -1) {
open_max = 1024;
}

for (fd = min_fd + 1; fd < open_max; fd++)
set_cloexec(fd);
}


6 changes: 6 additions & 0 deletions rpmio/rpmutil.h
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@ typedef void * (*rpmMemFailFunc) (size_t size, void *data);
*/
rpmMemFailFunc rpmSetMemFail(rpmMemFailFunc func, void *data);

/** \ingroup rpmutil
* Set close-on-exec flag for all opened file descriptors, except
* stdin/stdout/stderr.
*/
void rpmSetCloseOnExec(void);

#ifdef __cplusplus
}
#endif
Expand Down

0 comments on commit fee7297

Please sign in to comment.