diff --git a/src/tests/userprog/Make.tests b/src/tests/userprog/Make.tests index 02c73fe..542ea0b 100644 --- a/src/tests/userprog/Make.tests +++ b/src/tests/userprog/Make.tests @@ -9,14 +9,14 @@ sc-bad-arg sc-bad-num sc-boundary sc-boundary-2 halt exit create-normal \ create-empty create-null create-bad-ptr create-long create-exists \ create-bound open-normal open-missing open-boundary open-empty \ open-null open-bad-ptr open-twice close-normal close-twice close-stdin \ -close-stdout close-bad-fd read-normal read-bad-ptr read-boundary \ -read-zero read-stdout read-bad-fd write-normal write-bad-ptr \ +close-stdout close-bad-fd read-normal read-bad-ptr read-bad-buf read-boundary \ +read-zero read-stdout read-bad-fd write-normal write-bad-ptr write-bad-buf \ write-boundary write-zero write-stdin write-bad-fd exec-once exec-arg \ exec-large-arg exec-multiple exec-missing exec-over-arg exec-over-args \ exec-bad-ptr wait-simple wait-twice wait-killed wait-load-kill \ wait-bad-pid wait-bad-child multi-recurse multi-child-fd rox-simple \ rox-child rox-multichild bad-read bad-write bad-read2 bad-write2 \ -bad-jump bad-jump2 bad-maths) +bad-jump bad-jump2 bad-maths overflow-stack) tests/userprog_PROGS = $(tests/userprog_TESTS) $(addprefix \ tests/userprog/,child-simple child-args child-bad child-close child-rox exec-exit) @@ -36,6 +36,7 @@ tests/userprog/bad-read2_SRC = tests/userprog/bad-read2.c tests/main.c tests/userprog/bad-write2_SRC = tests/userprog/bad-write2.c tests/main.c tests/userprog/bad-jump2_SRC = tests/userprog/bad-jump2.c tests/main.c tests/userprog/bad-maths_SRC = tests/userprog/bad-maths.c tests/main.c +tests/userprog/overflow-stack_SRC = tests/userprog/overflow-stack.c tests/main.c tests/userprog/sc-boundary_SRC = tests/userprog/sc-boundary.c \ tests/userprog/boundary.c tests/main.c tests/userprog/sc-boundary-2_SRC = tests/userprog/sc-boundary-2.c \ @@ -66,6 +67,7 @@ tests/userprog/close-stdout_SRC = tests/userprog/close-stdout.c tests/main.c tests/userprog/close-bad-fd_SRC = tests/userprog/close-bad-fd.c tests/main.c tests/userprog/read-normal_SRC = tests/userprog/read-normal.c tests/main.c tests/userprog/read-bad-ptr_SRC = tests/userprog/read-bad-ptr.c tests/main.c +tests/userprog/read-bad-buf_SRC = tests/userprog/read-bad-buf.c tests/main.c tests/userprog/read-boundary_SRC = tests/userprog/read-boundary.c \ tests/userprog/boundary.c tests/main.c tests/userprog/read-zero_SRC = tests/userprog/read-zero.c tests/main.c @@ -73,6 +75,7 @@ tests/userprog/read-stdout_SRC = tests/userprog/read-stdout.c tests/main.c tests/userprog/read-bad-fd_SRC = tests/userprog/read-bad-fd.c tests/main.c tests/userprog/write-normal_SRC = tests/userprog/write-normal.c tests/main.c tests/userprog/write-bad-ptr_SRC = tests/userprog/write-bad-ptr.c tests/main.c +tests/userprog/write-bad-buf_SRC = tests/userprog/write-bad-buf.c tests/main.c tests/userprog/write-boundary_SRC = tests/userprog/write-boundary.c \ tests/userprog/boundary.c tests/main.c tests/userprog/write-zero_SRC = tests/userprog/write-zero.c tests/main.c @@ -122,10 +125,12 @@ tests/userprog/close-normal_PUTFILES += tests/userprog/sample.txt tests/userprog/close-twice_PUTFILES += tests/userprog/sample.txt tests/userprog/read-normal_PUTFILES += tests/userprog/sample.txt tests/userprog/read-bad-ptr_PUTFILES += tests/userprog/sample.txt +tests/userprog/read-bad-buf_PUTFILES += tests/userprog/sample.txt tests/userprog/read-boundary_PUTFILES += tests/userprog/sample.txt tests/userprog/read-zero_PUTFILES += tests/userprog/sample.txt tests/userprog/write-normal_PUTFILES += tests/userprog/sample.txt tests/userprog/write-bad-ptr_PUTFILES += tests/userprog/sample.txt +tests/userprog/write-bad-buf_PUTFILES += tests/userprog/sample.txt tests/userprog/write-boundary_PUTFILES += tests/userprog/sample.txt tests/userprog/write-zero_PUTFILES += tests/userprog/sample.txt tests/userprog/multi-child-fd_PUTFILES += tests/userprog/sample.txt diff --git a/src/tests/userprog/Rubric.robustnessCR b/src/tests/userprog/Rubric.robustnessCR index 9784817..256caed 100644 --- a/src/tests/userprog/Rubric.robustnessCR +++ b/src/tests/userprog/Rubric.robustnessCR @@ -1,5 +1,9 @@ -Full robustness of argument passing code: -- Test user stack overflow robustness of "exec" system calls. +Full robustness of argument passing and syscall handling code: +- Test user stack overflow robustness of "exec" system calls and user code. 5 exec-over-arg 5 exec-over-args +5 overflow-stack +- Test syscall user provided buffer validity checks. +5 read-bad-buf +5 write-bad-buf diff --git a/src/tests/userprog/exec-bad-ptr.ck b/src/tests/userprog/exec-bad-ptr.ck index 63f5f78..7f2604c 100644 --- a/src/tests/userprog/exec-bad-ptr.ck +++ b/src/tests/userprog/exec-bad-ptr.ck @@ -2,11 +2,7 @@ use strict; use warnings; use tests::tests; -check_expected ([<<'EOF', <<'EOF']); -(exec-bad-ptr) begin -(exec-bad-ptr) end -exec-bad-ptr: exit(0) -EOF +check_expected ([<<'EOF']); (exec-bad-ptr) begin exec-bad-ptr: exit(-1) EOF diff --git a/src/tests/userprog/open-bad-ptr.ck b/src/tests/userprog/open-bad-ptr.ck index 45349e2..fa686d0 100644 --- a/src/tests/userprog/open-bad-ptr.ck +++ b/src/tests/userprog/open-bad-ptr.ck @@ -2,11 +2,7 @@ use strict; use warnings; use tests::tests; -check_expected ([<<'EOF', <<'EOF']); -(open-bad-ptr) begin -(open-bad-ptr) end -open-bad-ptr: exit(0) -EOF +check_expected ([<<'EOF']); (open-bad-ptr) begin open-bad-ptr: exit(-1) EOF diff --git a/src/tests/userprog/overflow-stack.c b/src/tests/userprog/overflow-stack.c new file mode 100644 index 0000000..d7c9c85 --- /dev/null +++ b/src/tests/userprog/overflow-stack.c @@ -0,0 +1,17 @@ +/* Attempt to overflow the user stack by allocating a 4kB buffer and writing into it. + The process must be terminated with -1 exit code until stack growth has been implemented in Task 3 +*/ + +#include +#include +#include "tests/lib.h" +#include "tests/main.h" + +void +test_main (void) +{ + char stack_obj[4096]; + memset (stack_obj, 'a', sizeof stack_obj); + memset (stack_obj+10, '\0', 1); + msg ("buffer: %s", stack_obj); +} diff --git a/src/tests/userprog/overflow-stack.ck b/src/tests/userprog/overflow-stack.ck new file mode 100644 index 0000000..8998230 --- /dev/null +++ b/src/tests/userprog/overflow-stack.ck @@ -0,0 +1,14 @@ +# -*- perl -*- +use strict; +use warnings; +use tests::tests; +check_expected (IGNORE_USER_FAULTS => 1, [<<'EOF',<<'EOF']); +(overflow-stack) begin +overflow-stack: exit(-1) +EOF +(overflow-stack) begin +(overflow-stack) buffer: aaaaaaaaaa +(overflow-stack) end +overflow-stack: exit(0) +EOF +pass; diff --git a/src/tests/userprog/read-bad-buf.c b/src/tests/userprog/read-bad-buf.c new file mode 100644 index 0000000..d2dc613 --- /dev/null +++ b/src/tests/userprog/read-bad-buf.c @@ -0,0 +1,17 @@ +/* Passes a buffer to the read system call that starts in valid memory, but runs into kernel space. + The process must be terminated with -1 exit code. +*/ + +#include +#include "tests/lib.h" +#include "tests/main.h" + +void +test_main (void) +{ + int handle; + CHECK ((handle = open ("sample.txt")) > 1, "open \"sample.txt\""); + + read (handle, (char *) 0xbfffffe0, 100); + fail ("should not have survived read()"); +} diff --git a/src/tests/userprog/read-bad-buf.ck b/src/tests/userprog/read-bad-buf.ck new file mode 100644 index 0000000..703b633 --- /dev/null +++ b/src/tests/userprog/read-bad-buf.ck @@ -0,0 +1,10 @@ +# -*- perl -*- +use strict; +use warnings; +use tests::tests; +check_expected (IGNORE_KERNEL_FAULTS => 1, [<<'EOF']); +(read-bad-buf) begin +(read-bad-buf) open "sample.txt" +read-bad-buf: exit(-1) +EOF +pass; diff --git a/src/tests/userprog/read-bad-ptr.ck b/src/tests/userprog/read-bad-ptr.ck index d10accf..abd67f7 100644 --- a/src/tests/userprog/read-bad-ptr.ck +++ b/src/tests/userprog/read-bad-ptr.ck @@ -2,12 +2,7 @@ use strict; use warnings; use tests::tests; -check_expected ([<<'EOF', <<'EOF']); -(read-bad-ptr) begin -(read-bad-ptr) open "sample.txt" -(read-bad-ptr) end -read-bad-ptr: exit(0) -EOF +check_expected ([<<'EOF']); (read-bad-ptr) begin (read-bad-ptr) open "sample.txt" read-bad-ptr: exit(-1) diff --git a/src/tests/userprog/write-bad-buf.c b/src/tests/userprog/write-bad-buf.c new file mode 100644 index 0000000..589499c --- /dev/null +++ b/src/tests/userprog/write-bad-buf.c @@ -0,0 +1,17 @@ +/* Passes a buffer to the write system call that starts in valid memory, but runs into kernel space. + The process must be terminated with -1 exit code. +*/ + +#include +#include "tests/lib.h" +#include "tests/main.h" + +void +test_main (void) +{ + int handle; + CHECK ((handle = open ("sample.txt")) > 1, "open \"sample.txt\""); + + write (handle, (char *) 0xbffffff0, 32); + fail ("should have exited with -1"); +} diff --git a/src/tests/userprog/write-bad-buf.ck b/src/tests/userprog/write-bad-buf.ck new file mode 100644 index 0000000..b0678cb --- /dev/null +++ b/src/tests/userprog/write-bad-buf.ck @@ -0,0 +1,10 @@ +# -*- perl -*- +use strict; +use warnings; +use tests::tests; +check_expected (IGNORE_KERNEL_FAULTS => 1, [<<'EOF']); +(write-bad-buf) begin +(write-bad-buf) open "sample.txt" +write-bad-buf: exit(-1) +EOF +pass; diff --git a/src/tests/userprog/write-bad-ptr.ck b/src/tests/userprog/write-bad-ptr.ck index ad9f399..3014879 100644 --- a/src/tests/userprog/write-bad-ptr.ck +++ b/src/tests/userprog/write-bad-ptr.ck @@ -2,12 +2,7 @@ use strict; use warnings; use tests::tests; -check_expected ([<<'EOF', <<'EOF']); -(write-bad-ptr) begin -(write-bad-ptr) open "sample.txt" -(write-bad-ptr) end -write-bad-ptr: exit(0) -EOF +check_expected ([<<'EOF']); (write-bad-ptr) begin (write-bad-ptr) open "sample.txt" write-bad-ptr: exit(-1) diff --git a/src/threads/synch.c b/src/threads/synch.c index c8bbd6d..65c5bde 100644 --- a/src/threads/synch.c +++ b/src/threads/synch.c @@ -212,6 +212,7 @@ donate_priority (struct thread *donee) { ASSERT (intr_get_level () == INTR_OFF); struct thread *donor = thread_current (); + list_remove (&donor->donor_elem); list_push_back (&donee->donors_list, &donor->donor_elem); while (donee != NULL) @@ -260,6 +261,7 @@ lock_acquire (struct lock *lock) ASSERT (!lock_held_by_current_thread (lock)); struct thread *t = thread_current (); + ASSERT (t->waiting_lock == NULL); enum intr_level old_level = intr_disable (); if (lock->holder != NULL) @@ -341,7 +343,6 @@ lock_release (struct lock *lock) released, transfer the remaining orphaned donors to its donor list. */ if (max_donor != NULL) { - list_remove (&max_donor->donor_elem); while (!list_empty (&orphan_list)) list_push_back (&max_donor->donors_list, list_pop_front (&orphan_list)); } diff --git a/src/threads/thread.c b/src/threads/thread.c index fd239ee..74ac41e 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -373,7 +373,9 @@ thread_exit (void) and schedule another process. That process will destroy us when it calls thread_schedule_tail(). */ intr_disable (); - list_remove (&thread_current()->allelem); + struct thread *t = thread_current (); + list_remove (&t->allelem); + list_remove (&t->donor_elem); thread_current ()->status = THREAD_DYING; schedule (); NOT_REACHED (); @@ -679,6 +681,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->base_priority = thread_mlfqs ? calculate_bsd_priority (recent_cpu, nice) : priority; list_init (&t->donors_list); + list_push_back (&t->donors_list, &t->donor_elem); t->waiting_lock = NULL; t->nice = nice; diff --git a/src/userprog/process.c b/src/userprog/process.c index 3d12bd1..e1200bc 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -8,6 +8,7 @@ #include #include "userprog/gdt.h" #include "userprog/pagedir.h" +#include "userprog/syscall.h" #include "userprog/tss.h" #include "filesys/directory.h" #include "filesys/file.h" @@ -46,6 +47,9 @@ struct process_start_data tokens while maintaining state. */ char file_name[FNAME_MAX_LEN + 1]; /* Name of the file of the process to be started. */ + bool success; /* Indicates whether the process was successfully loaded. */ + struct semaphore loaded; /* Semaphore used to signal that the process has + been loaded. */ }; static thread_func start_process NO_RETURN; @@ -66,6 +70,8 @@ process_execute (const char *cmd) { return TID_ERROR; } + sema_init (&data->loaded, 0); + data->success = false; /* Make a copy of command. Otherwise there's a race between the caller and load(). */ @@ -81,8 +87,14 @@ process_execute (const char *cmd) of the process. */ char *file_name = strtok_r (cmd_copy, " ", &data->cmd_saveptr); -/* Validates that the current file to be executed is a valid file */ -if (filesys_open (file_name) == NULL) + /* NOTE: Currently, the file being executed is closed in load () and then + reopened here. Because load is an exported public function, this + might be necessary. */ + lock_acquire (&filesys_lock); + /* Validates that the current file to be executed is a valid file */ + bool valid_file = filesys_open (file_name) != NULL; + lock_release (&filesys_lock); + if (!valid_file) return TID_ERROR; /* Create a new thread to execute the command, by initializing @@ -93,7 +105,14 @@ if (filesys_open (file_name) == NULL) tid = thread_create (file_name, PRI_DEFAULT, start_process, data); if (tid == TID_ERROR) - palloc_free_page (cmd_copy); + palloc_free_page (cmd_copy); + else + { + sema_down (&data->loaded); + if (!data->success) + tid = TID_ERROR; + } + free (data); return tid; } @@ -120,6 +139,20 @@ start_process (void *proc_start_data) if_.gs = if_.fs = if_.es = if_.ds = if_.ss = SEL_UDSEG; if_.cs = SEL_UCSEG; if_.eflags = FLAG_IF | FLAG_MBS; + + lock_acquire (&filesys_lock); + + /* Prevent writing to the file being executed. */ + struct file *exec_file = filesys_open (data->file_name); + if (exec_file == NULL) + { + lock_release (&filesys_lock); + goto fail; + } + thread_current ()->exec_file = exec_file; + file_deny_write (exec_file); + lock_release (&filesys_lock); + success = load (data->file_name, &if_.eip, &if_.esp); /* If load failed, quit. */ @@ -137,17 +170,11 @@ start_process (void *proc_start_data) /* If stack initialization failed, free resources and quit. */ if (!success) { - process_exit (); goto fail; } - /* NOTE: Currently, the file being executed is closed in load () and then - reopened here. Because load is an exported public function, this - might be necessary. */ - struct file *exec_file = filesys_open (data->file_name); - thread_current ()->exec_file = exec_file; - file_deny_write (exec_file); - + data->success = true; + sema_up (&data->loaded); /* Start the user process by simulating a return from an interrupt, implemented by intr_exit (in threads/intr-stubs.S). Because intr_exit takes all of its @@ -157,9 +184,10 @@ start_process (void *proc_start_data) asm volatile ("movl %0, %%esp; jmp intr_exit" : : "g" (&if_) : "memory"); NOT_REACHED (); -/* If starting the process failed, free its common resources and exit. */ - fail: - free (data); + /* If starting the process failed, exit. */ +fail: + data->success = false; + sema_up (&data->loaded); thread_exit (); } @@ -316,7 +344,17 @@ process_exit (void) uint32_t *pd; printf ("%s: exit(%d)\n", cur->name, cur->exit_status); - file_close (cur->exec_file); + + /* Clean up all open files */ + hash_destroy (&cur->open_files, fd_cleanup); + + /* Close the executable file. */ + if (cur->exec_file != NULL) + { + lock_acquire (&filesys_lock); + file_close (cur->exec_file); + lock_release (&filesys_lock); + } /* Update process result. */ if (cur->result != NULL) @@ -341,10 +379,11 @@ process_exit (void) /* Free child process results or signal parent's death. */ struct list_elem *e; for (e = list_begin (&cur->child_results); - e != list_end (&cur->child_results); e = list_next (e)) + e != list_end (&cur->child_results);) { struct process_result *result = list_entry (e, struct process_result, elem); + struct list_elem *next = list_next (e); lock_acquire (&result->lock); /* Child has died (and was not waited for). Free the result. */ if (sema_try_down (&result->sema)) @@ -358,6 +397,7 @@ process_exit (void) sema_up (&result->sema); lock_release (&result->lock); } + e = next; } /* Destroy the current process's page directory and switch back @@ -476,6 +516,7 @@ load (const char *file_name, void (**eip) (void), void **esp) off_t file_ofs; bool success = false; int i; + lock_acquire (&filesys_lock); /* Allocate and activate page directory. */ t->pagedir = pagedir_create (); @@ -575,6 +616,7 @@ load (const char *file_name, void (**eip) (void), void **esp) done: /* We arrive here whether the load is successful or not. */ file_close (file); + lock_release (&filesys_lock); return success; } diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index ccb02f3..29231f3 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -13,7 +13,9 @@ #include #include -static struct lock filesys_lock; +#define MAX_SYSCALL_ARGS 3 +#define EXIT_FAILURE -1 + static unsigned fd_counter = MIN_USER_FD; struct open_file @@ -46,18 +48,19 @@ static unsigned syscall_tell (int fd); static void syscall_close (int fd); static struct open_file *fd_get_file (int fd); -static void *validate_user_pointer (const void *ptr, size_t size); +static void validate_user_pointer (const void *start, size_t size); +static void validate_user_string (const char *str); /* A struct defining a syscall_function pointer along with its arity. */ -typedef struct +struct syscall_arguments { syscall_function function; /* Function pointer. */ int arity; /* Number of arguments of the function. */ - } syscall_arguments; + }; /* A look-up table mapping numbers to system call functions with their number of arguments. */ -static const syscall_arguments syscall_lookup[] = +static const struct syscall_arguments syscall_lookup[] = { [SYS_HALT] = {(syscall_function) syscall_halt, 0}, [SYS_EXIT] = {(syscall_function) syscall_exit, 1}, @@ -77,8 +80,7 @@ static const syscall_arguments syscall_lookup[] = /* The number of syscall functions (i.e, number of elements) within the syscall_lookup table. */ static const int LOOKUP_SIZE - = sizeof (syscall_lookup) / sizeof (syscall_arguments); - + = sizeof (syscall_lookup) / sizeof (struct syscall_arguments); /* Initialises the syscall handling system, as well as a global lock to synchronise all file access between processes. */ @@ -89,28 +91,29 @@ syscall_init (void) lock_init (&filesys_lock); } -/* Function that takes a interrupt frame containing a syscall and its args. +/* Function that takes an interrupt frame containing a syscall and its args. Validates the arguments and pointers before calling the relevant high-level system call function, storing its output (if any) in f->eax */ static void syscall_handler (struct intr_frame *f) { /* First, read the system call number from the stack. */ - validate_user_pointer (f->esp, 1); - unsigned syscall_number = *(int *) f->esp; + validate_user_pointer (f->esp, sizeof (uintptr_t)); + uintptr_t syscall_number = *(int *) f->esp; /* Ensures the number corresponds to a system call that can be handled. */ if (syscall_number >= LOOKUP_SIZE) - thread_exit (); + syscall_exit (EXIT_FAILURE); - syscall_arguments syscall = syscall_lookup[syscall_number]; + struct syscall_arguments syscall = syscall_lookup[syscall_number]; /* Next, read and copy the arguments from the stack pointer. */ validate_user_pointer (f->esp + sizeof (uintptr_t), syscall.arity * sizeof (uintptr_t)); - uintptr_t args[3] = {0}; - for (int i=0; i < syscall.arity; i++) - args[i] = *(uintptr_t *) (f->esp + sizeof (uintptr_t) * (i + 1)); + + uintptr_t args[MAX_SYSCALL_ARGS] = {0}; + for (int i = 0; i < syscall.arity && i < MAX_SYSCALL_ARGS; i++) + args[i] = *(uintptr_t *) (f->esp + sizeof (uintptr_t) * (i + 1)); /* Call the function that handles this system call with the arguments. When there is a return value it is stored in f->eax. */ @@ -135,19 +138,13 @@ syscall_exit (int status) } /* Executes a given command with the relevant args, by calling process_execute. - Acquires the filesystem lock as process_execute accesses the file system. - Returns PID for the process that is running the CMD_LINE - */ + Returns PID for the process that is running the CMD_LINE. */ static pid_t syscall_exec (const char *cmd_line) { - validate_user_pointer (cmd_line, 1); + validate_user_string (cmd_line); - lock_acquire (&filesys_lock); - pid_t pid = process_execute(cmd_line); - lock_release (&filesys_lock); - - return pid; + return process_execute (cmd_line); /* Returns the PID of the new process */ } /* Handles the syscall of wait. Effectively a wrapper for process_wait as the @@ -155,16 +152,16 @@ syscall_exec (const char *cmd_line) static int syscall_wait (pid_t pid) { - return process_wait (pid); + return process_wait (pid); /* Returns the exit status of the waited process */ } /* Handles the syscall for file creation. First validates the user file pointer. Acquires the file system lock to prevent synchronisation issues, and then uses FILESYS_CREATE to create the file, returning the same status */ static bool -syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) +syscall_create (const char *file, unsigned initial_size) { - validate_user_pointer (file, 1); + validate_user_string (file); lock_acquire (&filesys_lock); bool status = filesys_create (file, initial_size); @@ -179,7 +176,7 @@ syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) static bool syscall_remove (const char *file) { - validate_user_pointer (file, 1); + validate_user_string (file); lock_acquire (&filesys_lock); bool status = filesys_remove (file); @@ -195,20 +192,23 @@ syscall_remove (const char *file) static int syscall_open (const char *file) { - validate_user_pointer (file, 1); + validate_user_string (file); lock_acquire (&filesys_lock); struct file *ptr = filesys_open (file); lock_release (&filesys_lock); if (ptr == NULL) - return -1; + return EXIT_FAILURE; /* Allocate space for a struct representing a mapping from an FD to a struct file. */ struct open_file *file_info = (struct open_file*) malloc (sizeof (struct open_file)); if (file_info == NULL) - return -1; + { + file_close (ptr); + return EXIT_FAILURE; + } /* Populate the above struct, with a unique FD and the current open file */ file_info->fd = fd_counter++; @@ -229,7 +229,7 @@ syscall_filesize (int fd) { struct open_file *file_info = fd_get_file (fd); if (file_info == NULL) - return -1; + return EXIT_FAILURE; lock_acquire (&filesys_lock); int bytes = file_length (file_info->file); @@ -248,7 +248,7 @@ syscall_read (int fd, void *buffer, unsigned size) /* Only console (fd = 0) or other files, not including STDOUT, (fd > 1) are allowed. */ if (fd < 0 || fd == STDOUT_FILENO) - return -1; + return EXIT_FAILURE; validate_user_pointer (buffer, size); @@ -266,7 +266,7 @@ syscall_read (int fd, void *buffer, unsigned size) /* Reading from a file. */ struct open_file *file_info = fd_get_file (fd); if (file_info == NULL) - return -1; + return EXIT_FAILURE; lock_acquire (&filesys_lock); int bytes_written = file_read (file_info->file, buffer, size); @@ -381,6 +381,20 @@ fd_less (const struct hash_elem *a_, const struct hash_elem *b_, return a->fd < b->fd; } +/* Function to clean up an open file entry. Closes the file and frees the + associated memory. */ +void +fd_cleanup (struct hash_elem *e, void *aux UNUSED) +{ + struct open_file *file_info = hash_entry (e, struct open_file, elem); + + lock_acquire (&filesys_lock); + file_close (file_info->file); + lock_release (&filesys_lock); + + free (file_info); +} + /* Gets a file from its descriptor (FD number). If there is no file with the fd FD it returns NULL. */ static struct open_file * @@ -400,18 +414,57 @@ fd_get_file (int fd) return hash_entry (e, struct open_file, elem); } -/* Validates if a block of memory starting at PTR and of size SIZE bytes is - fully contained within user virtual memory. Kills the thread (by calling - thread_exit) if the memory is invalid. Otherwise, returns the PTR given. - If the size is 0, the function does no checks and returns PTR.*/ -static void * -validate_user_pointer (const void *ptr, size_t size) +/* Validates if a block of memory starting at START and of size SIZE bytes is + fully contained within user virtual memory. Kills the thread (by exiting with + failure) if the memory is invalid. Otherwise, returns (nothing) normally. + If the size is 0, the function does no checks and returns the given ptr. */ +static void +validate_user_pointer (const void *start, size_t size) { - if (size > 0 && (ptr == NULL || - !is_user_vaddr (ptr) || - !is_user_vaddr (ptr + size - 1) || - pagedir_get_page (thread_current()->pagedir, ptr) == NULL)) - thread_exit (); + if (size == 0) + return; - return (void *) ptr; + const void *end = start + size - 1; + + if (start == NULL || !is_user_vaddr (start) || !is_user_vaddr (end)) + syscall_exit (EXIT_FAILURE); + + /* We now need to check if the entire memory block is mapped to physical + memory by the page table. */ + for (const void *ptr = start; ptr <= end; ptr += PGSIZE) + if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL) + syscall_exit (EXIT_FAILURE); +} + +/* Validates if a string is fully contained within user virtual memory. Kills + the thread (by exiting with failure) if the memory is invalid. Otherwise, + returns (nothing) normally. */ +static void +validate_user_string (const char *str) +{ + if (str == NULL || !is_user_vaddr (str)) + syscall_exit (EXIT_FAILURE); + + size_t offset = (uintptr_t) str % PGSIZE; + + /* We move page by page, checking if the page is mapped to physical memory. */ + for (;;) + { + void *page = pg_round_down (str); + + if (!is_user_vaddr(page) || + pagedir_get_page (thread_current ()->pagedir, page) == NULL) + syscall_exit (EXIT_FAILURE); + + while (offset < PGSIZE) + { + if (*str == '\0') + return; /* We reached the end of the string without issues. */ + + str++; + offset++; + } + + offset = 0; /* Next page will start at the beginning. */ + } } diff --git a/src/userprog/syscall.h b/src/userprog/syscall.h index 0f9288b..681a8fb 100644 --- a/src/userprog/syscall.h +++ b/src/userprog/syscall.h @@ -2,14 +2,18 @@ #define USERPROG_SYSCALL_H #include +#include "threads/synch.h" #define MIN_USER_FD 2 typedef int pid_t; +struct lock filesys_lock; + void syscall_init (void); unsigned fd_hash (const struct hash_elem *element, void *aux); bool fd_less (const struct hash_elem *a, const struct hash_elem *b, void *aux); +void fd_cleanup (struct hash_elem *e, void *aux); #endif /* userprog/syscall.h */