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