[lxc-devel] [PATCH] fix memory leaks in cgroup functions
Serge Hallyn
serge.hallyn at ubuntu.com
Sat May 25 00:17:14 UTC 2013
Quoting Serge Hallyn (serge.hallyn at ubuntu.com):
> Quoting Dwight Engen (dwight.engen at oracle.com):
> > There were several memory leaks in the cgroup functions, notably in the
> > success cases.
> >
> > The cgpath test program was refactored and additional tests added to it.
> > It was used in various modes under valgrind to test that the leaks were
> > fixed.
> >
> > Simplify lxc_cgroup_path_get() and cgroup_path_get by having them return a
> > char * instead of an int and an output char * argument. The only return
> > values ever used were -1 and 0, which are now handled with NULL and non-NULL
> > returns respectively.
> >
> > Use consistent variable names of cgabspath when refering to an absolute path
> > to a cgroup subsystem or file, and cgrelpath when refering to a container
> > "group/name" within the cgroup heirarchy.
>
> Excellent.
>
> > Remove unused subsystem argument to lxc_cmd_get_cgroup_path().
> >
> > Remove unused #define MAXPRIOLEN
> >
> > Make template arg to lxcapi_create() const
> >
> > Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
>
> This is great, thanks Dwight.
>
> Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
>
> Except for one thing below,
>
> > ---
> > src/lxc/cgroup.c | 270 ++++++++++++++++++-----------------
> > src/lxc/cgroup.h | 6 +-
> > src/lxc/commands.c | 4 +-
> > src/lxc/commands.h | 3 +-
> > src/lxc/freezer.c | 36 ++---
> > src/lxc/lxccontainer.c | 6 +-
> > src/lxc/lxccontainer.h | 4 +-
> > src/lxc/state.c | 38 ++---
> > src/tests/cgpath.c | 372 +++++++++++++++++++++++++++++++++++++------------
> > 9 files changed, 463 insertions(+), 276 deletions(-)
> >
> > diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
> > index bb1268b..f04d59a 100644
> > --- a/src/lxc/cgroup.c
> > +++ b/src/lxc/cgroup.c
> > @@ -147,102 +147,113 @@ out:
> > }
> >
> > /*
> > - * cgroup_path_get: Calculate the full path for a particular subsystem, plus
> > - * a passed-in (to be appended) relative cgpath for a container.
> > - * @path: a char** into which a pointer to the answer is copied
> > - * @subsystem: subsystem of interest (i.e. freezer).
> > - * @cgpath: a container's (relative) cgroup path, i.e. "/lxc/c1".
> > + * cgroup_path_get: Get the absolute path to a particular subsystem,
> > + * plus a passed-in (to be appended) relative cgpath for a container.
> > *
> > - * Returns 0 on success, -1 on error.
> > + * @subsystem : subsystem of interest (e.g. "freezer")
> > + * @cgrelpath : a container's relative cgroup path (e.g. "lxc/c1")
> > + *
> > + * Returns absolute path on success, NULL on error. The caller must free()
> > + * the returned path.
> > *
> > + * Note that @subsystem may be the name of an item (e.g. "freezer.state")
> > + * in which case the subsystem will be determined by taking the string up
> > + * to the first '.'
> > */
> > -extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpath)
> > +char *cgroup_path_get(const char *subsystem, const char *cgrelpath)
> > {
> > int rc;
> >
> > char *buf = NULL;
> > - char *retbuf = NULL;
> > + char *cgabspath = NULL;
> >
> > buf = malloc(MAXPATHLEN * sizeof(char));
> > if (!buf) {
> > ERROR("malloc failed");
> > - goto fail;
> > + goto out1;
> > }
> >
> > - retbuf = malloc(MAXPATHLEN * sizeof(char));
> > - if (!retbuf) {
> > + cgabspath = malloc(MAXPATHLEN * sizeof(char));
> > + if (!cgabspath) {
> > ERROR("malloc failed");
> > - goto fail;
> > + goto out2;
> > }
> >
> > /* 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);
> > + rc = snprintf(cgabspath, MAXPATHLEN, "%s", subsystem);
> > if (rc < 0 || rc >= MAXPATHLEN) {
> > ERROR("subsystem name too long");
> > - goto fail;
> > + goto err3;
> > }
> > - char *s = index(retbuf, '.');
> > + char *s = index(cgabspath, '.');
> > if (s)
> > *s = '\0';
> > - DEBUG("%s: called for subsys %s name %s\n", __func__, retbuf, cgpath);
> > + DEBUG("%s: called for subsys %s name %s\n", __func__,
> > + subsystem, cgrelpath);
> > }
> > - if (get_cgroup_mount(subsystem ? retbuf : NULL, buf)) {
> > + if (get_cgroup_mount(subsystem ? cgabspath : NULL, buf)) {
> > ERROR("cgroup is not mounted");
> > - goto fail;
> > + goto err3;
> > }
> >
> > - rc = snprintf(retbuf, MAXPATHLEN, "%s/%s", buf, cgpath);
> > + rc = snprintf(cgabspath, MAXPATHLEN, "%s/%s", buf, cgrelpath);
> > if (rc < 0 || rc >= MAXPATHLEN) {
> > ERROR("name too long");
> > - goto fail;
> > + goto err3;
> > }
> >
> > - DEBUG("%s: returning %s for subsystem %s", __func__, retbuf, subsystem);
> > + DEBUG("%s: returning %s for subsystem %s relpath %s", __func__,
> > + cgabspath, subsystem, cgrelpath);
> > + goto out2;
> >
> > - if(buf)
> > - free(buf);
> > -
> > - *path = retbuf;
> > - return 0;
> > -fail:
> > - if (buf)
> > - free(buf);
> > - if (retbuf)
> > - free(retbuf);
> > - return -1;
> > +err3:
> > + free(cgabspath);
> > + cgabspath = NULL;
> > +out2:
> > + free(buf);
> > +out1:
> > + return cgabspath;
> > }
> >
> > /*
> > - * lxc_cgroup_path_get: determine full pathname for a cgroup
> > - * file for a specific container.
> > - * @path: char ** used to return the answer.
> > - * @subsystem: cgroup subsystem (i.e. "freezer") for which to
> > - * return an answer. If NULL, then the first cgroup entry in
> > - * mtab will be used.
> > + * lxc_cgroup_path_get: Get the absolute pathname for a cgroup
> > + * file for a running container.
> > + *
> > + * @subsystem : subsystem of interest (e.g. "freezer"). If NULL, then
> > + * the first cgroup entry in mtab will be used.
> > + * @name : name of container to connect to
> > + * @lxcpath : the lxcpath in which the container is running
> > *
> > * This is the exported function, which determines cgpath from the
> > - * monitor running in lxcpath.
> > + * lxc-start of the @name container running in @lxcpath.
> > *
> > - * Returns 0 on success, < 0 on error.
> > + * Returns path on success, NULL on error. The caller must free()
> > + * the returned path.
> > */
> > -int lxc_cgroup_path_get(char **path, const char *subsystem, const char *name, const char *lxcpath)
> > +char *lxc_cgroup_path_get(const char *subsystem, const char *name,
> > + const char *lxcpath)
> > {
> > - int ret;
> > - char *cgpath;
> > + char *cgabspath;
> > + char *cgrelpath;
> >
> > - cgpath = lxc_cmd_get_cgroup_path(subsystem, name, lxcpath);
> > - if (!cgpath)
> > - return -1;
> > + cgrelpath = lxc_cmd_get_cgroup_path(name, lxcpath);
> > + if (!cgrelpath)
> > + return NULL;
> >
> > - ret = cgroup_path_get(path, subsystem, cgpath);
> > - free(cgpath);
> > - return ret;
> > + cgabspath = cgroup_path_get(subsystem, cgrelpath);
> > + free(cgrelpath);
> > + return cgabspath;
> > }
> >
> > /*
> > - * small helper which simply write a value into a (cgroup) file
> > + * do_cgroup_set: Write a value into a cgroup file
> > + *
> > + * @path : absolute path to cgroup file
> > + * @value : value to write into file
> > + *
> > + * Returns 0 on success, < 0 on error.
> > */
> > static int do_cgroup_set(const char *path, const char *value)
> > {
> > @@ -267,87 +278,86 @@ static int do_cgroup_set(const char *path, const char *value)
> > }
> >
> > /*
> > - * small helper to write a value into a file in a particular directory.
> > - * @cgpath: the directory in which to find the file
> > - * @filename: the file (under cgpath) to which to write
> > - * @value: what to write
> > + * lxc_cgroup_set_bypath: Write a value into a cgroup file
> > + *
> > + * @cgrelpath : a container's relative cgroup path (e.g. "lxc/c1")
> > + * @filename : the cgroup file to write (e.g. "freezer.state")
> > + * @value : value to write into file
> > *
> > * Returns 0 on success, < 0 on error.
> > */
> > -int lxc_cgroup_set_bypath(const char *cgpath, const char *filename, const char *value)
> > +int lxc_cgroup_set_bypath(const char *cgrelpath, const char *filename, const char *value)
> > {
> > int ret;
> > - char *dirpath = NULL;
> > + char *cgabspath;
> > char path[MAXPATHLEN];
> >
> > - ret = cgroup_path_get(&dirpath, filename, cgpath);
> > - if (ret)
> > - goto fail;
> > + cgabspath = cgroup_path_get(filename, cgrelpath);
> > + if (!cgabspath)
> > + return -1;
> >
> > - ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
> > + ret = snprintf(path, MAXPATHLEN, "%s/%s", cgabspath, filename);
> > if (ret < 0 || ret >= MAXPATHLEN) {
> > ERROR("pathname too long");
> > - goto fail;
> > + ret = -1;
> > + goto out;
> > }
> >
> > - return do_cgroup_set(path, value);
> > + ret = do_cgroup_set(path, value);
> >
> > -fail:
> > - if(dirpath)
> > - free(dirpath);
> > - return -1;
> > +out:
> > + free(cgabspath);
> > + return ret;
> > }
> >
> > /*
> > - * set a cgroup value for a container
> > + * lxc_cgroup_set: Write a value into a cgroup file
> > *
> > - * @name: name of the container
> > - * @filename: the cgroup file (i.e. freezer.state) whose value to change
> > - * @value: the value to write to the file
> > - * @lxcpath: the lxcpath under which the container is running.
> > + * @name : name of container to connect to
> > + * @filename : the cgroup file to write (e.g. "freezer.state")
> > + * @value : value to write into file
> > + * @lxcpath : the lxcpath in which the container is running
> > *
> > * Returns 0 on success, < 0 on error.
> > */
> > -
> > int lxc_cgroup_set(const char *name, const char *filename, const char *value,
> > const char *lxcpath)
> > {
> > int ret;
> > - char *dirpath = NULL;
> > + char *cgabspath;
> > char path[MAXPATHLEN];
> >
> > - ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
> > - if (ret)
> > - goto fail;
> > + cgabspath = lxc_cgroup_path_get(filename, name, lxcpath);
> > + if (!cgabspath)
> > + return -1;
> >
> > - ret = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
> > + ret = snprintf(path, MAXPATHLEN, "%s/%s", cgabspath, filename);
> > if (ret < 0 || ret >= MAXPATHLEN) {
> > ERROR("pathname too long");
> > - goto fail;
> > + ret = -1;
> > + goto out;
> > }
> >
> > - return do_cgroup_set(path, value);
> > + ret = do_cgroup_set(path, value);
> >
> > -fail:
> > - if(dirpath)
> > - free(dirpath);
> > - return -1;
> > +out:
> > + free(cgabspath);
> > + return ret;
> > }
> >
> > /*
> > - * Get value of a cgroup setting for a container.
> > + * lxc_cgroup_get: Read value from a cgroup file
> > *
> > - * @name: name of the container
> > - * @filename: the cgroup file to read (i.e. 'freezer.state')
> > - * @value: a preallocated char* into which to copy the answer
> > - * @len: the length of pre-allocated @value
> > - * @lxcpath: the lxcpath in which the container is running (i.e.
> > - * /var/lib/lxc)
> > + * @name : name of container to connect to
> > + * @filename : the cgroup file to read (e.g. "freezer.state")
> > + * @value : a pre-allocated buffer to copy the answer into
> > + * @len : the length of pre-allocated @value
> > + * @lxcpath : the lxcpath in which the container is running
> > *
> > - * Returns < 0 on error, or the number of bytes read.
> > + * Returns the number of bytes read on success, < 0 on error
> > *
> > - * If you pass in NULL value or 0 len, then you are asking for the size of the
> > - * file.
> > + * If you pass in NULL value or 0 len, the return value will be the size of
> > + * the file, and @value will not contain the contents.
> > *
> > * Note that we can't get the file size quickly through stat or lseek.
> > * Therefore if you pass in len > 0 but less than the file size, your only
> > @@ -357,25 +367,26 @@ fail:
> > int lxc_cgroup_get(const char *name, const char *filename, char *value,
> > size_t len, const char *lxcpath)
> > {
> > - int fd, ret = -1;
> > - char *dirpath = NULL;
> > + int fd, ret;
> > + char *cgabspath;
> > char path[MAXPATHLEN];
> > - int rc;
> >
> > - ret = lxc_cgroup_path_get(&dirpath, filename, name, lxcpath);
> > - if (ret)
> > - goto fail;
> > + cgabspath = lxc_cgroup_path_get(filename, name, lxcpath);
> > + if (!cgabspath)
> > + return -1;
> >
> > - rc = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
> > - if (rc < 0 || rc >= MAXPATHLEN) {
> > + ret = snprintf(path, MAXPATHLEN, "%s/%s", cgabspath, filename);
> > + if (ret < 0 || ret >= MAXPATHLEN) {
> > ERROR("pathname too long");
> > - goto fail;
> > + ret = -1;
> > + goto out;
> > }
> >
> > fd = open(path, O_RDONLY);
> > if (fd < 0) {
> > ERROR("open %s : %s", path, strerror(errno));
> > - goto fail;
> > + ret = -1;
> > + goto out;
> > }
> >
> > if (!len || !value) {
> > @@ -394,47 +405,45 @@ int lxc_cgroup_get(const char *name, const char *filename, char *value,
> > ERROR("read %s : %s", path, strerror(errno));
> >
> > close(fd);
> > +out:
> > + free(cgabspath);
> > return ret;
> > -fail:
> > - if(dirpath)
> > - free(dirpath);
> > - return -1;
> > }
> >
> > -int lxc_cgroup_nrtasks(const char *cgpath)
> > +int lxc_cgroup_nrtasks(const char *cgrelpath)
> > {
> > - char *dirpath = NULL;
> > + char *cgabspath = NULL;
> > char path[MAXPATHLEN];
> > - int pid, ret, count = 0;
> > + int pid, ret;
> > FILE *file;
> > - int rc;
> >
> > - ret = cgroup_path_get(&dirpath, NULL, cgpath);
> > - if (ret)
> > - goto fail;
> > + cgabspath = cgroup_path_get(NULL, cgrelpath);
> > + if (!cgabspath)
> > + return -1;
> >
> > - rc = snprintf(path, MAXPATHLEN, "%s/tasks", dirpath);
> > - if (rc < 0 || rc >= MAXPATHLEN) {
> > + ret = snprintf(path, MAXPATHLEN, "%s/tasks", cgabspath);
> > + if (ret < 0 || ret >= MAXPATHLEN) {
> > ERROR("pathname too long");
> > - goto fail;
> > + ret = -1;
> > + goto out;
> > }
> >
> > file = fopen(path, "r");
> > if (!file) {
> > SYSERROR("fopen '%s' failed", path);
> > - return -1;
> > + ret = -1;
> > + goto out;
> > }
> >
> > + ret = 0;
> > while (fscanf(file, "%d", &pid) != EOF)
> > - count++;
> > + ret++;
> >
> > fclose(file);
> >
> > - return count;
> > -fail:
> > - if(dirpath)
> > - free(dirpath);
> > - return -1;
> > +out:
> > + free(cgabspath);
> > + return ret;
> > }
> >
> > /*
> > @@ -654,12 +663,12 @@ char *lxc_cgroup_path_create(const char *lxcgroup, const char *name)
> >
> > char buf[LARGE_MAXPATHLEN] = {0};
> >
> > - if (create_lxcgroups(lxcgroup) < 0)
> > - return NULL;
> > -
> > if (!allcgroups)
> > return NULL;
> >
> > + if (create_lxcgroups(lxcgroup) < 0)
> > + goto err1;
> > +
> > again:
> > if (visited) {
> > /* we're checking for a new name, so start over with all cgroup
> > @@ -670,9 +679,7 @@ again:
> > file = setmntent(MTAB, "r");
> > if (!file) {
> > SYSERROR("failed to open %s", MTAB);
> > - if (allcgroups)
> > - free(allcgroups);
> > - return NULL;
> > + goto err1;
> > }
> >
> > if (i)
> > @@ -730,6 +737,7 @@ next:
> >
> > fail:
> > endmntent(file);
> > +err1:
> > free(allcgroups);
> > if (visited)
> > free(visited);
> > @@ -880,7 +888,7 @@ int lxc_cgroup_attach(pid_t pid, const char *name, const char *lxcpath)
> > int ret;
> > char *dirpath;
> >
> > - dirpath = lxc_cmd_get_cgroup_path(NULL, name, lxcpath);
> > + dirpath = lxc_cmd_get_cgroup_path(name, lxcpath);
> > if (!dirpath) {
> > ERROR("Error getting cgroup for container %s: %s", lxcpath, name);
> > return -1;
> > diff --git a/src/lxc/cgroup.h b/src/lxc/cgroup.h
> > index 971311e..747ff5c 100644
> > --- a/src/lxc/cgroup.h
> > +++ b/src/lxc/cgroup.h
> > @@ -23,15 +23,13 @@
> > #ifndef _cgroup_h
> > #define _cgroup_h
> >
> > -#define MAXPRIOLEN 24
> > -
> > struct lxc_handler;
> > extern int lxc_cgroup_destroy(const char *cgpath);
> > -extern int lxc_cgroup_path_get(char **path, const char *subsystem, const char *name,
> > +extern char *lxc_cgroup_path_get(const char *subsystem, const char *name,
> > const char *lxcpath);
> > extern int lxc_cgroup_nrtasks(const char *cgpath);
> > extern char *lxc_cgroup_path_create(const char *lxcgroup, const char *name);
> > extern int lxc_cgroup_enter(const char *cgpath, pid_t pid);
> > extern int lxc_cgroup_attach(pid_t pid, const char *name, const char *lxcpath);
> > -extern int cgroup_path_get(char **path, const char *subsystem, const char *cgpath);
> > +extern char *cgroup_path_get(const char *subsystem, const char *cgpath);
> > #endif
> > diff --git a/src/lxc/commands.c b/src/lxc/commands.c
> > index 3f21488..169914e 100644
> > --- a/src/lxc/commands.c
> > +++ b/src/lxc/commands.c
> > @@ -335,15 +335,13 @@ static int lxc_cmd_get_clone_flags_callback(int fd, struct lxc_cmd_req *req,
> > * particular subsystem. This is the cgroup path relative to the root
> > * of the cgroup filesystem.
> > *
> > - * @subsystem : the cgroup subsystem of interest (i.e. freezer)
> > * @name : name of container to connect to
> > * @lxcpath : the lxcpath in which the container is running
> > *
> > * Returns the path on success, NULL on failure. The caller must free() the
> > * returned path.
> > */
> > -char *lxc_cmd_get_cgroup_path(const char *subsystem, const char *name,
> > - const char *lxcpath)
> > +char *lxc_cmd_get_cgroup_path(const char *name, const char *lxcpath)
> > {
> > int ret, stopped = 0;
> > struct lxc_cmd_rr cmd = {
> > diff --git a/src/lxc/commands.h b/src/lxc/commands.h
> > index b5b4788..08bde9c 100644
> > --- a/src/lxc/commands.h
> > +++ b/src/lxc/commands.h
> > @@ -67,8 +67,7 @@ struct lxc_cmd_console_rsp_data {
> >
> > extern int lxc_cmd_console(const char *name, int *ttynum, int *fd,
> > const char *lxcpath);
> > -extern char *lxc_cmd_get_cgroup_path(const char *subsystem,
> > - const char *name, const char *lxcpath);
> > +extern char *lxc_cmd_get_cgroup_path(const char *name, const char *lxcpath);
> > extern int lxc_cmd_get_clone_flags(const char *name, const char *lxcpath);
> > extern char *lxc_cmd_get_config_item(const char *name, const char *item, const char *lxcpath);
> > extern pid_t lxc_cmd_get_init_pid(const char *name, const char *lxcpath);
> > diff --git a/src/lxc/freezer.c b/src/lxc/freezer.c
> > index 35bf3a7..37a07fd 100644
> > --- a/src/lxc/freezer.c
> > +++ b/src/lxc/freezer.c
> > @@ -120,19 +120,16 @@ out:
> >
> > static int freeze_unfreeze(const char *name, int freeze, const char *lxcpath)
> > {
> > - char *nsgroup = NULL;
> > + char *cgabspath;
> > int ret;
> > -
> > - ret = lxc_cgroup_path_get(&nsgroup, "freezer", name, lxcpath);
> > - if (ret)
> > - goto fail;
> >
> > - return do_unfreeze(nsgroup, freeze, name, lxcpath);
> > + cgabspath = lxc_cgroup_path_get("freezer", name, lxcpath);
> > + if (!cgabspath)
> > + return -1;
> >
> > -fail:
> > - if (nsgroup)
> > - free(nsgroup);
> > - return -1;
> > + ret = do_unfreeze(cgabspath, freeze, name, lxcpath);
> > + free(cgabspath);
> > + return ret;
> > }
> >
> > int lxc_freeze(const char *name, const char *lxcpath)
> > @@ -146,19 +143,16 @@ int lxc_unfreeze(const char *name, const char *lxcpath)
> > return freeze_unfreeze(name, 0, lxcpath);
> > }
> >
> > -int lxc_unfreeze_bypath(const char *cgpath)
> > +int lxc_unfreeze_bypath(const char *cgrelpath)
> > {
> > - char *nsgroup = NULL;
> > + char *cgabspath;
> > int ret;
> > -
> > - ret = cgroup_path_get(&nsgroup, "freezer", cgpath);
> > - if (ret)
> > - goto fail;
> >
> > - return do_unfreeze(nsgroup, 0, NULL, NULL);
> > + cgabspath = cgroup_path_get("freezer", cgrelpath);
> > + if (!cgabspath)
> > + return -1;
> >
> > -fail:
> > - if (nsgroup)
> > - free(nsgroup);
> > - return -1;
> > + ret = do_unfreeze(cgabspath, 0, NULL, NULL);
> > + free(cgabspath);
> > + return ret;
> > }
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 2934afa..b2e5e36 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -570,7 +570,7 @@ static bool create_container_dir(struct lxc_container *c)
> > * for ->create, argv contains the arguments to pass to the template,
> > * terminated by NULL. If no arguments, you can just pass NULL.
> > */
> > -static bool lxcapi_create(struct lxc_container *c, char *t, char *const argv[])
> > +static bool lxcapi_create(struct lxc_container *c, const char *t, char *const argv[])
> > {
> > bool bret = false;
> > pid_t pid;
> > @@ -636,7 +636,7 @@ static bool lxcapi_create(struct lxc_container *c, char *t, char *const argv[])
> > newargv = malloc(nargs * sizeof(*newargv));
> > if (!newargv)
> > exit(1);
> > - newargv[0] = t;
> > + newargv[0] = (char *)t;
>
> You're typecasting const char* to char*?
>
> I agree with making the template arg const char *, but then if you need
> to do this, it's better to make newargv a char *const (or whatever :) right?
I went ahead and pushed as is (the compile didn't complain) for now.
thanks,
-serge
More information about the lxc-devel
mailing list