After a report from jsg about a memory leak (or was it a double free?),

refactor the code around getcwd and canonpath, with some help from semarie
ok semarie

1 files changed, 26 insertions, 38 deletions

diff --git a/sys/kern/kern_tame.c b/sys/kern/kern_tame.c
index 75690fa7a19..b61fd02a51d 100644
--- a/sys/kern/kern_tame.c
+++ b/sys/kern/kern_tame.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_tame.c,v 1.31 2015/08/26 05:55:53 doug Exp $	*/
+/*	$OpenBSD: kern_tame.c,v 1.32 2015/08/26 06:33:57 deraadt Exp $	*/
 
 /*
  * Copyright (c) 2015 Nicholas Marriott <nicm@openbsd.org>
@@ -230,7 +230,7 @@ sys_tame(struct proc *p, void *v, register_t *retval)
 		const char **u = SCARG(uap, paths), *sp;
 		struct whitepaths *wl;
 		char *cwdpath = NULL, *path;
-		size_t cwdpathlen, cwdlen, len, maxargs = 0;
+		size_t cwdpathlen = MAXPATHLEN * 4, cwdlen, len, maxargs = 0;
 		int i, error;
 
 		if (p->p_p->ps_tamepaths)
@@ -257,7 +257,7 @@ sys_tame(struct proc *p, void *v, register_t *retval)
 		/* Copy in */
 		for (i = 0; i < wl->wl_count; i++) {
 			char *fullpath = NULL, *builtpath = NULL, *canopath = NULL, *cwd;
-			size_t builtlen;
+			size_t builtlen = 0;
 
 			if ((error = copyin(u + i, &sp, sizeof(sp))) != 0)
 				break;
@@ -271,7 +271,6 @@ sys_tame(struct proc *p, void *v, register_t *retval)
 				if (cwdpath == NULL) {
 					char *bp, *bpend;
 
-					cwdpathlen = MAXPATHLEN * 4;
 					cwdpath = malloc(cwdpathlen, M_TEMP, M_WAITOK);
 					bp = &cwdpath[cwdpathlen];
 					bpend = bp;
@@ -280,10 +279,8 @@ sys_tame(struct proc *p, void *v, register_t *retval)
 					error = vfs_getcwd_common(p->p_fd->fd_cdir,
 					    NULL, &bp, cwdpath, cwdpathlen/2,
 					    GETCWD_CHECK_ACCESS, p);
-					if (error) {
-						printf("getcwd: %d\n", error);
+					if (error)
 						break;
-					}
 					cwd = bp;
 					cwdlen = (bpend - bp);
 				}
@@ -302,17 +299,14 @@ sys_tame(struct proc *p, void *v, register_t *retval)
 				fullpath = path;
 
 			canopath = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
-			if (canonpath(fullpath, canopath, MAXPATHLEN) != 0) {
-				free(path, M_TEMP, MAXPATHLEN);
+			error = canonpath(fullpath, canopath, MAXPATHLEN);
+
+			free(builtpath, M_TEMP, builtlen);
+			if (error != 0) {
 				free(canopath, M_TEMP, MAXPATHLEN);
-				if (builtpath)
-					free(builtpath, M_TEMP, builtlen);
-				return (tame_fail(p, EPERM, TAME_RPATH));
+				break;
 			}
 
-			if (builtpath)
-				free(builtpath, M_TEMP, builtlen);
-
 			len = strlen(canopath) + 1;
 
 			//printf("tame: canopath = %s %lld strlen %lld\n", canopath,
@@ -328,8 +322,7 @@ sys_tame(struct proc *p, void *v, register_t *retval)
 			free(canopath, M_TEMP, MAXPATHLEN);
 		}
 		free(path, M_TEMP, MAXPATHLEN);
-		if (cwdpath)
-			free(cwdpath, M_TEMP, cwdpathlen);
+		free(cwdpath, M_TEMP, cwdpathlen);
 
 		if (error) {
 			printf("%s(%d): path load error %d\n",
@@ -515,15 +508,14 @@ tame_namei(struct proc *p, char *origpath)
 	 */
 	if (p->p_p->ps_tamepaths) {
 		struct whitepaths *wl = p->p_p->ps_tamepaths;
-		char *fullpath = path, *builtpath = NULL, *canopath = NULL;
-		char *cwdpath = NULL, *cwd;
-		size_t cwdpathlen, cwdlen, builtlen;
+		char *fullpath, *builtpath = NULL, *canopath = NULL;
+		size_t builtlen = 0;
 		int i, error;
 
 		if (origpath[0] != '/') {
-			char *bp, *bpend;
+			char *cwdpath, *cwd, *bp, *bpend;
+			size_t cwdpathlen = MAXPATHLEN * 4, cwdlen;
 
-			cwdpathlen = MAXPATHLEN * 4;
 			cwdpath = malloc(cwdpathlen, M_TEMP, M_WAITOK);
 			bp = &cwdpath[cwdpathlen];
 			bpend = bp;
@@ -534,7 +526,6 @@ tame_namei(struct proc *p, char *origpath)
 			    GETCWD_CHECK_ACCESS, p);
 			if (error) {
 				free(cwdpath, M_TEMP, cwdpathlen);
-				printf("getcwd: %d\n", error);
 				return (error);
 			}
 			cwd = bp;
@@ -544,25 +535,27 @@ tame_namei(struct proc *p, char *origpath)
 			builtlen = cwdlen + 1 + strlen(origpath);
 			builtpath = malloc(builtlen, M_TEMP, M_WAITOK);
 			snprintf(builtpath, builtlen, "%s/%s", cwd, origpath);
+			fullpath = builtpath;
+			free(cwdpath, M_TEMP, cwdpathlen);
+
 			//printf("namei: builtpath = %s %lld strlen %lld\n", builtpath,
 			//    (long long)builtlen, (long long)strlen(builtpath));
-			fullpath = builtpath;
-		}
+		} else
+			fullpath = path;
 
 		canopath = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
-		if (canonpath(fullpath, canopath, MAXPATHLEN) != 0) {
-			if (cwdpath)
-				free(cwdpath, M_TEMP, cwdpathlen);
+		error = canonpath(fullpath, canopath, MAXPATHLEN);
+
+		free(builtpath, M_TEMP, builtlen);
+		if (error != 0) {
 			free(canopath, M_TEMP, MAXPATHLEN);
-			if (builtpath)
-				free(builtpath, M_TEMP, builtlen);
 			return (tame_fail(p, EPERM, TAME_RPATH));
 		}
 
 		//printf("namei: canopath = %s strlen %lld\n", canopath,
 		//    (long long)strlen(canopath));
 
-		error = EACCES;
+		error = ENOENT;
 		for (i = 0; i < wl->wl_count && wl->wl_paths[i].name && error; i++) {
 			if (strncmp(canopath, wl->wl_paths[i].name,
 			    wl->wl_paths[i].len - 1) == 0) {
@@ -572,15 +565,10 @@ tame_namei(struct proc *p, char *origpath)
 					error = 0;
 			}
 		}
-		if (error)
-			printf("bad path: %s\n", canopath);
-		if (cwdpath)
-			free(cwdpath, M_TEMP, cwdpathlen);
 		free(canopath, M_TEMP, MAXPATHLEN);
-		if (builtpath)
-			free(builtpath, M_TEMP, builtlen);
 		if (error)
-			return (ENOENT);	/* Don't hint why it failed */
+			printf("bad path: %s\n", canopath);
+		return (error);			/* Don't hint why it failed */
 	}
 
 	if (p->p_p->ps_tame & _TM_RPATH)