Re: Looks like split of execve(2) syscall created bugs

[Maxim Sobolev <sobomax@xxxxxxxxxxxx>]

On Sat, Jan 29, 2005 at 12:49:09PM -0800, Matthew Dillon wrote:
> :>     You'll have to be more specific about case (2).  What in the codebase
> :>     are you refering to, file and line ?
> :
> :Trunk as of several hours ago, sys/kern/kern_exec.c function 
> :exec_copyin_args() around line 700. The code there fetches pointer to 
> :argv[0] from userspace, checks if it's NULL and puts first argument 
> :instead of it. Then it increases userspace pointer by one and fetches 
> :the next pointer *unconditionally*, so that in the case when argv[0] is 
> :NULL you may get some invalid (e.g. junk but non-NULL pointer) and get 
> :EFAULT for no reason. The same code ignores argv being NULL - see my 
> :follow-up. FreeBSD code in this case explicitly returns EFAULT.
> :
> :-Maxim
> 
>     That looks pretty straightforward.  The code has changed very little
>     from FreeBSD.  I'll do the first part of the cleanup but for the moment
>     I think we should leave the argv[0] NULL check in the common code rather
>     then just having it in the script code.

I can't agree. It's much easier to handle it in the
imgact_shell - it only adds 2 new lines of the code.
Additional benefit will be that the code is the same
in FreeBSD and DF after that (you are probably don't
care, but anyway ;), since it's what I am going to
commit in few minutes.

-Maxim

Index: imgact_shell.c
===================================================================
RCS file: /home/dcvs/src/sys/kern/imgact_shell.c,v
retrieving revision 1.4
diff -d -u -r1.4 imgact_shell.c
--- imgact_shell.c      16 Nov 2003 02:37:39 -0000      1.4
+++ imgact_shell.c      29 Jan 2005 22:03:42 -0000
@@ -109,7 +109,8 @@
	 * added and 'length' is the number of bytes being removed.
	 */
	offset += strlen(imgp->args->fname) + 1;        /* add fname */
-       length = strlen(imgp->args->begin_argv) + 1;    /* bytes to delete */
+       length = (imgp->args->argc == 0) ? 0 :
+           strlen(imgp->args->begin_argv) + 1;         /* bytes to delete */

	if (offset - length > imgp->args->space)
		return (E2BIG);
@@ -121,7 +122,13 @@
	imgp->args->begin_envv += offset;
	imgp->args->endp += offset;
	imgp->args->space -= offset;
-       /* decr argc remove old argv[0], incr argc for fname add, net 0 */
+
+       /*
+        * If there were no arguments then we've added one, otherwise
+        * decr argc remove old argv[0], incr argc for fname add, net 0
+        */
+       if (imgp->args->argc == 0)
+               imgp->args->argc = 1;

	/*
	 * Loop through the interpreter name yet again, copying as