Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 13 Mar 2004 07:30:42 +0100 (CET)
From:      =?ISO-8859-1?Q?Magnus_B=E4ckstr=F6m?= <b@etek.chalmers.se>
To:        FreeBSD-gnats-submit@FreeBSD.org
Subject:   kern/64196: Remove the arbitrary MAXSHELLCMDLEN [PATCH]
Message-ID:  <20040313070129.R680@rockerduck.eep1>
Resent-Message-ID: <200403130640.i2D6e9Yh052603@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help

>Number:         64196
>Category:       kern
>Synopsis:       Remove the arbitrary MAXSHELLCMDLEN
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          change-request
>Submitter-Id:   current-users
>Arrival-Date:   Fri Mar 12 22:40:08 PST 2004
>Closed-Date:
>Last-Modified:
>Originator:     Magnus Bäckström
>Release:        FreeBSD 5.2-CURRENT i386
>Organization:
Chalmers University of Technology
>Environment:
System: FreeBSD rockerduck.eep1 5.2-CURRENT FreeBSD 5.2-CURRENT #3: Sat Mar 13 00:12:59 CET 2004 b@rockerduck.eep1:/foo/cur/sys.mine/i386/compile/I4100 i386



>Description:

Remove the arbitrary MAXSHELLCMDLEN on the allowed length
of #!/command/interpreter lines.  It bites me every now and then, and
is easy enough to walk around (bump the default value of 128 a little)
but the limit is really rather silly and should be fixed.

MAXSHELLCMDLEN's reason for existence at all is connected with the
size of struct image_params, which is allocated off the kernel stack
in kern_execve() in sys/kern/kern_exec.c.

The patch below removes the troublesome connection with stack size, and
does away with MAXSHELLCMDLEN.  This allows command interpreters to be
up to PAGE_SIZE long.

NB: linux.ko if used should be rebuilt along with the kernel image after
    applying this fix.  Using an old linux.ko with a patched kernel will
    crash the system.  HEADSUP?

>How-To-Repeat:

Don't. :)

>Fix:

Index: src/lib/libc/sys/execve.2
diff -u src/lib/libc/sys/execve.2:1.2 src/lib/libc/sys/execve.2:1.2.2.1
--- src/lib/libc/sys/execve.2:1.2	Fri Mar 12 23:41:05 2004
+++ src/lib/libc/sys/execve.2	Fri Mar 12 23:42:32 2004
@@ -225,7 +225,7 @@
 .It Bq Er ENAMETOOLONG
 When invoking an interpreted script, the interpreter name
 exceeds
-.Dv MAXSHELLCMDLEN
+.Dv PAGE_SIZE
 characters.
 .It Bq Er ENOENT
 The new process file does not exist.
Index: src/sys/alpha/linux/linux_sysvec.c
diff -u src/sys/alpha/linux/linux_sysvec.c:1.2 src/sys/alpha/linux/linux_sysvec.c:1.2.2.1
--- src/sys/alpha/linux/linux_sysvec.c:1.2	Fri Mar 12 23:50:09 2004
+++ src/sys/alpha/linux/linux_sysvec.c	Fri Mar 12 23:51:44 2004
@@ -159,7 +159,7 @@
 			if (rpath != imgp->interpreter_name) {
 				int len = strlen(rpath) + 1;

-				if (len <= MAXSHELLCMDLEN) {
+				if (len <= PAGE_SIZE) {
 					memcpy(imgp->interpreter_name, rpath,
 					    len);
 				}
Index: src/sys/i386/linux/linux_sysvec.c
diff -u src/sys/i386/linux/linux_sysvec.c:1.2 src/sys/i386/linux/linux_sysvec.c:1.2.2.1
--- src/sys/i386/linux/linux_sysvec.c:1.2	Fri Mar 12 20:32:28 2004
+++ src/sys/i386/linux/linux_sysvec.c	Fri Mar 12 23:09:25 2004
@@ -805,7 +805,7 @@
 		    if (rpath != imgp->interpreter_name) {
 			    int len = strlen(rpath) + 1;

-			    if (len <= MAXSHELLCMDLEN) {
+			    if (len <= PAGE_SIZE) {
 				    memcpy(imgp->interpreter_name, rpath, len);
 			    }
 			    free(rpath, M_TEMP);
Index: src/sys/kern/imgact_shell.c
diff -u src/sys/kern/imgact_shell.c:1.2 src/sys/kern/imgact_shell.c:1.2.2.4
--- src/sys/kern/imgact_shell.c:1.2	Wed Mar 10 22:25:12 2004
+++ src/sys/kern/imgact_shell.c	Sat Mar 13 00:12:36 2004
@@ -28,6 +28,8 @@
 __FBSDID("$FreeBSD: src/sys/kern/imgact_shell.c,v 1.26 2003/06/11 00:56:54 obrien Exp $");

 #include <sys/param.h>
+#include <sys/vnode.h>
+#include <sys/proc.h>
 #include <sys/systm.h>
 #include <sys/sysproto.h>
 #include <sys/exec.h>
@@ -50,7 +52,9 @@
 {
 	const char *image_header = imgp->image_header;
 	const char *ihp, *line_endp;
+	struct vattr vattr;
 	char *interp;
+	int error;

 	/* a shell script? */
 	if (((const short *) image_header)[0] != SHELLMAGIC)
@@ -68,14 +72,23 @@
 	/*
 	 * Copy shell name and arguments from image_header into string
 	 *	buffer.
+	 *
+	 * At this point we have the first page of the file mapped.
+	 * However, we don't know how far into the page the contents are
+	 * valid -- the actual file might be much shorter than the page.
+	 * So find out the file size.
 	 */
+	error = VOP_GETATTR(imgp->vp, &vattr, imgp->proc->p_ucred, curthread);
+
+	if (error)
+		return (error);

-	/*
-	 * Find end of line; return if the line > MAXSHELLCMDLEN long.
-	 */
 	for (ihp = &image_header[2]; *ihp != '\n' && *ihp != '#'; ++ihp) {
-		if (ihp >= &image_header[MAXSHELLCMDLEN])
-			return(ENAMETOOLONG);
+		if (ihp >= &image_header[PAGE_SIZE - 1])
+			return (ENAMETOOLONG);
+
+		if (ihp >= &image_header[vattr.va_size])
+			break;
 	}
 	line_endp = ihp;

Index: src/sys/kern/kern_exec.c
diff -u src/sys/kern/kern_exec.c:1.2 src/sys/kern/kern_exec.c:1.2.2.1
--- src/sys/kern/kern_exec.c:1.2	Fri Mar 12 19:13:32 2004
+++ src/sys/kern/kern_exec.c	Fri Mar 12 23:09:26 2004
@@ -288,7 +288,6 @@
 	imgp->entry_addr = 0;
 	imgp->vmspace_destroyed = 0;
 	imgp->interpreted = 0;
-	imgp->interpreter_name[0] = '\0';
 	imgp->auxargs = NULL;
 	imgp->vp = NULL;
 	imgp->object = NULL;
@@ -306,10 +305,13 @@

 	/*
 	 * Allocate temporary demand zeroed space for argument and
-	 *	environment strings
+	 *	environment strings.
+	 * ARG_MAX for argument and environment,
+	 * PAGE_SIZE for the first page of the image,
+	 * another PAGE_SIZE for the name of interpreters.
 	 */
 	imgp->stringbase = (char *)kmem_alloc_wait(exec_map, ARG_MAX +
-	    PAGE_SIZE);
+	    PAGE_SIZE * 2);
 	if (imgp->stringbase == NULL) {
 		error = ENOMEM;
 		mtx_lock(&Giant);
@@ -318,6 +320,7 @@
 	imgp->stringp = imgp->stringbase;
 	imgp->stringspace = ARG_MAX;
 	imgp->image_header = imgp->stringbase + ARG_MAX;
+	imgp->interpreter_name = imgp->stringbase + ARG_MAX + PAGE_SIZE;

 	/*
 	 * Translate the file name. namei() returns a vnode pointer
@@ -333,7 +336,7 @@
 	error = namei(ndp);
 	if (error) {
 		kmem_free_wakeup(exec_map, (vm_offset_t)imgp->stringbase,
-		    ARG_MAX + PAGE_SIZE);
+		    ARG_MAX + PAGE_SIZE * 2);
 		goto exec_fail;
 	}

@@ -704,7 +707,7 @@

 	if (imgp->stringbase != NULL)
 		kmem_free_wakeup(exec_map, (vm_offset_t)imgp->stringbase,
-		    ARG_MAX + PAGE_SIZE);
+		    ARG_MAX + PAGE_SIZE * 2);

 	if (imgp->object != NULL)
 		vm_object_deallocate(imgp->object);
Index: src/sys/sys/imgact.h
diff -u src/sys/sys/imgact.h:1.2 src/sys/sys/imgact.h:1.2.2.1
--- src/sys/sys/imgact.h:1.2	Fri Mar 12 23:19:03 2004
+++ src/sys/sys/imgact.h	Fri Mar 12 23:22:10 2004
@@ -36,8 +36,6 @@
 #ifndef _SYS_IMGACT_H_
 #define	_SYS_IMGACT_H_

-#define MAXSHELLCMDLEN	128
-
 struct label;
 struct sysentvec;
 struct thread;
@@ -61,7 +59,7 @@
 	unsigned long entry_addr; /* entry address of target executable */
 	char vmspace_destroyed;	/* flag - we've blown away original vm space */
 	char interpreted;	/* flag - this executable is interpreted */
-	char interpreter_name[MAXSHELLCMDLEN]; /* name of the interpreter */
+	char *interpreter_name; /* name of the interpreter */
 	void *auxargs;		/* ELF Auxinfo structure pointer */
 	struct vm_page *firstpage;	/* first page that we mapped */
 	char *fname;            /* pointer to filename of executable (user space) */
>Release-Note:
>Audit-Trail:
>Unformatted:



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040313070129.R680>