[syslinux:elflink] fs: Fix searchdir resource leak

syslinux-bot for Shao Miller sha0.miller at gmail.com
Fri Nov 30 05:48:02 PST 2012


Commit-ID:  57acc34bdf83fc5ea08dbf44b74a5dd2c1131187
Gitweb:     http://www.syslinux.org/commit/57acc34bdf83fc5ea08dbf44b74a5dd2c1131187
Author:     Shao Miller <sha0.miller at gmail.com>
AuthorDate: Fri, 2 Nov 2012 11:59:10 -0400
Committer:  Shao Miller <sha0.miller at gmail.com>
CommitDate: Thu, 29 Nov 2012 13:51:22 -0500

fs: Fix searchdir resource leak

This is a significant rewrite of the generic lookup logic inside
core/fs/fs.c's searchdir function.  Previously, there was a
memory leak if a path involved multiple directories.  After a
sufficiently large number of invocations, this could be observed.

Reported-by: Ady <ady-sf at hotmail.com>
Signed-off-by: Shao Miller <sha0.miller at gmail.com>

---
 core/fs/fs.c | 263 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 155 insertions(+), 108 deletions(-)

diff --git a/core/fs/fs.c b/core/fs/fs.c
index 7b0f6ce..c85e132 100644
--- a/core/fs/fs.c
+++ b/core/fs/fs.c
@@ -219,11 +219,10 @@ void pm_searchdir(com32sys_t *regs)
 
 int searchdir(const char *name)
 {
-    struct inode *inode = NULL;
-    struct inode *parent = NULL;
+    static char root_name[] = "/";
     struct file *file;
-    char *pathbuf = NULL;
-    char *part, *p, echar;
+    char *path, *inode_name, *next_inode_name;
+    struct inode *tmp, *inode = NULL;
     int symlink_count = MAX_SYMLINK_CNT;
 
     dprintf("searchdir: %s  root: %p  cwd: %p\n",
@@ -245,113 +244,165 @@ int searchdir(const char *name)
 
     /* else, try the generic-path-lookup method */
 
-    parent = get_inode(this_fs->cwd);
-    p = pathbuf = strdup(name);
-    if (!pathbuf)
-	goto err;
+    /* Copy the path */
+    path = strdup(name);
+    if (!path) {
+	dprintf("searchdir: Couldn't copy path\n");
+	goto err_path;
+    }
+
+    /* Work with the current directory, by default */
+    inode = get_inode(this_fs->cwd);
+    if (!inode) {
+	dprintf("searchdir: Couldn't use current directory\n");
+	goto err_curdir;
+    }
 
-    do {
-    got_link:
-	if (*p == '/') {
-	    put_inode(parent);
-	    parent = get_inode(this_fs->root);
+    for (inode_name = path; inode_name; inode_name = next_inode_name) {
+	/* Root directory? */
+	if (inode_name[0] == '/') {
+	    next_inode_name = inode_name + 1;
+	    inode_name = root_name;
+	} else {
+	    /* Find the next inode name */
+	    next_inode_name = strchr(inode_name + 1, '/');
+	    if (next_inode_name) {
+		/* Terminate the current inode name and point to next */
+		*next_inode_name++ = '\0';
+	    }
+	}
+	if (next_inode_name) {
+	    /* Advance beyond redundant slashes */
+	    while (*next_inode_name == '/')
+		next_inode_name++;
+
+	    /* Check if we're at the end */
+	    if (*next_inode_name == '\0')
+		next_inode_name = NULL;
+	}
+	dprintf("searchdir: inode_name: %s\n", inode_name);
+	if (next_inode_name)
+	    dprintf("searchdir: Remaining: %s\n", next_inode_name);
+
+	/* Root directory? */
+	if (inode_name[0] == '/') {
+	    /* Release any chain that's already been established */
+	    put_inode(inode);
+	    inode = get_inode(this_fs->root);
+	    continue;
 	}
 
-	do {
-	    inode = get_inode(parent);
-
-	    while (*p == '/')
-		p++;
-
-	    if (!*p)
-		break;
-
-	    part = p;
-	    while ((echar = *p) && echar != '/')
-		p++;
-	    *p++ = '\0';
-
-	    if (part[0] == '.' && part[1] == '.' && part[2] == '\0') {
-		if (inode->parent) {
-		    put_inode(parent);
-		    parent = get_inode(inode->parent);
-		    put_inode(inode);
-		    inode = NULL;
-		    if (!echar) {
-			/* Terminal double dots */
-			inode = parent;
-			parent = inode->parent ?
-			    get_inode(inode->parent) : NULL;
-		    }
-		}
-	    } else if (part[0] != '.' || part[1] != '\0') {
-		inode = this_fs->fs_ops->iget(part, parent);
-		if (!inode)
-		    goto err;
-		if (inode->mode == DT_LNK) {
-		    char *linkbuf, *q;
-		    int name_len = echar ? strlen(p) : 0;
-		    int total_len = inode->size + name_len + 2;
-		    int link_len;
-
-		    if (!this_fs->fs_ops->readlink ||
-			--symlink_count == 0       ||      /* limit check */
-			total_len > MAX_SYMLINK_BUF)
-			goto err;
-
-		    linkbuf = malloc(total_len);
-		    if (!linkbuf)
-			goto err;
-
-		    link_len = this_fs->fs_ops->readlink(inode, linkbuf);
-		    if (link_len <= 0) {
-			free(linkbuf);
-			goto err;
-		    }
-
-		    q = linkbuf + link_len;
-
-		    if (echar) {
-			if (link_len > 0 && q[-1] != '/')
-			    *q++ = '/';
-
-			memcpy(q, p, name_len+1);
-		    } else {
-			*q = '\0';
-		    }
-
-		    free(pathbuf);
-		    p = pathbuf = linkbuf;
-		    put_inode(inode);
-		    inode = NULL;
-		    goto got_link;
-		}
-
-		inode->name = strdup(part);
-		dprintf("path component: %s\n", inode->name);
-
-		inode->parent = parent;
-		parent = NULL;
-
-		if (!echar)
-		    break;
-
-		if (inode->mode != DT_DIR)
-		    goto err;
-
-		parent = inode;
-		inode = NULL;
+	/* Current directory? */
+	if (!strncmp(inode_name, ".", sizeof "."))
+	    continue;
+
+	/* Parent directory? */
+	if (!strncmp(inode_name, "..", sizeof "..")) {
+	    /* If there is no parent, just ignore it */
+	    if (!inode->parent)
+		continue;
+
+	    /* Add a reference to the parent so we can release the child */
+	    tmp = get_inode(inode->parent);
+
+	    /* Releasing the child will drop the parent back down to 1 */
+	    put_inode(inode);
+
+	    inode = tmp;
+	    continue;
+	}
+
+	/* Anything else */
+	tmp = inode;
+	inode = this_fs->fs_ops->iget(inode_name, inode);
+	if (!inode) {
+	    /* Failure.  Release the chain */
+	    put_inode(tmp);
+	    break;
+	}
+
+	/* Sanity-check */
+	if (inode->parent && inode->parent != tmp) {
+	    dprintf("searchdir: iget returned a different parent\n");
+	    put_inode(inode);
+	    inode = NULL;
+	    put_inode(tmp);
+	    break;
+	}
+	inode->parent = tmp;
+	inode->name = strdup(inode_name);
+	dprintf("searchdir: path component: %s\n", inode->name);
+
+	/* Symlink handling */
+	if (inode->mode == DT_LNK) {
+	    char *new_path;
+	    int new_len, copied;
+
+	    /* target path + NUL */
+	    new_len = inode->size + 1;
+
+	    if (next_inode_name) {
+		/* target path + slash + remaining + NUL */
+		new_len += strlen(next_inode_name) + 1;
+	    }
+
+	    if (!this_fs->fs_ops->readlink ||
+		/* limit checks */
+		--symlink_count == 0 ||
+		new_len > MAX_SYMLINK_BUF)
+		goto err_new_len;
+
+	    new_path = malloc(new_len);
+	    if (!new_path)
+		goto err_new_path;
+
+	    copied = this_fs->fs_ops->readlink(inode, new_path);
+	    if (copied <= 0)
+		goto err_copied;
+	    new_path[copied] = '\0';
+	    dprintf("searchdir: Symlink: %s\n", new_path);
+
+	    if (next_inode_name) {
+		new_path[copied] = '/';
+		strcpy(new_path + copied + 1, next_inode_name);
+		dprintf("searchdir: New path: %s\n", new_path);
 	    }
-	} while (echar);
-    } while (0);
 
-    free(pathbuf);
-    pathbuf = NULL;
-    put_inode(parent);
-    parent = NULL;
+	    free(path);
+	    path = next_inode_name = new_path;
 
-    if (!inode)
+            /* Add a reference to the parent so we can release the child */
+            tmp = get_inode(inode->parent);
+
+            /* Releasing the child will drop the parent back down to 1 */
+            put_inode(inode);
+
+            inode = tmp;
+	    continue;
+err_copied:
+	    free(new_path);
+err_new_path:
+err_new_len:
+	    put_inode(inode);
+	    inode = NULL;
+	    break;
+	}
+
+	/* If there's more to process, this should be a directory */
+	if (next_inode_name && inode->mode != DT_DIR) {
+	    dprintf("searchdir: Expected a directory\n");
+	    put_inode(inode);
+	    inode = NULL;
+	    break;
+	}
+    }
+err_curdir:
+    free(path);
+err_path:
+    if (!inode) {
+	dprintf("searchdir: Not found\n");
 	goto err;
+    }
 
     file->inode  = inode;
     file->offset = 0;
@@ -359,10 +410,6 @@ int searchdir(const char *name)
     return file_to_handle(file);
 
 err:
-    put_inode(inode);
-    put_inode(parent);
-    if (pathbuf)
-	free(pathbuf);
     _close_file(file);
 err_no_close:
     return -1;


More information about the Syslinux-commits mailing list