Bug in #! processing - One More Time

Maxim Sobolev sobomax at FreeBSD.org
Sat May 14 09:41:59 PDT 2005


Great, I've catched one bug in my patch using this set of tests - 
whitespaces have not being trimmed from the end of the arguments line. 
Just in case attached is the updated patch.

-Maxim

Garance A Drosihn wrote:
> At 9:09 PM +0300 5/13/05, Maxim Sobolev wrote:
> 
>> Garance A Drosihn wrote:
>>
>>>
>>> I should have done enough testing by Sunday evening to say
>>> something, one way or another.  Sure.
>>
>>
>> Good, since this issue has been taking too much time to fix.
> 
> 
> I'll admit to being guilty on that.
> 
>>> Note that I'm not just "running this through buildworld".  That's
>>> how all the previous changes were tested, too.  I have a whole
>>> battery of tests that I've been slogging through.
>>
>>
>> Well, I'd suggest you to put those tests into src/tools/regression,
>> to ensure that this won't be broken occasionally in the future.
> 
> 
> Well, there's some simply tests in ~gad/shellargs on the freefall.org
> machines.  Copy the directory, 'make /tmp/shellargs', and then
> 'make run/tests'.  These aren't quite in the right format for the
> standard regression-testing ideas, but they're in the ballpark.  The
> main idea was to give me a way to check what *other* operating systems
> actually do with whatever is on the #!-line.  The 'results' directory
> has the results from several hosts around here (@RPI).  Linux, Solaris,
> AIX, IRIX.
> 
> I also have a userland program written for testing imgact_shell.c
> itself, but that is a bit more bizarre.  Still, it's what I felt I
> needed so I could do a lot of testing of perverse examples, without
> having to rebuild kernels and rebooting every 5 minutes.  It also
> lets me test with triggering panics which kill the system, as had
> happened in some previous updates to imgact_shell.  Not sure that
> program would make any sense in regression testing.  Maybe if it was
> cleaned up a bit.
> 
> Anyway, back to my testing...
> 

-------------- next part --------------
Index: kern/imgact_shell.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/imgact_shell.c,v
retrieving revision 1.32
diff -d -u -d -u -r1.32 imgact_shell.c
--- kern/imgact_shell.c	25 Feb 2005 10:17:53 -0000	1.32
+++ kern/imgact_shell.c	14 May 2005 15:48:05 -0000
@@ -42,6 +42,48 @@
 #define SHELLMAGIC	0x2321
 #endif
 
+static inline const char *
+find_token_end(const char *bcp, const char *ecp)
+{
+
+	for (; bcp < ecp; bcp++)
+		if ((*bcp == '\0') || (*bcp == '\n') || (*bcp == ' ') ||
+		    (*bcp == '\t'))
+			break;
+	return (bcp);
+}
+
+static inline const char *
+find_whitespace_end(const char *bcp, const char *ecp)
+{
+
+	for (; bcp < ecp; bcp++)
+		if ((*bcp == '\0') || (*bcp == '\n') || ((*bcp != ' ') &&
+		    (*bcp != '\t')))
+			break;
+	return (bcp);
+}
+
+static inline const char *
+find_line_end(const char *bcp, const char *ecp)
+{
+
+	for (; bcp < ecp; bcp++)
+		if ((*bcp == '\0') || (*bcp == '\n'))
+			break;
+	return (bcp);
+}
+
+static inline const char *
+find_whitespace_begin(const char *bcp, const char *ecp)
+{
+
+	for (; ecp > bcp; ecp--)
+		if ((ecp[-1] != ' ') && (ecp[-1] != '\t'))
+			break;
+	return (ecp);
+}
+
 /*
  * Shell interpreter image activator. An interpreter name beginning
  *	at imgp->args->begin_argv is the minimal successful exit requirement.
@@ -50,14 +92,16 @@
 exec_shell_imgact(imgp)
 	struct image_params *imgp;
 {
-	const char *image_header = imgp->image_header;
-	const char *ihp;
-	int error, offset;
-	size_t length, clength;
+	const char *bcp, *ecp;
+	int error, alength;
+	size_t rlength;
 	struct vattr vattr;
+	const char *argv[2];
+	int argv_len[2];
 
+	bcp = imgp->image_header;
 	/* a shell script? */
-	if (((const short *) image_header)[0] != SHELLMAGIC)
+	if (((const short *)bcp)[0] != SHELLMAGIC)
 		return(-1);
 
 	/*
@@ -65,7 +109,7 @@
 	 *	script. :-)
 	 */
 	if (imgp->interpreted)
-		return(ENOEXEC);
+		return (ENOEXEC);
 
 	imgp->interpreted = 1;
 
@@ -78,68 +122,53 @@
 	error = VOP_GETATTR(imgp->vp, &vattr, imgp->proc->p_ucred, curthread);
 	if (error)
 		return (error);
+	if (vattr.va_size < 3)
+		return (ENOEXEC);
 
-	clength = (vattr.va_size > MAXSHELLCMDLEN) ?
-	    MAXSHELLCMDLEN : vattr.va_size;
+	ecp = bcp + vattr.va_size;
 	/*
-	 * Figure out the number of bytes that need to be reserved in the
+	 * Parse the first line of the shell script which is necessary to
+	 * figure out the number of bytes that need to be reserved in the
 	 * argument string to copy the contents of the interpreter's command
 	 * line into the argument string.
 	 */
-	ihp = &image_header[2];
-	offset = 0;
-	while (ihp < &image_header[clength]) {
-		/* Skip any whitespace */
-		if ((*ihp == ' ') || (*ihp == '\t')) {
-			ihp++;
-			continue;
-		}
-
-		/* End of line? */
-		if ((*ihp == '\n') || (*ihp == '#') || (*ihp == '\0'))
-			break;
-
-		/* Found a token */
-		do {
-			offset++;
-			ihp++;
-		} while ((*ihp != ' ') && (*ihp != '\t') && (*ihp != '\n') &&
-		    (*ihp != '#') && (*ihp != '\0') &&
-		    (ihp < &image_header[clength]));
-		/* Include terminating nulls in the offset */
-		offset++;
-	}
-
+	argv[0] = find_whitespace_end(bcp + 2, ecp);
+	argv_len[0] = find_token_end(argv[0], ecp) - argv[0];
+	/* Check that the name of interpreter is in allowed range */
+	if (argv_len[0] > MAXSHELLCMDLEN)
+		return (ENAMETOOLONG);
 	/* If the script gives a null line as the interpreter, we bail */
-	if (offset == 0)
+	if (argv_len[0] == 0)
 		return (ENOEXEC);
-
-	/* Check that we aren't too big */
-	if (ihp == &image_header[MAXSHELLCMDLEN])
-		return (ENAMETOOLONG);
+	argv[1] = find_whitespace_end(argv[0] + argv_len[0], ecp);
+	argv_len[1] = find_whitespace_begin(argv[1],
+	    find_line_end(argv[1], ecp)) - argv[1];
 
 	/*
 	 * The full path name of the original script file must be tagged
 	 * onto the end, adjust the offset to deal with it.
 	 *
-	 * The original argv[0] is being replaced, set 'length' to the number
-	 * of bytes being removed.  So 'offset' is the number of bytes being
-	 * added and 'length' is the number of bytes being removed.
+	 * The original argv[0] is being replaced, set 'rlength' to the number
+	 * of bytes being removed.  So 'alength' is the number of bytes being
+	 * added and 'rlength' is the number of bytes being removed.
 	 */
-	offset += strlen(imgp->args->fname) + 1;	/* add fname */
-	length = (imgp->args->argc == 0) ? 0 :
+	alength = argv_len[0] + argv_len[1] +
+	    strlen(imgp->args->fname) + 1;		/* add fname */
+	alength += (argv_len[1] == 0) ? 1 : 2;	/* compensate for NULs */
+	rlength = (imgp->args->argc == 0) ? 0 :
 	    strlen(imgp->args->begin_argv) + 1;		/* bytes to delete */
 
-	if (offset - length > imgp->args->stringspace)
+	if (alength - rlength > imgp->args->stringspace)
 		return (E2BIG);
 
-	bcopy(imgp->args->begin_argv + length, imgp->args->begin_argv + offset,
-	    imgp->args->endp - (imgp->args->begin_argv + length));
+	/* Move the rest of the arguments */
+	bcopy(imgp->args->begin_argv + rlength,
+	    imgp->args->begin_argv + alength,
+	    imgp->args->endp - (imgp->args->begin_argv + rlength));
 
-	offset -= length;		/* calculate actual adjustment */
-	imgp->args->begin_envv += offset;
-	imgp->args->endp += offset;
-	imgp->args->stringspace -= offset;
+	imgp->args->begin_envv += alength - rlength;
+	imgp->args->endp += alength - rlength;
+	imgp->args->stringspace -= alength - rlength;
 
 	/*
 	 * If there were no arguments then we've added one, otherwise
@@ -148,44 +177,29 @@
 	if (imgp->args->argc == 0)
 		imgp->args->argc = 1;
 
-	/*
-	 * Loop through the interpreter name yet again, copying as
-	 * we go.
-	 */
-	ihp = &image_header[2];
-	offset = 0;
-	while (ihp < &image_header[clength]) {
-		/* Skip whitespace */
-		if ((*ihp == ' ') || (*ihp == '\t')) {
-			ihp++;
-			continue;
-		}
-
-		/* End of line? */
-		if ((*ihp == '\n') || (*ihp == '#') || (*ihp == '\0'))
-			break;
-
-		/* Found a token, copy it */
-		do {
-			imgp->args->begin_argv[offset++] = *ihp++;
-		} while ((*ihp != ' ') && (*ihp != '\t') && (*ihp != '\n') &&
-		    (*ihp != '#') && (*ihp != '\0') &&
-		    (ihp < &image_header[MAXSHELLCMDLEN]));
-		imgp->args->begin_argv[offset++] = '\0';
+	/* Fill argv[0] and optionally argv[1] */
+	bcopy(argv[0], imgp->args->begin_argv, argv_len[0]);
+	imgp->args->begin_argv[argv_len[0]] = '\0';
+	imgp->args->argc++;
+	if (argv_len[1] != 0) {
+		bcopy(argv[1], imgp->args->begin_argv + argv_len[0] + 1,
+		    argv_len[1]);
+		imgp->args->begin_argv[argv_len[0] + 1 + argv_len[1]] = '\0';
 		imgp->args->argc++;
 	}
 
+	/* Add the filename as argv[1] or argv[2] for the interpreter to use */
+	error = copystr(imgp->args->fname, imgp->args->begin_argv +
+	    argv_len[0] + argv_len[1] + ((argv_len[1] == 0) ? 1 : 2),
+	    imgp->args->stringspace, &rlength);
+
 	/*
-	 * Finally, add the filename onto the end for the interpreter to
-	 * use and copy the interpreter's name to imgp->interpreter_name
-	 * for exec to use.
+	 * Finally copy the interpreter's name including terminating
+	 * NUL to imgp->interpreter_name for exec to use.
 	 */
-	error = copystr(imgp->args->fname, imgp->args->buf + offset,
-	    imgp->args->stringspace, &length);
-
 	if (error == 0)
-		error = copystr(imgp->args->begin_argv, imgp->interpreter_name,
-		    MAXSHELLCMDLEN, &length);
+		bcopy(imgp->args->begin_argv, imgp->interpreter_name,
+		    argv_len[0] + 1);
 
 	return (error);
 }


More information about the freebsd-arch mailing list