Support stopping containers concurrently

Trying to stop multiple containers concurrently ends up with "cgroup is not mounted" errors as multiple threads corrupts the shared variables.
Fix that stack corruption and start to use getmntent_r to support stopping multiple containers concurrently.

Signed-off-by: S.Çağlar Onur <caglar@10ur.org>
Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 368214f2440c39a7e25eff598d3d831e558f0335..b6f3e6ea112a7be3e449cad0cbec255a4d9a5096 100644 (file)
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -54,6 +54,11 @@ lxc_log_define(lxc_cgroup, lxc);
 
 #define MTAB "/proc/mounts"
 
+/* In the case of a bind mount, there could be two long pathnames in the
+ * mntent plus options so use large enough buffer size
+ */
+#define LARGE_MAXPATHLEN 4 * MAXPATHLEN
+
 /* Check if a mount is a cgroup hierarchy for any subsystem.
  * Return the first subsystem found (or NULL if none).
  */
@@ -100,29 +105,31 @@ static char *mount_has_subsystem(const struct mntent *mntent)
  */
 static int get_cgroup_mount(const char *subsystem, char *mnt)
 {
-       struct mntent *mntent;
+       struct mntent *mntent, mntent_r;
        FILE *file = NULL;
        int ret, err = -1;
 
+       char buf[LARGE_MAXPATHLEN] = {0};
+
        file = setmntent(MTAB, "r");
        if (!file) {
                SYSERROR("failed to open %s", MTAB);
                return -1;
        }
 
-       while ((mntent = getmntent(file))) {
-               if (strcmp(mntent->mnt_type, "cgroup"))
+       while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
+               if (strcmp(mntent_r.mnt_type, "cgroup") != 0)
                        continue;
-
+               
                if (subsystem) {
-                       if (!hasmntopt(mntent, subsystem))
+                       if (!hasmntopt(&mntent_r, subsystem))
                                continue;
                } else {
-                       if (!mount_has_subsystem(mntent))
+                       if (!mount_has_subsystem(&mntent_r))
                                continue;
                }
 
-               ret = snprintf(mnt, MAXPATHLEN, "%s", mntent->mnt_dir);
+               ret = snprintf(mnt, MAXPATHLEN, "%s", mntent_r.mnt_dir);
                if (ret < 0 || ret >= MAXPATHLEN)
                        goto fail;
 
@@ -148,22 +155,33 @@ out:
  *
  * Returns 0 on success, -1 on error.
  *
- * The answer is written in a static char[MAXPATHLEN] in this function and
- * should not be freed.
  */
 extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpath)
 {
-       static char        buf[MAXPATHLEN];
-       static char        retbuf[MAXPATHLEN];
        int rc;
 
+       char *buf = NULL;
+       char *retbuf = NULL;
+
+       buf = malloc(MAXPATHLEN * sizeof(char));
+       if (!buf) {
+               ERROR("malloc failed");
+               goto fail;
+       }
+
+       retbuf = malloc(MAXPATHLEN * sizeof(char));
+       if (!retbuf) {
+               ERROR("malloc failed");
+               goto fail;
+       }
+
        /* lxc_cgroup_set passes a state object for the subsystem,
         * so trim it to just the subsystem part */
        if (subsystem) {
                rc = snprintf(retbuf, MAXPATHLEN, "%s", subsystem);
                if (rc < 0 || rc >= MAXPATHLEN) {
                        ERROR("subsystem name too long");
-                       return -1;
+                       goto fail;
                }
                char *s = index(retbuf, '.');
                if (s)
@@ -172,19 +190,28 @@ extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpat
        }
        if (get_cgroup_mount(subsystem ? retbuf : NULL, buf)) {
                ERROR("cgroup is not mounted");
-               return -1;
+               goto fail;
        }
 
        rc = snprintf(retbuf, MAXPATHLEN, "%s/%s", buf, cgpath);
        if (rc < 0 || rc >= MAXPATHLEN) {
                ERROR("name too long");
-               return -1;
+               goto fail;
        }
 
        DEBUG("%s: returning %s for subsystem %s", __func__, retbuf, subsystem);
 
+       if(buf)
+               free(buf);
+
        *path = retbuf;
        return 0;
+fail:
+       if (buf)
+               free(buf);
+       if (retbuf)
+               free(retbuf);
+       return -1;
 }
 
 /*
@@ -292,20 +319,25 @@ static int do_cgroup_set(const char *path, const char *value)
 int lxc_cgroup_set_bypath(const char *cgpath, const char *filename, const char *value)
 {
        int ret;
-       char *dirpath;
+       char *dirpath = NULL;
        char path[MAXPATHLEN];
 
        ret = cgroup_path_get(&dirpath, filename, cgpath);
        if (ret)
-               return -1;
+               goto fail;
 
        ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
        if (ret < 0 || ret >= MAXPATHLEN) {
                ERROR("pathname too long");
-               return -1;
+               goto fail;
        }
 
        return do_cgroup_set(path, value);
+
+fail:
+       if(dirpath)
+               free(dirpath);
+       return -1;
 }
 
 /*
@@ -323,20 +355,25 @@ int lxc_cgroup_set(const char *name, const char *filename, const char *value,
                   const char *lxcpath)
 {
        int ret;
-       char *dirpath;
+       char *dirpath = NULL;
        char path[MAXPATHLEN];
 
        ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
        if (ret)
-               return -1;
+               goto fail;
 
        ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
        if (ret < 0 || ret >= MAXPATHLEN) {
                ERROR("pathname too long");
-               return -1;
+               goto fail;
        }
 
        return do_cgroup_set(path, value);
+
+fail:
+       if(dirpath)
+               free(dirpath);
+       return -1;
 }
 
 /*
@@ -363,24 +400,24 @@ int lxc_cgroup_get(const char *name, const char *filename, char *value,
                   size_t len, const char *lxcpath)
 {
        int fd, ret = -1;
-       char *dirpath;
+       char *dirpath = NULL;
        char path[MAXPATHLEN];
        int rc;
 
        ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
        if (ret)
-               return -1;
+               goto fail;
 
        rc = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
        if (rc < 0 || rc >= MAXPATHLEN) {
                ERROR("pathname too long");
-               return -1;
+               goto fail;
        }
 
        fd = open(path, O_RDONLY);
        if (fd < 0) {
                ERROR("open %s : %s", path, strerror(errno));
-               return -1;
+               goto fail;
        }
 
        if (!len || !value) {
@@ -400,24 +437,28 @@ int lxc_cgroup_get(const char *name, const char *filename, char *value,
 
        close(fd);
        return ret;
+fail:
+       if(dirpath)
+               free(dirpath);
+       return -1;
 }
 
 int lxc_cgroup_nrtasks(const char *cgpath)
 {
-       char *dpath;
+       char *dirpath = NULL;
        char path[MAXPATHLEN];
        int pid, ret, count = 0;
        FILE *file;
        int rc;
 
-       ret = cgroup_path_get(&dpath, NULL, cgpath);
+       ret = cgroup_path_get(&dirpath, NULL, cgpath);
        if (ret)
-               return -1;
+               goto fail;
 
-       rc = snprintf(path, MAXPATHLEN, "%s/tasks", dpath);
+       rc = snprintf(path, MAXPATHLEN, "%s/tasks", dirpath);
        if (rc < 0 || rc >= MAXPATHLEN) {
                ERROR("pathname too long");
-               return -1;
+               goto fail;
        }
 
        file = fopen(path, "r");
@@ -432,6 +473,10 @@ int lxc_cgroup_nrtasks(const char *cgpath)
        fclose(file);
 
        return count;
+fail:
+       if(dirpath)
+               free(dirpath);
+       return -1;
 }
 
 /*
@@ -472,33 +517,35 @@ static void set_clone_children(const char *mntdir)
 static int create_lxcgroups(const char *lxcgroup)
 {
        FILE *file = NULL;
-       struct mntent *mntent;
+       struct mntent *mntent, mntent_r;
        int ret, retv = -1;
        char path[MAXPATHLEN];
 
+       char buf[LARGE_MAXPATHLEN] = {0};
+
        file = setmntent(MTAB, "r");
        if (!file) {
                SYSERROR("failed to open %s", MTAB);
                return -1;
        }
 
-       while ((mntent = getmntent(file))) {
+       while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
 
-               if (strcmp(mntent->mnt_type, "cgroup"))
+               if (strcmp(mntent_r.mnt_type, "cgroup"))
                        continue;
-               if (!mount_has_subsystem(mntent))
+               if (!mount_has_subsystem(&mntent_r))
                        continue;
 
-               /* 
+               /*
                 * TODO - handle case where lxcgroup has subdirs?  (i.e. build/l1)
                 * We probably only want to support that for /users/joe
                 */
                ret = snprintf(path, MAXPATHLEN, "%s/%s",
-                              mntent->mnt_dir, lxcgroup ? lxcgroup : "lxc");
+                              mntent_r.mnt_dir, lxcgroup ? lxcgroup : "lxc");
                if (ret < 0 || ret >= MAXPATHLEN)
                        goto fail;
                if (access(path, F_OK)) {
-                       set_clone_children(mntent->mnt_dir);
+                       set_clone_children(mntent_r.mnt_dir);
                        ret = mkdir(path, 0755);
                        if (ret == -1 && errno != EEXIST) {
                                SYSERROR("failed to create '%s' directory", path);
@@ -535,7 +582,7 @@ fail:
  * freezer cgroup's full path will be /sys/fs/cgroup/freezer/lxc/r1/.
  *
  * XXX This should probably be locked globally
- * 
+ *
  * Races won't be determintal, you'll just end up with leftover unused cgroups
  */
 char *lxc_cgroup_path_create(const char *lxcgroup, const char *name)
@@ -544,7 +591,9 @@ char *lxc_cgroup_path_create(const char *lxcgroup, const char *name)
        char *retpath, path[MAXPATHLEN];
        char tail[12];
        FILE *file = NULL;
-       struct mntent *mntent;
+       struct mntent *mntent, mntent_r;
+
+       char buf[LARGE_MAXPATHLEN] = {0};
 
        if (create_lxcgroups(lxcgroup) < 0)
                return NULL;
@@ -561,15 +610,15 @@ again:
        else
                *tail = '\0';
 
-       while ((mntent = getmntent(file))) {
+       while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
 
-               if (strcmp(mntent->mnt_type, "cgroup"))
+               if (strcmp(mntent_r.mnt_type, "cgroup"))
                        continue;
-               if (!mount_has_subsystem(mntent))
+               if (!mount_has_subsystem(&mntent_r))
                        continue;
 
                /* find unused mnt_dir + lxcgroup + name + -$i */
-               ret = snprintf(path, MAXPATHLEN, "%s/%s/%s%s", mntent->mnt_dir,
+               ret = snprintf(path, MAXPATHLEN, "%s/%s/%s%s", mntent_r.mnt_dir,
                               lxcgroup ? lxcgroup : "lxc", name, tail);
                if (ret < 0 || ret >= MAXPATHLEN)
                        goto fail;
@@ -609,8 +658,9 @@ int lxc_cgroup_enter(const char *cgpath, pid_t pid)
 {
        char path[MAXPATHLEN];
        FILE *file = NULL, *fout;
-       struct mntent *mntent;
+       struct mntent *mntent, mntent_r;
        int ret, retv = -1;
+       char buf[LARGE_MAXPATHLEN] = {0};
 
        file = setmntent(MTAB, "r");
        if (!file) {
@@ -618,13 +668,13 @@ int lxc_cgroup_enter(const char *cgpath, pid_t pid)
                return -1;
        }
 
-       while ((mntent = getmntent(file))) {
-               if (strcmp(mntent->mnt_type, "cgroup"))
+       while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
+               if (strcmp(mntent_r.mnt_type, "cgroup"))
                        continue;
-               if (!mount_has_subsystem(mntent))
+               if (!mount_has_subsystem(&mntent_r))
                        continue;
                ret = snprintf(path, MAXPATHLEN, "%s/%s/tasks",
-                              mntent->mnt_dir, cgpath);
+                              mntent_r.mnt_dir, cgpath);
                if (ret < 0 || ret >= MAXPATHLEN) {
                        ERROR("entering cgroup");
                        goto out;
@@ -716,23 +766,25 @@ static int lxc_one_cgroup_destroy(struct mntent *mntent, const char *cgpath)
  */
 int lxc_cgroup_destroy(const char *cgpath)
 {
-       struct mntent *mntent;
+       struct mntent *mntent, mntent_r;
        FILE *file = NULL;
        int err, retv  = 0;
 
+       char buf[LARGE_MAXPATHLEN] = {0};
+
        file = setmntent(MTAB, "r");
        if (!file) {
                SYSERROR("failed to open %s", MTAB);
                return -1;
        }
 
-       while ((mntent = getmntent(file))) {
-               if (strcmp(mntent->mnt_type, "cgroup"))
+       while ((mntent = getmntent_r(file, &mntent_r, buf, sizeof(buf)))) {
+               if (strcmp(mntent_r.mnt_type, "cgroup"))
                        continue;
-               if (!mount_has_subsystem(mntent))
+               if (!mount_has_subsystem(&mntent_r))
                        continue;
 
-               err = lxc_one_cgroup_destroy(mntent, cgpath);
+               err = lxc_one_cgroup_destroy(&mntent_r, cgpath);
                if (err)  // keep trying to clean up the others
                        retv = -1;
        }

diff --git a/src/lxc/freezer.c b/src/lxc/freezer.c
index 111bc35404b8e0b28d20e3eaac2706f92be2f98e..35bf3a7648516c9d7b6fefc5cf702f107f9d0e06 100644 (file)
--- a/src/lxc/freezer.c
+++ b/src/lxc/freezer.c
@@ -120,14 +120,19 @@ out:
 
 static int freeze_unfreeze(const char *name, int freeze, const char *lxcpath)
 {
-       char *nsgroup;
+       char *nsgroup = NULL;
        int ret;
        
        ret = lxc_cgroup_path_get(&nsgroup, "freezer", name, lxcpath);
        if (ret)
-               return -1;
+               goto fail;
 
        return do_unfreeze(nsgroup, freeze, name, lxcpath);
+
+fail:
+       if (nsgroup)
+               free(nsgroup);
+       return -1;
 }
 
 int lxc_freeze(const char *name, const char *lxcpath)
@@ -143,12 +148,17 @@ int lxc_unfreeze(const char *name, const char *lxcpath)
 
 int lxc_unfreeze_bypath(const char *cgpath)
 {
-       char *nsgroup;
+       char *nsgroup = NULL;
        int ret;
        
        ret = cgroup_path_get(&nsgroup, "freezer", cgpath);
        if (ret)
-               return -1;
+               goto fail;
 
        return do_unfreeze(nsgroup, 0, NULL, NULL);
+
+fail:
+       if (nsgroup)
+               free(nsgroup);
+       return -1;
 }

diff --git a/src/lxc/state.c b/src/lxc/state.c
index 437f11aa66a0c995e1c15516ad45758058629aed..313cbb9ce98a31ff472e1518d08d6898e73a0539 100644 (file)
--- a/src/lxc/state.c
+++ b/src/lxc/state.c
@@ -69,7 +69,7 @@ lxc_state_t lxc_str2state(const char *state)
 
 static lxc_state_t freezer_state(const char *name, const char *lxcpath)
 {
-       char *nsgroup;
+       char *nsgroup = NULL;
        char freezer[MAXPATHLEN];
        char status[MAXPATHLEN];
        FILE *file;
@@ -77,25 +77,30 @@ static lxc_state_t freezer_state(const char *name, const char *lxcpath)
 
        err = lxc_cgroup_path_get(&nsgroup, "freezer", name, lxcpath);
        if (err)
-               return -1;
+               goto fail;
 
        err = snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
        if (err < 0 || err >= MAXPATHLEN)
-               return -1;
+               goto fail;
 
        file = fopen(freezer, "r");
        if (!file)
-               return -1;
+               goto fail;
 
        err = fscanf(file, "%s", status);
        fclose(file);
 
        if (err == EOF) {
                SYSERROR("failed to read %s", freezer);
-               return -1;
+               goto fail;
        }
 
        return lxc_str2state(status);
+
+fail:
+       if (nsgroup)
+               free(nsgroup);
+       return -1;
 }
 
 static lxc_state_t __lxc_getstate(const char *name, const char *lxcpath)