From 6a1d10a19b767b0cbb0ff6f5a04118860b2e7597 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 13:28:55 +0000 Subject: [PATCH 1/9] Refactor synch to follow PintOS curly braces indentation style in if statements --- src/threads/synch.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/threads/synch.c b/src/threads/synch.c index 65c5bde..622e015 100644 --- a/src/threads/synch.c +++ b/src/threads/synch.c @@ -119,14 +119,14 @@ sema_up (struct semaphore *sema) old_level = intr_disable (); if (!list_empty (&sema->waiters)) - { - /* Enforces wake-up of the highest priority thread waiting for the - semaphore. */ - struct list_elem *e = list_max (&sema->waiters, priority_less, NULL); - list_remove (e); - thread_unblock (list_entry (e, struct thread, elem)); - thread_unblocked = true; - } + { + /* Enforces wake-up of the highest priority thread waiting for the + semaphore. */ + struct list_elem *e = list_max (&sema->waiters, priority_less, NULL); + list_remove (e); + thread_unblock (list_entry (e, struct thread, elem)); + thread_unblocked = true; + } sema->value++; intr_set_level (old_level); @@ -134,12 +134,12 @@ sema_up (struct semaphore *sema) priority that the current running thread, including the case when called within an interrupt handler. */ if (thread_unblocked) - { - if (intr_context ()) - intr_yield_on_return (); - else - thread_yield (); - } + { + if (intr_context ()) + intr_yield_on_return (); + else + thread_yield (); + } } static void sema_test_helper (void *sema_); From 1c757ecdfea7182203d0d107b096c65567dc1323 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 13:44:21 +0000 Subject: [PATCH 2/9] Update syscall to add more helpful comments for clarity and readability --- src/userprog/syscall.c | 50 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 29231f3..11dd5bd 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -142,6 +142,7 @@ syscall_exit (int status) static pid_t syscall_exec (const char *cmd_line) { + /* Validate the user string before executing the process. */ validate_user_string (cmd_line); return process_execute (cmd_line); /* Returns the PID of the new process */ @@ -161,12 +162,15 @@ syscall_wait (pid_t pid) static bool syscall_create (const char *file, unsigned initial_size) { + /* Validate the user string before creating the file. */ validate_user_string (file); + /* Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); bool status = filesys_create (file, initial_size); lock_release (&filesys_lock); + /* Return the status of the file creation. */ return status; } @@ -176,12 +180,15 @@ syscall_create (const char *file, unsigned initial_size) static bool syscall_remove (const char *file) { + /* Validate the user string before removing the file. */ validate_user_string (file); + /* Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); bool status = filesys_remove (file); lock_release (&filesys_lock); + /* Return the status of the file removal. */ return status; } @@ -192,11 +199,15 @@ syscall_remove (const char *file) static int syscall_open (const char *file) { + /* Validate the user string before opening the file. */ validate_user_string (file); + /* Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); struct file *ptr = filesys_open (file); lock_release (&filesys_lock); + + /* If the file could not be opened, return failure. */ if (ptr == NULL) return EXIT_FAILURE; @@ -206,6 +217,8 @@ syscall_open (const char *file) = (struct open_file*) malloc (sizeof (struct open_file)); if (file_info == NULL) { + /* If we could not allocate memory for the file_info struct, close the + file and return failure. */ file_close (ptr); return EXIT_FAILURE; } @@ -227,14 +240,17 @@ syscall_open (const char *file) static int syscall_filesize (int fd) { + /* Try to get the file from the FD. If it does not exist, return failure. */ struct open_file *file_info = fd_get_file (fd); if (file_info == NULL) return EXIT_FAILURE; + /* Acquire the file system lock to prevent any race conditions. */ lock_acquire (&filesys_lock); int bytes = file_length (file_info->file); lock_release (&filesys_lock); + /* Return the number of bytes in the file. */ return bytes; } @@ -247,9 +263,10 @@ 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) + if (fd < STDIN_FILENO || fd == STDOUT_FILENO) return EXIT_FAILURE; + /* Validate the user buffer for the provided size before reading. */ validate_user_pointer (buffer, size); if (fd == STDIN_FILENO) @@ -259,18 +276,24 @@ syscall_read (int fd, void *buffer, unsigned size) for (int i = 0; i < size; i++) write_buffer[i] = input_getc (); + /* In case of console, read is always (eventually) successful. So return + the size for the number of bytes read. */ return size; } else { /* Reading from a file. */ + /* Find the file from the FD. If it does not exist, return failure. */ struct open_file *file_info = fd_get_file (fd); if (file_info == NULL) return EXIT_FAILURE; + /* Acquire the file system lock to prevent race-conditions. */ lock_acquire (&filesys_lock); int bytes_written = file_read (file_info->file, buffer, size); lock_release (&filesys_lock); + + /* Return the number of bytes read. */ return bytes_written; } } @@ -287,25 +310,32 @@ syscall_write (int fd, const void *buffer, unsigned size) if (fd <= 0) return 0; + /* Validate the user buffer for the provided size before writing. */ validate_user_pointer (buffer, size); if (fd == STDOUT_FILENO) { /* Writing to the console. */ putbuf (buffer, size); + + /* In case of console, write is always successful. So return the size for + the number of bytes written. */ return size; } else { /* Writing to a file. */ + /* Find the file from the FD. If it does not exist, return failure. */ struct open_file *file_info = fd_get_file (fd); if (file_info == NULL) return 0; + /* Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); int bytes = file_write (file_info->file, buffer, size); lock_release (&filesys_lock); + /* Return the number of bytes written. */ return bytes; } } @@ -317,9 +347,11 @@ syscall_write (int fd, const void *buffer, unsigned size) static void syscall_seek (int fd, unsigned position) { + /* Find the file from the FD. If it does not exist, do nothing. */ struct open_file *file_info = fd_get_file (fd); if (file_info != NULL) { + /* File exists: Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); file_seek (file_info->file, position); lock_release (&filesys_lock); @@ -332,14 +364,17 @@ syscall_seek (int fd, unsigned position) static unsigned syscall_tell (int fd) { + /* Find the file from the FD. If it does not exist, return 0. */ struct open_file *file_info = fd_get_file (fd); if (file_info == NULL) return 0; + /* Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); unsigned pos = file_tell (file_info->file); lock_release (&filesys_lock); + /* Return the current position in the file. */ return pos; } @@ -349,14 +384,21 @@ syscall_tell (int fd) static void syscall_close (int fd) { + /* Find the file from the FD. If it does not exist, do nothing. */ struct open_file *file_info = fd_get_file (fd); if (file_info != NULL) { + /* File exists */ + /* First, remove the file from the hash table of open files. */ hash_delete (&thread_current ()->open_files, &file_info->elem); + /* Then, close the file, acquiring the file system lock to prevent race + conditions. */ lock_acquire (&filesys_lock); file_close (file_info->file); lock_release (&filesys_lock); + + /* Free the memory allocated for the file_info struct. */ free (file_info); } } @@ -421,11 +463,13 @@ fd_get_file (int fd) static void validate_user_pointer (const void *start, size_t size) { + /* If the size is 0, we do not need to check anything. */ if (size == 0) return; const void *end = start + size - 1; + /* Check if the start and end pointers are valid user virtual addresses. */ if (start == NULL || !is_user_vaddr (start) || !is_user_vaddr (end)) syscall_exit (EXIT_FAILURE); @@ -442,9 +486,11 @@ validate_user_pointer (const void *start, size_t size) static void validate_user_string (const char *str) { + /* Check if the string pointer is a valid user virtual address. */ if (str == NULL || !is_user_vaddr (str)) syscall_exit (EXIT_FAILURE); + /* Calculate the offset of the string within the (first) page. */ size_t offset = (uintptr_t) str % PGSIZE; /* We move page by page, checking if the page is mapped to physical memory. */ @@ -452,6 +498,8 @@ validate_user_string (const char *str) { void *page = pg_round_down (str); + /* If we reach addresses that are not mapped to physical memory before the + end of the string, the thread is terminated. */ if (!is_user_vaddr(page) || pagedir_get_page (thread_current ()->pagedir, page) == NULL) syscall_exit (EXIT_FAILURE); From 82d45880f72331df77d819dcd919f8756eaad5e1 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 13:49:58 +0000 Subject: [PATCH 3/9] Update validate_user_pointer to start from the beginning of the page rather than the given ptr --- src/userprog/syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 11dd5bd..1759bf1 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -475,7 +475,7 @@ validate_user_pointer (const void *start, size_t size) /* 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) + for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE) if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL) syscall_exit (EXIT_FAILURE); } From f4c900e56cc85273015ecc4cf562d4190a0ea087 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 14:37:38 +0000 Subject: [PATCH 4/9] Refactor process.c for comments, clarity and readability --- src/userprog/process.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index afc39f4..ad0bdec 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -84,9 +84,9 @@ process_execute (const char *cmd) /* Validates that the current file to be executed can be opened/exists. */ lock_acquire (&filesys_lock); - bool valid_file = filesys_open (file_name) != NULL; + struct file *file = filesys_open (file_name); lock_release (&filesys_lock); - if (!valid_file) + if (file == NULL) return TID_ERROR; /* Create a new thread to execute the command, by initializing @@ -128,7 +128,6 @@ static void start_process (void *proc_start_data) { struct intr_frame if_; - struct process_start_data *data = proc_start_data; /* Initialize interrupt frame and load executable. */ @@ -136,41 +135,46 @@ 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; - + + /* Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); - - /* Prevent writing to the file being executed. */ + struct file *exec_file = filesys_open (data->file_name); if (exec_file == NULL) { + /* If the executable file cannot be opened, free resources and quit. */ lock_release (&filesys_lock); sema_up (&data->loaded); thread_exit (); } + + /* Deny write to the executable file to prevent writing to it and release the + file system lock. */ file_deny_write (exec_file); lock_release (&filesys_lock); + thread_current ()->exec_file = exec_file; + /* Load the ELF executable file, and store the success of the operation in + the 'success' variable in data. */ data->success = load (data->file_name, &if_.eip, &if_.esp); - /* If load failed, quit. */ - if (!data->success) + /* If load was sucessful, initialize user process stack and free page used + to store the command that executed the process. */ + if (data->success) { - sema_up (&data->loaded); - thread_exit (); + data->success = + process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name); } - /* Initialize user process stack and free page used to store the - command that executed the process. */ - data->success = process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name); + /* Signal that the process has finished attempting to load. */ bool success = data->success; sema_up (&data->loaded); - /* If stack initialization failed, free resources and quit. */ + /* If the load was unsuccessful or if it was but the stack initialization + failed, exit the thread. */ if (!success) - { thread_exit (); - } /* Start the user process by simulating a return from an interrupt, implemented by intr_exit (in From 0f1bce2e88b0367020d2820e80b4fea55bcace9c Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 14:52:21 +0000 Subject: [PATCH 5/9] Refactor process_init_stack to add asserts and comments --- src/userprog/process.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index ad0bdec..02a5274 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -191,6 +191,10 @@ start_process (void *proc_start_data) static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name) { + ASSERT (cmd_saveptr != NULL); + ASSERT (esp != NULL); + ASSERT (file_name != NULL); + /* Load command line argument *data* to user process stack. This can't cause overflow due to enforcing that the size of command line input must fit in a page. Also keep track @@ -202,7 +206,10 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) int arg_count = 0; while (arg != NULL) { - push_to_stack (esp, arg, (strlen (arg) + 1) * sizeof (char)); + /* filename has already been validated to be a safe-to-access string, + so we can safely use strlen here. Filename has already been + split from the command line arguments. */ + push_to_stack (esp, arg, strlen (arg) + 1); struct arg_elem *arg_elem = malloc (sizeof (struct arg_elem)); if (arg_elem == NULL) From a7f1d519dad92de1133911bc230ba9ffde8ade41 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 15:09:24 +0000 Subject: [PATCH 6/9] Refactor process_wait to add more comments and improve readability --- src/userprog/process.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 02a5274..1e7b227 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -210,7 +210,8 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) so we can safely use strlen here. Filename has already been split from the command line arguments. */ push_to_stack (esp, arg, strlen (arg) + 1); - + + /* Try to allocate memory for the argument pointer. */ struct arg_elem *arg_elem = malloc (sizeof (struct arg_elem)); if (arg_elem == NULL) { @@ -219,9 +220,11 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) return false; } + /* Store the argument pointer in the linked list. */ arg_elem->arg = *esp; list_push_front (&arg_list, &arg_elem->elem); + /* Increment the argument count and get the next argument. */ arg_count++; arg = strtok_r (NULL, " ", &cmd_saveptr); } @@ -242,7 +245,10 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) int overflow_bytes = (PHYS_BASE - *esp) + remaining_size - PGSIZE; if (overflow_bytes > 0) { + /* Calculate the number of pages needed to allocate. */ int pages_needed = DIV_CEIL (overflow_bytes, PGSIZE); + + /* Allocate the pages and map them to the user process. */ for (int i = 1; i < pages_needed + 1; i++) { uint8_t *kpage = palloc_get_page (PAL_USER | PAL_ZERO); @@ -307,11 +313,12 @@ push_to_stack (void **esp, void *data, size_t data_size) * This function will be implemented in task 2. * For now, it does nothing. */ int -process_wait (tid_t child_tid UNUSED) +process_wait (tid_t child_tid) { struct process_result *child_result = NULL; struct list_elem *e; struct thread *cur = thread_current (); + for (e = list_begin (&cur->child_results); e != list_end (&cur->child_results); e = list_next (e)) { @@ -319,27 +326,40 @@ process_wait (tid_t child_tid UNUSED) = list_entry (e, struct process_result, elem); if (result->tid == child_tid) { + /* Found the child process. */ child_result = result; break; } - /* List is ordered, allowing us to break early. */ + + /* List is ordered, allowing us to break early if the child_tid is + greater than the current result's tid. */ else if (result->tid > child_tid) break; } + + /* If the child process was not found, return -1. */ if (child_result == NULL) return -1; + /* Wait for child to die. */ sema_down (&child_result->sema); + /* We need lock release in process_exit, so we need to acquire (and possibly wait) for it here to ensure we don't free the lock memory before it is released in process_exit. */ lock_acquire (&child_result->lock); + /* To prevent waiting for child twice, remove it from the list. No need to use lock since this is the only thread with access to the struct process_result now. */ list_remove (&child_result->elem); + + /* Get the exit status of the child */ int exit_status = child_result->exit_status; + + /* Release the lock */ lock_release (&child_result->lock); + free (child_result); return exit_status; } From 7daf4fb079a89e8292de4f7891de19da5c8c173e Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 15:35:07 +0000 Subject: [PATCH 7/9] Refactor process_exit to add more comments for readability --- src/userprog/process.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 1e7b227..e712e22 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -209,7 +209,7 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) /* filename has already been validated to be a safe-to-access string, so we can safely use strlen here. Filename has already been split from the command line arguments. */ - push_to_stack (esp, arg, strlen (arg) + 1); + push_to_stack (esp, arg, (strlen (arg) + 1) * sizeof (char)); /* Try to allocate memory for the argument pointer. */ struct arg_elem *arg_elem = malloc (sizeof (struct arg_elem)); @@ -376,9 +376,10 @@ process_exit (void) /* Clean up all open files */ hash_destroy (&cur->open_files, fd_cleanup); - /* Close the executable file. */ + /* Close the executable file, implicitly allowing it to be written to. */ if (cur->exec_file != NULL) { + /* Acquire the file system lock to prevent race conditions. */ lock_acquire (&filesys_lock); file_close (cur->exec_file); lock_release (&filesys_lock); From 6b1dbdd34fddd7e6429403a1f64326a4ae07d875 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 15:48:56 +0000 Subject: [PATCH 8/9] Update thread and syscall to use local fd counter instead global one, preventing overflow --- src/threads/thread.c | 1 + src/threads/thread.h | 7 ++++++- src/userprog/syscall.c | 4 +--- src/userprog/syscall.h | 2 -- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 74ac41e..1fcbc01 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -688,6 +688,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->recent_cpu = recent_cpu; t->priority = t->base_priority; + t->fd_counter = MINIMUM_USER_FD; t->exit_status = -1; list_init (&t->child_results); diff --git a/src/threads/thread.h b/src/threads/thread.h index 33a5485..c18500b 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -32,6 +32,9 @@ typedef int tid_t; #define NICE_DEFAULT 0 /* Default niceness. */ #define NICE_MAX 20 /* Highest niceness. */ +/* File Descriptors. */ +#define MINIMUM_USER_FD 2 /* Minimum file descriptor for user programs. */ + /* A process result, synchronised between parent and child. */ struct process_result { @@ -137,7 +140,9 @@ struct thread #ifdef USERPROG /* Owned by userprog/process.c. */ uint32_t *pagedir; /* Page directory. */ - struct hash open_files; /* Hash Table of FD -> Struct File */ + unsigned int fd_counter; /* File descriptor counter for thread's + open files. */ + struct hash open_files; /* Hash Table of FD -> Struct File. */ #endif /* Owned by thread.c. */ diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 1759bf1..015ebb7 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -16,8 +16,6 @@ #define MAX_SYSCALL_ARGS 3 #define EXIT_FAILURE -1 -static unsigned fd_counter = MIN_USER_FD; - struct open_file { int fd; /* File Descriptor / Identifier */ @@ -224,7 +222,7 @@ syscall_open (const char *file) } /* Populate the above struct, with a unique FD and the current open file */ - file_info->fd = fd_counter++; + file_info->fd = thread_current ()->fd_counter++; file_info->file = ptr; /* Add the new FD->file mapping to the hashtable for the current thread */ diff --git a/src/userprog/syscall.h b/src/userprog/syscall.h index 681a8fb..b1ec52d 100644 --- a/src/userprog/syscall.h +++ b/src/userprog/syscall.h @@ -4,8 +4,6 @@ #include #include "threads/synch.h" -#define MIN_USER_FD 2 - typedef int pid_t; struct lock filesys_lock; From ea3b3594ea9b04b3ddaf9a837f61cf51ae8bdeea Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 15 Nov 2024 15:53:01 +0000 Subject: [PATCH 9/9] Update fd_hash to use the fd itself as the hash value for performance, w/ G & E --- src/userprog/syscall.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 015ebb7..6beac7d 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -406,7 +406,12 @@ syscall_close (int fd) unsigned fd_hash (const struct hash_elem *element, void *aux UNUSED) { - return hash_int (hash_entry (element, struct open_file, elem)->fd); + /* We use the FD as the hash value. This is because the FD is incremented + sequentially and is therefore unique for each file. It positively affects + the performance of the hash table: 1. It is unique so no need to call + expensive hash functions. 2. It being sequential means that the hash table + is more likely to be weight balanced. */ + return hash_entry (element, struct open_file, elem)->fd; } /* Comparator function for the open_file table. Compares two entries based on