copyout() copies data to a va in a pagetable, for exec() &c

usertest that passes too many arguments, break exec

defs.h

@@ -165,6 +165,7 @@ int             loaduvm(pde_t*, char*, struct inode *, uint, uint);
 pde_t*          copyuvm(pde_t*,uint);
 void            switchuvm(struct proc*);
 void            switchkvm();
+int             copyout(pde_t *pgdir, uint va, void *buf, uint len);
 
 // number of elements in fixed-size array
 #define NELEM(x) (sizeof(x)/sizeof((x)[0]))

exec.c

@@ -9,16 +9,13 @@
 int
 exec(char *path, char **argv)
 {
-  char *mem, *s, *last;
-  int i, argc, arglen, len, off;
-  uint sz, sp, spbottom, argp;
+  char *s, *last;
+  int i, off;
+  uint sz = 0;
   struct elfhdr elf;
-  struct inode *ip;
+  struct inode *ip = 0;
   struct proghdr ph;
-  pde_t *pgdir, *oldpgdir;
-
-  pgdir = 0;
-  sz = 0;
+  pde_t *pgdir = 0, *oldpgdir;
 
   if((ip = namei(path)) == 0)
     return -1;
@@ -48,40 +45,65 @@ exec(char *path, char **argv)
   }
   iunlockput(ip);
 
-  // Allocate and initialize stack at sz
-  sz = spbottom = PGROUNDUP(sz);
+  // Allocate a one-page stack at the next page boundary
+  sz = PGROUNDUP(sz);
   if(!(sz = allocuvm(pgdir, sz, sz + PGSIZE)))
     goto bad;
-  mem = uva2ka(pgdir, (char *)spbottom);
 
-  arglen = 0;
-  for(argc=0; argv[argc]; argc++)
-    arglen += strlen(argv[argc]) + 1;
-  arglen = (arglen+3) & ~3;
+  // initialize stack content:
 
-  sp = sz;
-  argp = sz - arglen - 4*(argc+1);
+  // "argumentN"                      -- nul-terminated string
+  // ...
+  // "argument0"
+  // 0                                -- argv[argc]
+  // address of argumentN             
+  // ...
+  // address of argument0             -- argv[0]
+  // address of address of argument0  -- argv argument to main()
+  // argc                             -- argc argument to main()
+  // ffffffff                         -- return PC for main() call
 
-  // XXX rtm: does the following code work if the
-  // arguments &c do not fit in one page?
+  uint sp = sz;
 
-  // Copy argv strings and pointers to stack.
-  *(uint*)(mem+argp-spbottom + 4*argc) = 0;  // argv[argc]
-  for(i=argc-1; i>=0; i--){
-    len = strlen(argv[i]) + 1;
-    sp -= len;
-    memmove(mem+sp-spbottom, argv[i], len);
-    *(uint*)(mem+argp-spbottom + 4*i) = sp;  // argv[i]
+  // count arguments
+  int argc;
+  for(argc = 0; argv[argc]; argc++)
+    ;
+  if(argc >= MAXARG)
+    goto bad;
+
+  // push strings and remember where they are
+  uint strings[MAXARG];
+  for(i = argc - 1; i >= 0; --i){
+    sp -= strlen(argv[i]) + 1;
+    strings[i] = sp;
+    copyout(pgdir, sp, argv[i], strlen(argv[i]) + 1);
   }
 
-  // Stack frame for main(argc, argv), below arguments.
-  sp = argp;
+  // push 0 for argv[argc]
   sp -= 4;
-  *(uint*)(mem+sp-spbottom) = argp;
+  int zero = 0;
+  copyout(pgdir, sp, &zero, 4);
+
+  // push argv[] elements
+  for(i = argc - 1; i >= 0; --i){
+    sp -= 4;
+    copyout(pgdir, sp, &strings[i], 4);
+  }
+
+  // push argv
+  uint argvaddr = sp;
   sp -= 4;
-  *(uint*)(mem+sp-spbottom) = argc;
+  copyout(pgdir, sp, &argvaddr, 4);
+
+  // push argc
   sp -= 4;
-  *(uint*)(mem+sp-spbottom) = 0xffffffff;   // fake return pc
+  copyout(pgdir, sp, &argc, 4);
+
+  // push 0 in case main returns
+  sp -= 4;
+  uint ffffffff = 0xffffffff;
+  copyout(pgdir, sp, &ffffffff, 4);
 
   // Save program name for debugging.
   for(last=s=path; *s; s++)
@@ -103,6 +125,7 @@ exec(char *path, char **argv)
   return 0;
 
  bad:
+  cprintf("kernel: exec failed\n");
   if(pgdir) freevm(pgdir);
   iunlockput(ip);
   return -1;

param.h

@@ -7,4 +7,6 @@
 #define NINODE       50  // maximum number of active i-nodes
 #define NDEV         10  // maximum major device number
 #define ROOTDEV       1  // device number of file system root disk
+#define USERTOP  0xA0000 // end of user address space
 #define PHYSTOP  0x1000000 // use phys mem up to here as free pool
+#define MAXARG       32  // max exec arguments

sysfile.c

@@ -344,7 +344,7 @@ sys_chdir(void)
 int
 sys_exec(void)
 {
-  char *path, *argv[20];
+  char *path, *argv[MAXARG];
   int i;
   uint uargv, uarg;
 

usertests.c

@@ -1445,11 +1445,11 @@ bigargtest(void)
   ppid = getpid();
   pid = fork();
   if(pid == 0){
-    char *args[100];
+    char *args[32];
     int i;
-    for(i = 0; i < 99; i++)
-      args[i] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
-    args[99] = 0;
+    for(i = 0; i < 32-1; i++)
+      args[i] = "bigargs test: failed\n                                                                                                                     ";
+    args[32-1] = 0;
     printf(stdout, "bigarg test\n");
     exec("echo", args);
     printf(stdout, "bigarg test ok\n");
@@ -1472,7 +1472,7 @@ main(int argc, char *argv[])
   }
   close(open("usertests.ran", O_CREATE));
 
-  // bigargtest();
+  bigargtest();
   bsstest();
   sbrktest();
   validatetest();

vm.c

@@ -6,8 +6,6 @@
 #include "proc.h"
 #include "elf.h"
 
-#define USERTOP  0xA0000
-
 static pde_t *kpgdir;  // for use in scheduler()
 
 // Set up CPU's kernel segment descriptors.
@@ -126,7 +124,7 @@ setupkvm(void)
 {
   pde_t *pgdir;
   extern char etext[];
-  char *rwstart = PGROUNDDOWN(etext) - PGSIZE;
+  char *rwstart = PGROUNDDOWN(etext);
   uint rwlen = (uint)rwstart - 0x100000;
 
   // Allocate page directory
@@ -193,7 +191,10 @@ char*
 uva2ka(pde_t *pgdir, char *uva)
 {    
   pte_t *pte = walkpgdir(pgdir, uva, 0);
-  if(pte == 0) return 0;
+  if((*pte & PTE_P) == 0)
+    return 0;
+  if((*pte & PTE_U) == 0)
+    return 0;
   uint pa = PTE_ADDR(*pte);
   return (char *)pa;
 }
@@ -326,3 +327,26 @@ bad:
   return 0;
 }
 
+// copy some data to user address va in page table pgdir.
+// most useful when pgdir is not the current page table.
+// returns 1 if everthing OK, 0 on error.
+// uva2ka ensures this only works for PTE_U pages.
+int
+copyout(pde_t *pgdir, uint va, void *xbuf, uint len)
+{
+  char *buf = (char *) xbuf;
+  while(len > 0){
+    uint va0 = (uint)PGROUNDDOWN(va);
+    char *pa0 = uva2ka(pgdir, (char*) va0);
+    if(pa0 == 0)
+      return 0;
+    uint n = PGSIZE - (va - va0);
+    if(n > len)
+      n = len;
+    memmove(pa0 + (va - va0), buf, n);
+    len -= n;
+    buf += n;
+    va = va0 + PGSIZE;
+  }
+  return 1;
+}