From owner-freebsd-arch@FreeBSD.ORG Sat May 14 16:41:59 2005 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 601B916A4CE; Sat, 14 May 2005 16:41:59 +0000 (GMT) Received: from www.portaone.com (support.portaone.com [195.70.151.35]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1D02D43D72; Sat, 14 May 2005 16:41:58 +0000 (GMT) (envelope-from sobomax@FreeBSD.org) Received: from [192.168.1.144] (039sub204.uottawa.ca [137.122.39.154]) (authenticated bits=0) by www.portaone.com (8.12.11/8.12.11) with ESMTP id j4EFrlkh094987 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 14 May 2005 17:53:49 +0200 (CEST) (envelope-from sobomax@FreeBSD.org) Message-ID: <42861F04.4060004@FreeBSD.org> Date: Sat, 14 May 2005 18:53:40 +0300 From: Maxim Sobolev User-Agent: Mozilla Thunderbird 1.0.2 (Windows/20050317) X-Accept-Language: en-us, en MIME-Version: 1.0 To: Garance A Drosihn References: <200410020349.i923nG8v021675@northstar.hetzel.org> <20041002052856.GE17792@nexus.dglawrence.com> <20041002233542.GL714@nexus.dglawrence.com> <421DAD8F.6000704@portaone.com> <4284D038.50805@FreeBSD.org> <4284D4BC.9070705@FreeBSD.org> <4284ED54.9010603@FreeBSD.org> In-Reply-To: Content-Type: multipart/mixed; boundary="------------000408060608050105030907" X-Virus-Scanned: ClamAV 0.83/876/Fri May 13 01:14:29 2005 on www.portaone.com X-Virus-Status: Clean cc: Paul Saab cc: freebsd-arch@FreeBSD.org Subject: Re: Bug in #! processing - One More Time X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 14 May 2005 16:41:59 -0000 This is a multi-part message in MIME format. --------------000408060608050105030907 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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... > --------------000408060608050105030907 Content-Type: text/plain; name="diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="diff" 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); } --------------000408060608050105030907--