From 5efca9054f1911e206831e16c2ca5ac8c8fc7c12 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Wed, 1 Sep 2010 00:32:27 -0400 Subject: Tab police --- exec.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index 4f11695..39530aa 100644 --- a/exec.c +++ b/exec.c @@ -30,7 +30,7 @@ exec(char *path, char **argv) if(elf.magic != ELF_MAGIC) goto bad; - if (!(pgdir = setupkvm())) + if(!(pgdir = setupkvm())) goto bad; // Load program into memory. @@ -41,11 +41,11 @@ exec(char *path, char **argv) continue; if(ph.memsz < ph.filesz) goto bad; - if (!allocuvm(pgdir, (char *)ph.va, ph.memsz)) + if(!allocuvm(pgdir, (char *)ph.va, ph.memsz)) goto bad; if(ph.va + ph.memsz > sz) sz = ph.va + ph.memsz; - if (!loaduvm(pgdir, (char *)ph.va, ip, ph.offset, ph.filesz)) + if(!loaduvm(pgdir, (char *)ph.va, ip, ph.offset, ph.filesz)) goto bad; } iunlockput(ip); @@ -53,7 +53,7 @@ exec(char *path, char **argv) // Allocate and initialize stack at sz sz = PGROUNDUP(sz); sz += PGSIZE; // leave an invalid page - if (!allocuvm(pgdir, (char *)sz, PGSIZE)) + if(!allocuvm(pgdir, (char *)sz, PGSIZE)) goto bad; mem = uva2ka(pgdir, (char *)sz); spoffset = sz; @@ -105,7 +105,7 @@ exec(char *path, char **argv) return 0; bad: - if (pgdir) freevm(pgdir); + if(pgdir) freevm(pgdir); iunlockput(ip); return -1; } -- cgit v1.2.3 From b1d41d678888fd1a51e4844ab583f7c47f9fb218 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Wed, 1 Sep 2010 16:46:37 -0400 Subject: Remove the stack guard page. Processes are now contiguous from 0 to proc->sz, which means our syscall argument validation is correct. Add a pointer validation test and remove the stack test, which tested for the guard page. --- exec.c | 1 - 1 file changed, 1 deletion(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index 39530aa..c518c04 100644 --- a/exec.c +++ b/exec.c @@ -52,7 +52,6 @@ exec(char *path, char **argv) // Allocate and initialize stack at sz sz = PGROUNDUP(sz); - sz += PGSIZE; // leave an invalid page if(!allocuvm(pgdir, (char *)sz, PGSIZE)) goto bad; mem = uva2ka(pgdir, (char *)sz); -- cgit v1.2.3 From 79cd8b3eedeb1f85d3b19fb6119bd5224c4c532a Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Thu, 2 Sep 2010 18:28:36 -0400 Subject: Simplify allocuvm/deallocuvm to operate in a contiguous memory model. This makes their interface match up better with proc->sz and also simplifies the callers (it even gets the main body of exec on one page). --- exec.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index c518c04..222f64c 100644 --- a/exec.c +++ b/exec.c @@ -11,7 +11,7 @@ exec(char *path, char **argv) { char *mem, *s, *last; int i, argc, arglen, len, off; - uint sz, sp, spoffset, argp; + uint sz, sp, spbottom, argp; struct elfhdr elf; struct inode *ip; struct proghdr ph; @@ -41,22 +41,18 @@ exec(char *path, char **argv) continue; if(ph.memsz < ph.filesz) goto bad; - if(!allocuvm(pgdir, (char *)ph.va, ph.memsz)) + if(!(sz = allocuvm(pgdir, sz, ph.va + ph.memsz))) goto bad; - if(ph.va + ph.memsz > sz) - sz = ph.va + ph.memsz; if(!loaduvm(pgdir, (char *)ph.va, ip, ph.offset, ph.filesz)) goto bad; } iunlockput(ip); // Allocate and initialize stack at sz - sz = PGROUNDUP(sz); - if(!allocuvm(pgdir, (char *)sz, PGSIZE)) + sz = spbottom = PGROUNDUP(sz); + if(!(sz = allocuvm(pgdir, sz, sz + PGSIZE))) goto bad; - mem = uva2ka(pgdir, (char *)sz); - spoffset = sz; - sz += PGSIZE; + mem = uva2ka(pgdir, (char *)spbottom); arglen = 0; for(argc=0; argv[argc]; argc++) @@ -67,22 +63,22 @@ exec(char *path, char **argv) argp = sz - arglen - 4*(argc+1); // Copy argv strings and pointers to stack. - *(uint*)(mem+argp-spoffset + 4*argc) = 0; // argv[argc] + *(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-spoffset, argv[i], len); - *(uint*)(mem+argp-spoffset + 4*i) = sp; // argv[i] + memmove(mem+sp-spbottom, argv[i], len); + *(uint*)(mem+argp-spbottom + 4*i) = sp; // argv[i] } // Stack frame for main(argc, argv), below arguments. sp = argp; sp -= 4; - *(uint*)(mem+sp-spoffset) = argp; + *(uint*)(mem+sp-spbottom) = argp; sp -= 4; - *(uint*)(mem+sp-spoffset) = argc; + *(uint*)(mem+sp-spbottom) = argc; sp -= 4; - *(uint*)(mem+sp-spoffset) = 0xffffffff; // fake return pc + *(uint*)(mem+sp-spbottom) = 0xffffffff; // fake return pc // Save program name for debugging. for(last=s=path; *s; s++) -- cgit v1.2.3 From 4587b35847b116057e3ef1058da914b8837ff343 Mon Sep 17 00:00:00 2001 From: Robert Morris Date: Sun, 19 Sep 2010 07:18:42 -0400 Subject: exec questions --- exec.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'exec.c') diff --git a/exec.c b/exec.c index 222f64c..a6de18f 100644 --- a/exec.c +++ b/exec.c @@ -48,6 +48,9 @@ exec(char *path, char **argv) } iunlockput(ip); + // XXX rtm: what about the BSS? shouldn't there be some + // bzero()ing here? + // Allocate and initialize stack at sz sz = spbottom = PGROUNDUP(sz); if(!(sz = allocuvm(pgdir, sz, sz + PGSIZE))) @@ -62,6 +65,9 @@ exec(char *path, char **argv) sp = sz; argp = sz - arglen - 4*(argc+1); + // XXX rtm: does the following code work if the + // arguments &c do not fit in one page? + // Copy argv strings and pointers to stack. *(uint*)(mem+argp-spbottom + 4*argc) = 0; // argv[argc] for(i=argc-1; i>=0; i--){ -- cgit v1.2.3 From 05d66b06294df89ba3d5b8f6cf535f7edf00bd1f Mon Sep 17 00:00:00 2001 From: Robert Morris Date: Sun, 19 Sep 2010 13:47:52 -0400 Subject: my comment is wrong, exec handles BSS fine --- exec.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index a6de18f..0a9ca59 100644 --- a/exec.c +++ b/exec.c @@ -48,9 +48,6 @@ exec(char *path, char **argv) } iunlockput(ip); - // XXX rtm: what about the BSS? shouldn't there be some - // bzero()ing here? - // Allocate and initialize stack at sz sz = spbottom = PGROUNDUP(sz); if(!(sz = allocuvm(pgdir, sz, sz + PGSIZE))) -- cgit v1.2.3 From 4655d42e3b65f906eae8c815fb78331790f6e423 Mon Sep 17 00:00:00 2001 From: Robert Morris Date: Mon, 27 Sep 2010 16:14:33 -0400 Subject: copyout() copies data to a va in a pagetable, for exec() &c usertest that passes too many arguments, break exec --- exec.c | 93 +++++++++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 58 insertions(+), 35 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index 0a9ca59..2e2ced4 100644 --- a/exec.c +++ b/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; - - sp = sz; - argp = sz - arglen - 4*(argc+1); - - // XXX rtm: does the following code work if the - // arguments &c do not fit in one page? - - // 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] + + // initialize stack content: + + // "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 + + uint sp = sz; + + // 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); + } + + // push 0 for argv[argc] + sp -= 4; + 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); } - // Stack frame for main(argc, argv), below arguments. - sp = argp; + // push argv + uint argvaddr = sp; sp -= 4; - *(uint*)(mem+sp-spbottom) = argp; + copyout(pgdir, sp, &argvaddr, 4); + + // push argc sp -= 4; - *(uint*)(mem+sp-spbottom) = argc; + copyout(pgdir, sp, &argc, 4); + + // push 0 in case main returns sp -= 4; - *(uint*)(mem+sp-spbottom) = 0xffffffff; // fake return pc + 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; -- cgit v1.2.3 From 06feabeceeccc8dbd2658e9f10dd139c14f01ba6 Mon Sep 17 00:00:00 2001 From: Robert Morris Date: Mon, 27 Sep 2010 16:17:57 -0400 Subject: check exec() arg length fix double iunlockput --- exec.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index 2e2ced4..c0ea515 100644 --- a/exec.c +++ b/exec.c @@ -44,6 +44,7 @@ exec(char *path, char **argv) goto bad; } iunlockput(ip); + ip = 0; // Allocate a one-page stack at the next page boundary sz = PGROUNDUP(sz); @@ -105,6 +106,9 @@ exec(char *path, char **argv) uint ffffffff = 0xffffffff; copyout(pgdir, sp, &ffffffff, 4); + if(sp < sz - PGSIZE) + goto bad; + // Save program name for debugging. for(last=s=path; *s; s++) if(*s == '/') @@ -125,8 +129,9 @@ exec(char *path, char **argv) return 0; bad: - cprintf("kernel: exec failed\n"); - if(pgdir) freevm(pgdir); - iunlockput(ip); + if(pgdir) + freevm(pgdir); + if(ip) + iunlockput(ip); return -1; } -- cgit v1.2.3 From 2ea6c764c34d3025b5f3121a0919fda7d1eb9b01 Mon Sep 17 00:00:00 2001 From: Robert Morris Date: Wed, 29 Sep 2010 14:12:26 -0400 Subject: even more fabulous exec --- exec.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index c0ea515..1673a38 100644 --- a/exec.c +++ b/exec.c @@ -81,30 +81,19 @@ exec(char *path, char **argv) copyout(pgdir, sp, argv[i], strlen(argv[i]) + 1); } - // push 0 for argv[argc] - sp -= 4; - int zero = 0; - copyout(pgdir, sp, &zero, 4); +#define PUSH(x) { int xx = (int)(x); sp -= 4; copyout(pgdir, sp, &xx, 4); } + + PUSH(0); // argv[argc] is zero // push argv[] elements - for(i = argc - 1; i >= 0; --i){ - sp -= 4; - copyout(pgdir, sp, &strings[i], 4); - } + for(i = argc - 1; i >= 0; --i) + PUSH(strings[i]); - // push argv - uint argvaddr = sp; - sp -= 4; - copyout(pgdir, sp, &argvaddr, 4); + PUSH(sp); // argv - // push argc - sp -= 4; - copyout(pgdir, sp, &argc, 4); + PUSH(argc); - // push 0 in case main returns - sp -= 4; - uint ffffffff = 0xffffffff; - copyout(pgdir, sp, &ffffffff, 4); + PUSH(0xffffffff); // in case main tries to return if(sp < sz - PGSIZE) goto bad; -- cgit v1.2.3 From 1a81e38b17144624415d252a521fd5a06079d681 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 11 Jan 2011 13:01:13 -0500 Subject: make new code like old code Variable declarations at top of function, separate from initialization. Use == 0 instead of ! for checking pointers. Consistent spacing around {, *, casts. Declare 0-parameter functions as (void) not (). Integer valued functions return -1 on failure, 0 on success. --- exec.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index 1673a38..209bc79 100644 --- a/exec.c +++ b/exec.c @@ -10,16 +10,17 @@ int exec(char *path, char **argv) { char *s, *last; - int i, off; - uint sz = 0; + int i, off, argc; + uint sz, sp, strings[MAXARG]; struct elfhdr elf; - struct inode *ip = 0; + struct inode *ip; struct proghdr ph; - pde_t *pgdir = 0, *oldpgdir; + pde_t *pgdir, *oldpgdir; if((ip = namei(path)) == 0) return -1; ilock(ip); + pgdir = 0; // Check ELF header if(readi(ip, (char*)&elf, 0, sizeof(elf)) < sizeof(elf)) @@ -27,10 +28,11 @@ exec(char *path, char **argv) if(elf.magic != ELF_MAGIC) goto bad; - if(!(pgdir = setupkvm())) + if((pgdir = setupkvm()) == 0) goto bad; // Load program into memory. + sz = 0; for(i=0, off=elf.phoff; i= 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); } -#define PUSH(x) { int xx = (int)(x); sp -= 4; copyout(pgdir, sp, &xx, 4); } +#define PUSH(x){ int xx = (int)(x); sp -= 4; copyout(pgdir, sp, &xx, 4); } PUSH(0); // argv[argc] is zero -- cgit v1.2.3 From cf4b1ad90bcaeeb0c8458098c87948f61d408f94 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Sat, 19 Feb 2011 21:17:55 -0500 Subject: xv6: formatting, cleanup, rev5 (take 2) --- exec.c | 62 ++++++++++++++++++-------------------------------------------- 1 file changed, 18 insertions(+), 44 deletions(-) (limited to 'exec.c') diff --git a/exec.c b/exec.c index 209bc79..05f80f8 100644 --- a/exec.c +++ b/exec.c @@ -10,8 +10,8 @@ int exec(char *path, char **argv) { char *s, *last; - int i, off, argc; - uint sz, sp, strings[MAXARG]; + int i, off; + uint argc, sz, sp, ustack[3+MAXARG+1]; struct elfhdr elf; struct inode *ip; struct proghdr ph; @@ -53,49 +53,25 @@ exec(char *path, char **argv) if((sz = allocuvm(pgdir, sz, sz + PGSIZE)) == 0) goto bad; - // initialize stack content: - - // "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 - + // Push argument strings, prepare rest of stack in ustack. sp = sz; - - // count arguments - for(argc = 0; argv[argc]; argc++) - ; - if(argc >= MAXARG) - goto bad; - - // push strings and remember where they are - for(i = argc - 1; i >= 0; --i){ - sp -= strlen(argv[i]) + 1; - strings[i] = sp; - copyout(pgdir, sp, argv[i], strlen(argv[i]) + 1); + for(argc = 0; argv[argc]; argc++) { + if(argc >= MAXARG) + goto bad; + sp -= strlen(argv[argc]) + 1; + sp &= ~3; + if(copyout(pgdir, sp, argv[argc], strlen(argv[argc]) + 1) < 0) + goto bad; + ustack[3+argc] = sp; } + ustack[3+argc] = 0; -#define PUSH(x){ int xx = (int)(x); sp -= 4; copyout(pgdir, sp, &xx, 4); } + ustack[0] = 0xffffffff; // fake return PC + ustack[1] = argc; + ustack[2] = sp - (argc+1)*4; // argv pointer - PUSH(0); // argv[argc] is zero - - // push argv[] elements - for(i = argc - 1; i >= 0; --i) - PUSH(strings[i]); - - PUSH(sp); // argv - - PUSH(argc); - - PUSH(0xffffffff); // in case main tries to return - - if(sp < sz - PGSIZE) + sp -= (3+argc+1) * 4; + if(copyout(pgdir, sp, ustack, (3+argc+1)*4) < 0) goto bad; // Save program name for debugging. @@ -110,9 +86,7 @@ exec(char *path, char **argv) proc->sz = sz; proc->tf->eip = elf.entry; // main proc->tf->esp = sp; - - switchuvm(proc); - + switchuvm(proc); freevm(oldpgdir); return 0; -- cgit v1.2.3