On Wed, 2017-04-26 at 20:03 +0800, Jason Zaman wrote:
> On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia wrote:
> > Do not use flock() for file locking, but instead use generic text
> > files
> > that keep track of the process ID (PID) of the locking process.
> >
> > Remove semanage read and transaction lock files upon releasing
> > them.
> >
> > This third version fixes a bug in the previous version and also
> > applies
> > cleanly to the latest git tree.
> >
> > Signed-off-by: Guido Trentalancia <guido@xxxxxxxxxxxxxxxx>
> > ---
> > src/Makefile | 2
> > src/semanage_store.c | 214 +++++++++++++++++++++++++++++++++++++-
> > -------------
> > 2 files changed, 160 insertions(+), 56 deletions(-)
> >
> > --- a/src/Makefile 2017-04-25 22:27:38.105555427 +0200
> > +++ b/src/Makefile 2017-04-25 22:28:58.512555098 +0200
> > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS)
> > $(RANLIB) $@
> >
> > $(LIBSO): $(LOBJS)
> > - $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol
> > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > script=libsemanage.map,-z,defs
> > + $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol
> > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > script=libsemanage.map,-z,defs
> > ln -sf $@ $(TARGET)
> >
> > $(LIBPC): $(LIBPC).in ../VERSION
> > --- a/src/semanage_store.c 2017-04-20 16:30:21.218209972
> > +0200
> > +++ b/src/semanage_store.c 2017-04-25 22:24:35.883556172
> > +0200
> > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t;
> > #include <dirent.h>
> > #include <errno.h>
> > #include <fcntl.h>
> > +#include <math.h>
> > +#include <signal.h>
> > #include <stdio.h>
> > #include <stdio_ext.h>
> > #include <stdlib.h>
> > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t;
> > #include <unistd.h>
> > #include <sys/file.h>
> > #include <sys/stat.h>
> > +#include <sys/sysctl.h>
> > #include <sys/types.h>
> > #include <sys/wait.h>
> > #include <limits.h>
> > #include <libgen.h>
> >
> > +#include <linux/sysctl.h>
> > +
> > +#ifndef CONFIG_BASE_SMALL
> > +#define CONFIG_BASE_SMALL 0
> > +#endif
> > +
> > +#include <linux/threads.h>
> > +
> > +#ifndef PID_MAX_DEFAULT
> > +#define PID_MAX_DEFAULT 32768
> > +#endif
> > +
> > #include "debug.h"
> > #include "utilities.h"
> >
> > @@ -76,6 +91,8 @@ enum semanage_file_defs {
> > static char
> > *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS];
> > static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL };
> > static int semanage_paths_initialized = 0;
> > +static int pid_max;
> > +static ssize_t pid_max_length;
> >
> > /* These are paths relative to the bottom of the module store */
> > static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = {
> > @@ -427,8 +442,23 @@ cleanup:
> > int semanage_check_init(semanage_handle_t *sh, const char *prefix)
> > {
> > int rc;
> > + int fd;
> > + char root[PATH_MAX];
> > + ssize_t amount_read;
> > +
> > if (semanage_paths_initialized == 0) {
> > - char root[PATH_MAX];
> > + pid_max = PID_MAX_DEFAULT;
> > + pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1));
> > +
> > + fd = open("/proc/sys/kernel/pid_max", O_RDONLY);
> > + if (fd > 0) {
> > + char sysctlstring[pid_max_length];
> > + amount_read = read(fd, sysctlstring,
> > pid_max_length);
> > + if (amount_read > 0) {
> > + pid_max = atoi(sysctlstring);
> > + pid_max_length =
> > ceil(log10(pid_max + 1));
> > + }
> > + }
> >
> > rc = snprintf(root,
> > sizeof(root),
> > @@ -528,16 +558,23 @@ char *semanage_conf_path(void)
> >
> > /**************** functions that create module store
> > ***************/
> >
> > -/* Check that the semanage store exists. If 'create' is non-zero
> > then
> > - * create the directories. Returns 0 if module store exists
> > (either
> > - * already or just created), -1 if does not exist or could not be
> > - * read, or -2 if it could not create the store. */
> > +/* Check that the semanage store exists and that the read lock can
> > be
> > + * taken. If 'create' is non-zero then it creates the directories
> > + * and the lock file. Returns 0 if the module store exists
> > (either
> > + * already or just created) and the read lock can be taken, -1 if
> > it
> > + * does not exist or it is not possible to read from it, or -2 if
> > it
> > + * could not create the store or it could not take the lock file.
> > */
> > int semanage_create_store(semanage_handle_t * sh, int create)
> > {
> > struct stat sb;
> > int mode_mask = R_OK | W_OK | X_OK;
> > const char *path = semanage_files[SEMANAGE_ROOT];
> > int fd;
> > + pid_t pid, lock_pid;
> > + char *pid_string, *lock_pid_string;
> > + size_t pid_length;
> > + ssize_t pid_bytes;
> > + int invalid_lock = 0;
> >
> > if (stat(path, &sb) == -1) {
> > if (errno == ENOENT && create) {
> > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl
> > return -1;
> > }
> > }
> > + pid = getpid();
> > + pid_string = malloc(pid_max_length * sizeof(char));
> > + sprintf(pid_string, "%d", pid);
> > + pid_length = strlen(pid_string);
> > path = semanage_files[SEMANAGE_READ_LOCK];
> > if (stat(path, &sb) == -1) {
> > if (errno == ENOENT && create) {
> > if ((fd = creat(path, S_IRUSR | S_IWUSR))
> > == -1) {
> > ERR(sh, "Could not create lock
> > file at %s.",
> > path);
> > + free(pid_string);
> > return -2;
> > }
> > + pid_bytes = write(fd, pid_string,
> > pid_length);
>
> I'm pretty sure there is a race here. and another one between stat()
> and
> creat(). you can have two processes create and truncate the file then
> one write on top of the other. leaving lock files around is
> definitely
> the safest option. and are 0 bytes so its not like you're wasting any
> disk space.
>
> Overall I disagree with changing this just because a random file is
> left
> around. NFS may be a legit reason to change but as russel said, I
> think
> locks work somewhat over NFS too. Also, NFS mounting /home or /usr is
> common but /var is pretty commonly not NFS mounted.
I agree; absent a demonstration of an actual problem with the current
libsemanage locking scheme, let's leave it alone. Keeping around two
empty files is not a problem and this is a fairly well-established
paradigm for locking on Linux. flock(2) works over NFS since Linux
2.6.12.
>
> -- Jason
>
> > close(fd);
> > + free(pid_string);
> > + if (pid_bytes == (ssize_t) pid_length)
> > + return 0;
> > + else {
> > + ERR(sh, "Could not create lock
> > file at %s.", path);
> > + return -2;
> > + }
> > } else {
> > ERR(sh, "Could not read lock file at %s.",
> > path);
> > + free(pid_string);
> > return -1;
> > }
> > } else {
> > if (!S_ISREG(sb.st_mode) || access(path, R_OK |
> > W_OK) == -1) {
> > ERR(sh, "Could not access lock file at
> > %s.", path);
> > + free(pid_string);
> > return -1;
> > - }
> > + } else {
> > + fd = open(path, O_RDWR);
> > + if (fd > 0) {
> > + lock_pid_string =
> > malloc(pid_max_length * sizeof(char));
> > + pid_bytes = read(fd,
> > lock_pid_string, pid_max_length);
> > + if (pid_bytes > 0) {
> > + lock_pid = (pid_t)
> > atoi(lock_pid_string);
> > + if (lock_pid > pid_max)
> > + invalid_lock = 1;
> > + else if (kill(lock_pid, 0)
> > != 0)
> > + if (errno ==
> > ESRCH)
> > + invalid_lo
> > ck = 1;
> > +
> > + if (!invalid_lock) {
> > + ERR(sh, "Could not
> > create lock file at %s.", path);
> > + close(fd);
> > + free(pid_string);
> > + free(lock_pid_stri
> > ng);
> > + return -2;
> > + }
> > + } else
> > + invalid_lock = 1;
> > +
> > + if (invalid_lock) {
> > + pid_bytes = pwrite(fd,
> > pid_string, pid_length, 0);
> > + close(fd);
> > + free(pid_string);
> > + free(lock_pid_string);
> > + if (pid_bytes == (ssize_t)
> > pid_length)
> > + return 0;
> > + else {
> > + ERR(sh, "Could not
> > create lock file at %s.", path);
> > + return -2;
> > + }
> > + } else {
> > + ERR(sh, "Could not create
> > lock file at %s.", path);
> > + close(fd);
> > + free(pid_string);
> > + free(lock_pid_string);
> > + return -2;
> > + }
> > + }
> > + }
> > }
> > return 0;
> > }
> > @@ -1795,64 +1884,74 @@ int semanage_install_sandbox(semanage_ha
> > static int semanage_get_lock(semanage_handle_t * sh,
> > const char *lock_name, const char
> > *lock_file)
> > {
> > + pid_t pid, lock_pid;
> > + char *pid_string, *lock_pid_string;
> > int fd;
> > - struct timeval origtime, curtime;
> > + size_t pid_length;
> > + ssize_t pid_bytes;
> > int got_lock = 0;
> > + int overwritten_lock = 0;
> >
> > - if ((fd = open(lock_file, O_RDONLY)) == -1) {
> > - if ((fd =
> > - open(lock_file, O_RDWR | O_CREAT | O_TRUNC,
> > - S_IRUSR | S_IWUSR)) == -1) {
> > + pid = getpid();
> > + pid_string = malloc(pid_max_length * sizeof(char));
> > + sprintf(pid_string, "%d", pid);
> > + pid_length = strlen(pid_string);
> > + if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR |
> > S_IWUSR)) == -1) {
> > ERR(sh, "Could not open direct %s at %s.",
> > lock_name,
> > lock_file);
> > + free(pid_string);
> > return -1;
> > + } else {
> > + lock_pid_string = malloc(pid_max_length *
> > sizeof(char));
> > + pid_bytes = pread(fd, lock_pid_string,
> > pid_max_length, 0);
> > + if (pid_bytes == 0) {
> > + overwritten_lock = 1;
> > + pid_bytes = write(fd, pid_string,
> > pid_length);
> > + if (pid_bytes == (ssize_t) pid_length)
> > + got_lock = 1;
> > + } else {
> > + lock_pid = (pid_t) atoi(lock_pid_string);
> > + if (lock_pid > pid_max) {
> > + overwritten_lock = 1;
> > + pid_bytes = pwrite(fd, pid_string,
> > pid_length, 0);
> > + if (pid_bytes == (ssize_t)
> > pid_length)
> > + got_lock = 1;
> > + } else {
> > + if (lock_pid == pid)
> > + got_lock = 1;
> > + else if (kill(lock_pid, 0) != 0)
> > + if (errno == ESRCH) {
> > + overwritten_lock =
> > 1;
> > + pid_bytes =
> > pwrite(fd, pid_string, pid_length, 0);
> > + if (pid_bytes ==
> > (ssize_t) pid_length)
> > + got_lock =
> > 1;
> > + }
> > + }
> > }
> > }
> > - if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
> > - ERR(sh, "Could not set close-on-exec for %s at
> > %s.", lock_name,
> > - lock_file);
> > +
> > + if (!got_lock) {
> > close(fd);
> > + free(pid_string);
> > + free(lock_pid_string);
> > + if (overwritten_lock)
> > + unlink(lock_file);
> > + ERR(sh, "Could not get direct %s at %s.",
> > lock_name, lock_file);
> > return -1;
> > }
> >
> > - if (sh->timeout == 0) {
> > - /* return immediately */
> > - origtime.tv_sec = 0;
> > - } else {
> > - origtime.tv_sec = sh->timeout;
> > - }
> > - origtime.tv_usec = 0;
> > - do {
> > - curtime.tv_sec = 1;
> > - curtime.tv_usec = 0;
> > - if (flock(fd, LOCK_EX | LOCK_NB) == 0) {
> > - got_lock = 1;
> > - break;
> > - } else if (errno != EAGAIN) {
> > - ERR(sh, "Error obtaining direct %s at
> > %s.", lock_name,
> > - lock_file);
> > - close(fd);
> > - return -1;
> > - }
> > - if (origtime.tv_sec > 0 || sh->timeout == -1) {
> > - if (select(0, NULL, NULL, NULL, &curtime)
> > == -1) {
> > - if (errno == EINTR) {
> > - continue;
> > - }
> > - ERR(sh,
> > - "Error while waiting to get
> > direct %s at %s.",
> > - lock_name, lock_file);
> > - close(fd);
> > - return -1;
> > - }
> > - origtime.tv_sec--;
> > - }
> > - } while (origtime.tv_sec > 0 || sh->timeout == -1);
> > - if (!got_lock) {
> > - ERR(sh, "Could not get direct %s at %s.",
> > lock_name, lock_file);
> > + if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
> > + ERR(sh, "Could not set close-on-exec for %s at
> > %s.", lock_name,
> > + lock_file);
> > close(fd);
> > + free(pid_string);
> > + free(lock_pid_string);
> > + unlink(lock_file);
> > return -1;
> > }
> > +
> > + free(pid_string);
> > + free(lock_pid_string);
> > return fd;
> > }
> >
> > @@ -1901,29 +2000,27 @@ int semanage_get_active_lock(semanage_ha
> > }
> > }
> >
> > -/* Releases the transaction lock. Does nothing if there was not
> > one already
> > - * there. */
> > +/* Releases the transaction lock: the lock file is removed */
> > void semanage_release_trans_lock(semanage_handle_t * sh)
> > {
> > int errsv = errno;
> > if (sh->u.direct.translock_file_fd >= 0) {
> > - flock(sh->u.direct.translock_file_fd, LOCK_UN);
> > close(sh->u.direct.translock_file_fd);
> > sh->u.direct.translock_file_fd = -1;
> > }
> > + unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
> > errno = errsv;
> > }
> >
> > -/* Releases the read lock. Does nothing if there was not one
> > already
> > - * there. */
> > +/* Releases the read lock: the lock file is removed */
> > void semanage_release_active_lock(semanage_handle_t * sh)
> > {
> > int errsv = errno;
> > if (sh->u.direct.activelock_file_fd >= 0) {
> > - flock(sh->u.direct.activelock_file_fd, LOCK_UN);
> > close(sh->u.direct.activelock_file_fd);
> > sh->u.direct.activelock_file_fd = -1;
> > }
> > + unlink(semanage_files[SEMANAGE_READ_LOCK]);
> > errno = errsv;
> > }
> >
