From ddcd59fdf8a744cc2b19b1d43e78b300e67320b3 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 02:52:38 +0000 Subject: [PATCH 01/43] Add child and own process result information to struct thread --- src/threads/thread.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/threads/thread.h b/src/threads/thread.h index 1c05030..cf35b82 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -4,6 +4,8 @@ #include #include #include +#include +#include "threads/synch.h" #include "threads/fixed-point.h" /* States in a thread's life cycle. */ @@ -29,6 +31,19 @@ typedef int tid_t; #define NICE_DEFAULT 0 /* Default niceness. */ #define NICE_MAX 20 /* Highest niceness. */ +/* A process result, synchronised between parent and child. */ +struct process_result +{ + tid_t tid; /* The tid of the child process. */ + int exit_status; /* The exit status of the child process. Initially set to + -1, then to 0 when parent dies, or to exit_status when + child dies (whichever happens first). */ + struct lock lock; /* Lock to synchronise access to the exit_status. */ + struct semaphore sema; /* Semaphore to signal the parent that the exit_status + has been set. */ + struct list_elem elem; /* List element for the parent's children list. */ +}; + /* A kernel thread or user process. Each thread structure is stored in its own 4 kB page. The @@ -108,6 +123,11 @@ struct thread int nice; /* Nice value for this thread */ fp32_t recent_cpu; /* Amount of time this process received */ + /* Process wait properties. */ + struct process_result *result; /* Result of the process. */ + struct list child_results; /* List of children's of this thread + process results. */ + /* Shared between thread.c and synch.c. */ struct list_elem elem; /* List element. */ From ec8547aec9f145e5ead34b9f55a78d5ff238df47 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 03:31:48 +0000 Subject: [PATCH 02/43] Implement creation of process results --- src/threads/thread.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/threads/thread.c b/src/threads/thread.c index 457f7b9..ee76718 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -9,6 +9,7 @@ #include "threads/flags.h" #include "threads/interrupt.h" #include "threads/intr-stubs.h" +#include "threads/malloc.h" #include "threads/palloc.h" #include "threads/switch.h" #include "threads/synch.h" @@ -68,6 +69,7 @@ static void kernel_thread (thread_func *, void *aux); static void idle (void *aux UNUSED); static struct thread *running_thread (void); static struct thread *next_thread_to_run (void); +static void init_process_result (struct thread *t); static void init_thread (struct thread *, const char *name, int nice, int priority, fp32_t recent_cpu); static bool is_thread (struct thread *) UNUSED; @@ -110,6 +112,7 @@ thread_init (void) initial_thread_recent_cpu); initial_thread->status = THREAD_RUNNING; initial_thread->tid = allocate_tid (); + initial_thread->result = NULL; /* Main thread cannot be waited for. */ } /* Starts preemptive thread scheduling by enabling interrupts. @@ -236,6 +239,7 @@ thread_create (const char *name, int priority, struct thread *parent_thread = thread_current (); init_thread (t, name, parent_thread->nice, priority, parent_thread->recent_cpu); tid = t->tid = allocate_tid (); + init_process_result (t); /* Prepare thread for first run by initializing its stack. Do this atomically so intermediate values for the 'stack' @@ -259,6 +263,10 @@ thread_create (const char *name, int priority, intr_set_level (old_level); + /* No need to synchronise child_results since it is only ever accessed by one + thread. */ + list_insert (&parent_thread->child_results, &t->result->elem); + /* Add to run queue. */ thread_unblock (t); @@ -632,6 +640,18 @@ is_thread (struct thread *t) return t != NULL && t->magic == THREAD_MAGIC; } +/* Allocate and initialise a process result for given thread. */ +static void +init_process_result (struct thread *t) +{ + struct process_result *result = malloc (sizeof (struct process_result)); + result->tid = t->tid; + result->exit_status = t->exit_status; + lock_init (&result->lock); + sema_init (&result->sema, 0); + t->result = result; +} + /* Does basic initialization of T as a blocked thread named NAME. */ static void @@ -660,6 +680,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->priority = t->base_priority; t->exit_status = -1; + list_init (&t->child_results); old_level = intr_disable (); list_push_back (&all_list, &t->allelem); From 7349b4e66f3a4542d390a975fe41890d5b242d37 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 03:55:34 +0000 Subject: [PATCH 03/43] Fix typo list_insert -> list_push_back for thread.child_results --- src/threads/thread.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index ee76718..379894d 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -264,8 +264,8 @@ thread_create (const char *name, int priority, intr_set_level (old_level); /* No need to synchronise child_results since it is only ever accessed by one - thread. */ - list_insert (&parent_thread->child_results, &t->result->elem); + thread. By the nature of increasing TIDs, this list is ordered. */ + list_push_back (&parent_thread->child_results, &t->result->elem); /* Add to run queue. */ thread_unblock (t); From fde70dcf5934aa083c7513e47b316024ef3831d2 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 03:56:05 +0000 Subject: [PATCH 04/43] Implement process_wait. --- src/threads/thread.h | 5 ++- src/userprog/process.c | 76 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/src/threads/thread.h b/src/threads/thread.h index cf35b82..6465d53 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -36,9 +36,8 @@ struct process_result { tid_t tid; /* The tid of the child process. */ int exit_status; /* The exit status of the child process. Initially set to - -1, then to 0 when parent dies, or to exit_status when - child dies (whichever happens first). */ - struct lock lock; /* Lock to synchronise access to the exit_status. */ + -1, then to exit_status when child dies. */ + struct lock lock; /* Lock to synchronise access to the status and sema. */ struct semaphore sema; /* Semaphore to signal the parent that the exit_status has been set. */ struct list_elem elem; /* List element for the parent's children list. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index 6c8ef2d..5b41a34 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -1,6 +1,7 @@ #include "userprog/process.h" #include #include +#include #include #include #include @@ -202,12 +203,34 @@ start_process (void *file_name_) int process_wait (tid_t child_tid UNUSED) { - /* As a temporary wait, waiting will just put the thread to sleep for one - second (TIMER_FREQ = 100 ticks ~ 1 second). */ - /* TODO: Implement process_wait () correctly. Remove the next line. */ - timer_sleep (TIMER_FREQ); - - return 0; /* TODO: Change this too */ + 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)) + { + struct process_result *result + = list_entry (e, struct process_result, elem); + if (result->tid == child_tid) + { + child_result = result; + break; + } + /* List is ordered, allowing us to break early. */ + else if (result->tid > child_tid) + break; + } + if (child_result == NULL) + return -1; + /* Wait for child to die. */ + sema_down (&child_result->sema); + /* 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); + int exit_status = child_result->exit_status; + free (child_result); + return exit_status; } /* Free the current process's resources. */ @@ -219,6 +242,47 @@ process_exit (void) printf ("%s: exit(%d)\n", cur->name, cur->exit_status); + /* Update process result. */ + if (cur->result != NULL) + { + lock_acquire (&cur->result->lock); + cur->result->exit_status = cur->exit_status; + /* Parent has died, child has to free the struct process_result * */ + if (sema_try_down (&cur->result->sema)) + { + lock_release (&cur->result->lock); + free (cur->result); + } + /* Parent is still alive and will be the one to free the + struct process_result *, and may be waiting so call sema_up */ + else + { + sema_up (&cur->result->sema); + lock_release (&cur->result->lock); + } + } + + struct list_elem *e; + for (e = list_begin (&cur->child_results); + e != list_end (&cur->child_results); e = list_next (e)) + { + struct process_result *result + = list_entry (e, struct process_result, elem); + lock_acquire (&result->lock); + /* Child has died (and was not waited for). Free the result. */ + if (sema_try_down (&result->sema)) + { + lock_release (&result->lock); + free (result); + } + /* Child is still alive, signal via sema that parent has died. */ + else + { + sema_up (&result->sema); + lock_release (&result->lock); + } + } + /* Destroy the current process's page directory and switch back to the kernel-only page directory. */ pd = cur->pagedir; From d95894085b0b0d9305561fde7510df262ea21102 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 09:15:22 +0000 Subject: [PATCH 05/43] Implement syscall_exec via process_execute --- src/userprog/syscall.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 1dbe73b..03e0721 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -110,10 +110,14 @@ syscall_exit (int status) } static pid_t -syscall_exec (const char *cmd_line UNUSED) +syscall_exec (const char *cmd_line) { - //TODO - return 0; + /* To check the end we would need to traverse the null-terminate string, + which is equally unsafe as just leaving process_execute to do it. */ + cmd_line = validate_user_pointer (cmd_line, 1); + if (cmd_line == NULL) + thread_exit (); + process_execute (cmd_line); } static int From 84fe637c7e284dd8dab7e2a7ce036857900e1926 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 09:16:18 +0000 Subject: [PATCH 06/43] Remove process_result lock since it is an invalid solution TODO : synchronise process_result in another way --- src/threads/thread.c | 1 - src/threads/thread.h | 1 - src/userprog/process.c | 22 ++++------------------ 3 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 379894d..aecafdf 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -647,7 +647,6 @@ init_process_result (struct thread *t) struct process_result *result = malloc (sizeof (struct process_result)); result->tid = t->tid; result->exit_status = t->exit_status; - lock_init (&result->lock); sema_init (&result->sema, 0); t->result = result; } diff --git a/src/threads/thread.h b/src/threads/thread.h index 6465d53..f6227f5 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -37,7 +37,6 @@ struct process_result tid_t tid; /* The tid of the child process. */ int exit_status; /* The exit status of the child process. Initially set to -1, then to exit_status when child dies. */ - struct lock lock; /* Lock to synchronise access to the status and sema. */ struct semaphore sema; /* Semaphore to signal the parent that the exit_status has been set. */ struct list_elem elem; /* List element for the parent's children list. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index 5b41a34..1db334a 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -245,21 +245,14 @@ process_exit (void) /* Update process result. */ if (cur->result != NULL) { - lock_acquire (&cur->result->lock); cur->result->exit_status = cur->exit_status; /* Parent has died, child has to free the struct process_result * */ if (sema_try_down (&cur->result->sema)) - { - lock_release (&cur->result->lock); - free (cur->result); - } + free (cur->result); /* Parent is still alive and will be the one to free the struct process_result *, and may be waiting so call sema_up */ else - { - sema_up (&cur->result->sema); - lock_release (&cur->result->lock); - } + sema_up (&cur->result->sema); } struct list_elem *e; @@ -268,19 +261,12 @@ process_exit (void) { struct process_result *result = list_entry (e, struct process_result, elem); - lock_acquire (&result->lock); /* Child has died (and was not waited for). Free the result. */ if (sema_try_down (&result->sema)) - { - lock_release (&result->lock); - free (result); - } + free (result); /* Child is still alive, signal via sema that parent has died. */ else - { - sema_up (&result->sema); - lock_release (&result->lock); - } + sema_up (&result->sema); } /* Destroy the current process's page directory and switch back From 6ed1ccd21ea012ffb37d8c8d47e87b5f90979fc8 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 10:50:10 +0000 Subject: [PATCH 07/43] Fix process_result locking by acquiring in process_wait as well to prevent freeing memory too early --- src/threads/thread.c | 1 + src/threads/thread.h | 7 ++++--- src/userprog/process.c | 27 +++++++++++++++++++++++---- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index aecafdf..379894d 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -647,6 +647,7 @@ init_process_result (struct thread *t) struct process_result *result = malloc (sizeof (struct process_result)); result->tid = t->tid; result->exit_status = t->exit_status; + lock_init (&result->lock); sema_init (&result->sema, 0); t->result = result; } diff --git a/src/threads/thread.h b/src/threads/thread.h index f6227f5..2947256 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -34,9 +34,10 @@ typedef int tid_t; /* A process result, synchronised between parent and child. */ struct process_result { - tid_t tid; /* The tid of the child process. */ - int exit_status; /* The exit status of the child process. Initially set to - -1, then to exit_status when child dies. */ + tid_t tid; /* The tid of the child process. */ + int exit_status; /* The exit status of the child process. Initially set + to -1, then to exit_status when child dies. */ + struct lock lock; /* Lock the exit_status and sema. */ struct semaphore sema; /* Semaphore to signal the parent that the exit_status has been set. */ struct list_elem elem; /* List element for the parent's children list. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index 1db334a..370a216 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -224,11 +224,16 @@ process_wait (tid_t child_tid UNUSED) 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); int exit_status = child_result->exit_status; + lock_release (&child_result->lock); free (child_result); return exit_status; } @@ -245,14 +250,21 @@ process_exit (void) /* Update process result. */ if (cur->result != NULL) { + lock_acquire (&cur->result->lock); cur->result->exit_status = cur->exit_status; /* Parent has died, child has to free the struct process_result * */ if (sema_try_down (&cur->result->sema)) - free (cur->result); + { + lock_release (&cur->result->lock); + free (cur->result); + } /* Parent is still alive and will be the one to free the struct process_result *, and may be waiting so call sema_up */ else - sema_up (&cur->result->sema); + { + sema_up (&cur->result->sema); + lock_release (&cur->result->lock); + } } struct list_elem *e; @@ -261,12 +273,19 @@ process_exit (void) { struct process_result *result = list_entry (e, struct process_result, elem); + lock_acquire (&result->lock); /* Child has died (and was not waited for). Free the result. */ if (sema_try_down (&result->sema)) - free (result); + { + lock_release (&result->lock); + free (result); + } /* Child is still alive, signal via sema that parent has died. */ else - sema_up (&result->sema); + { + sema_up (&result->sema); + lock_release (&result->lock); + } } /* Destroy the current process's page directory and switch back From 26e38be761a6efe6395cbaace21f0ea8f101f88a Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 14:21:16 +0000 Subject: [PATCH 08/43] Update validate_user_pointer to check if the ptr is mapped to a physical memory address, w/ E --- src/userprog/syscall.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 1dbe73b..506498b 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -5,6 +5,7 @@ #include "threads/interrupt.h" #include "threads/thread.h" #include "userprog/process.h" +#include "userprog/pagedir.h" #include #include @@ -227,8 +228,9 @@ validate_user_pointer (const void *ptr, size_t size) { if (size > 0 && (ptr == NULL || !is_user_vaddr (ptr) || - !is_user_vaddr (ptr + size - 1))) + !is_user_vaddr (ptr + size - 1) || + pagedir_get_page (thread_current()->pagedir, ptr) == NULL)) thread_exit (); - return ptr; + return (void *) ptr; } From a8676f2e099e14c2eae2e9353d31d59891899cb9 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 8 Nov 2024 14:34:08 +0000 Subject: [PATCH 09/43] Implement syscall for file creation, with relevant locks w/ S. --- src/userprog/syscall.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 506498b..04d763f 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -1,14 +1,18 @@ #include "userprog/syscall.h" #include "devices/shutdown.h" #include "devices/input.h" +#include "filesys/filesys.h" #include "threads/vaddr.h" #include "threads/interrupt.h" #include "threads/thread.h" +#include "threads/synch.h" #include "userprog/process.h" #include "userprog/pagedir.h" #include #include +static struct lock filesys_lock; + static void syscall_handler (struct intr_frame *); /* A syscall_function is a function that receives up to 3 arguments, the @@ -68,6 +72,7 @@ void syscall_init (void) { intr_register_int (0x30, 3, INTR_ON, syscall_handler, "syscall"); + lock_init (&filesys_lock); } static void @@ -126,8 +131,13 @@ syscall_wait (pid_t pid) static bool syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) { - //TODO - return 0; + validate_user_pointer (file, 1); + + lock_acquire (&filesys_lock); + bool status = filesys_create (file, initial_size); + lock_release (&filesys_lock); + + return status; } static bool From dca9d8f5a33dd662a00f3beef305c187e271fbad Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 8 Nov 2024 14:35:23 +0000 Subject: [PATCH 10/43] Implement syscall for file removal w/ S. --- src/userprog/syscall.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 04d763f..cf353cc 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -141,10 +141,15 @@ syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) } static bool -syscall_remove (const char *file UNUSED) +syscall_remove (const char *file) { - //TODO - return 0; + validate_user_pointer (file, 1); + + lock_acquire (&filesys_lock); + bool status = filesys_remove (file); + lock_release (&filesys_lock); + + return status; } static int From b112824a6412760de7637db6f303099fd5ccad07 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 14:41:27 +0000 Subject: [PATCH 11/43] Implement the exec system call through process_execute, w/ E --- src/userprog/syscall.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index cf353cc..98c9dd4 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -116,10 +116,15 @@ syscall_exit (int status) } static pid_t -syscall_exec (const char *cmd_line UNUSED) +syscall_exec (const char *cmd_line) { - //TODO - return 0; + validate_user_pointer (cmd_line, 1); + + lock_acquire (&filesys_lock); + pid_t pid = process_execute(cmd_line); + lock_release (&filesys_lock); + + return pid; } static int From 5bbe7a03c08b236e20631c0ec05f303338d273c3 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 15:12:08 +0000 Subject: [PATCH 12/43] Add in syscall hash helper functions for open_file struct: fd_hash and fd_less, w/ E --- src/userprog/syscall.c | 23 +++++++++++++++++++++++ src/userprog/syscall.h | 5 +++++ 2 files changed, 28 insertions(+) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 98c9dd4..555b940 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -13,6 +13,13 @@ static struct lock filesys_lock; +struct open_file + { + int fd; /* File Descriptor / Identifier */ + struct file *file; /* Pointer to the associated file */ + struct hash_elem elem; /* elem for a hash table */ + }; + static void syscall_handler (struct intr_frame *); /* A syscall_function is a function that receives up to 3 arguments, the @@ -239,6 +246,22 @@ syscall_close (int fd UNUSED) //TODO } +unsigned +fd_hash (const struct hash_elem *element, void *aux UNUSED) +{ + return hash_int (hash_entry (element, struct open_file, elem)->fd); +} + +bool +fd_less (const struct hash_elem *a_, const struct hash_elem *b_, + void *aux UNUSED) +{ + struct open_file *a = hash_entry (a_, struct open_file, elem); + struct open_file *b = hash_entry (b_, struct open_file, elem); + + return a->fd < b->fd; +} + /* 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. diff --git a/src/userprog/syscall.h b/src/userprog/syscall.h index 702a6c7..9563059 100644 --- a/src/userprog/syscall.h +++ b/src/userprog/syscall.h @@ -1,8 +1,13 @@ #ifndef USERPROG_SYSCALL_H #define USERPROG_SYSCALL_H +#include + typedef int pid_t; 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); + #endif /* userprog/syscall.h */ From 5bd94894e051ff421f2badd26ae0ebddd47f2f95 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 15:13:04 +0000 Subject: [PATCH 13/43] Update thread structure to add a hash table of open files and initialise it, w/ E --- src/threads/thread.c | 3 +++ src/threads/thread.h | 2 ++ 2 files changed, 5 insertions(+) diff --git a/src/threads/thread.c b/src/threads/thread.c index 457f7b9..04380dd 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -1,5 +1,6 @@ #include "threads/thread.h" #include +#include #include #include #include @@ -15,6 +16,7 @@ #include "threads/vaddr.h" #ifdef USERPROG #include "userprog/process.h" +#include "userprog/syscall.h" #endif /* Random value for struct thread's `magic' member. @@ -660,6 +662,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->priority = t->base_priority; t->exit_status = -1; + hash_init (&t->open_files, fd_hash, fd_less, NULL); old_level = intr_disable (); list_push_back (&all_list, &t->allelem); diff --git a/src/threads/thread.h b/src/threads/thread.h index 1c05030..d17b4d7 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -2,6 +2,7 @@ #define THREADS_THREAD_H #include +#include #include #include #include "threads/fixed-point.h" @@ -116,6 +117,7 @@ struct thread #ifdef USERPROG /* Owned by userprog/process.c. */ uint32_t *pagedir; /* Page directory. */ + struct hash open_files; /* Hash Table of FD -> Struct File */ #endif /* Owned by thread.c. */ From 92e93b80608a45359aa2735b15549d3791a5a92e Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 8 Nov 2024 15:33:47 +0000 Subject: [PATCH 14/43] Implement syscall for file opening and refactor open_files initialisation in thread.c w/ S. --- src/threads/thread.c | 2 +- src/userprog/syscall.c | 29 ++++++++++++++++++++++++----- src/userprog/syscall.h | 2 ++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 04380dd..037f309 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -238,6 +238,7 @@ thread_create (const char *name, int priority, struct thread *parent_thread = thread_current (); init_thread (t, name, parent_thread->nice, priority, parent_thread->recent_cpu); tid = t->tid = allocate_tid (); + hash_init (&t->open_files, fd_hash, fd_less, NULL); /* Prepare thread for first run by initializing its stack. Do this atomically so intermediate values for the 'stack' @@ -662,7 +663,6 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->priority = t->base_priority; t->exit_status = -1; - hash_init (&t->open_files, fd_hash, fd_less, NULL); old_level = intr_disable (); list_push_back (&all_list, &t->allelem); diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 555b940..f89e2f9 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -4,6 +4,7 @@ #include "filesys/filesys.h" #include "threads/vaddr.h" #include "threads/interrupt.h" +#include "threads/malloc.h" #include "threads/thread.h" #include "threads/synch.h" #include "userprog/process.h" @@ -12,11 +13,12 @@ #include static struct lock filesys_lock; +static unsigned fd_counter = MIN_USER_FD; struct open_file { - int fd; /* File Descriptor / Identifier */ - struct file *file; /* Pointer to the associated file */ + int fd; /* File Descriptor / Identifier */ + struct file *file; /* Pointer to the associated file */ struct hash_elem elem; /* elem for a hash table */ }; @@ -165,10 +167,27 @@ syscall_remove (const char *file) } static int -syscall_open (const char *file UNUSED) +syscall_open (const char *file) { - //TODO - return 0; + validate_user_pointer (file, 1); + + lock_acquire (&filesys_lock); + struct file *ptr = filesys_open (file); + lock_release (&filesys_lock); + if (ptr == NULL) + return -1; + + struct open_file *file_info + = (struct open_file*) malloc (sizeof (struct open_file)); + if (file_info == NULL) + return -1; + + file_info->fd = fd_counter++; + file_info->file = ptr; + + hash_insert (&thread_current ()->open_files, &file_info->elem); + + return file_info->fd; } static int diff --git a/src/userprog/syscall.h b/src/userprog/syscall.h index 9563059..0f9288b 100644 --- a/src/userprog/syscall.h +++ b/src/userprog/syscall.h @@ -3,6 +3,8 @@ #include +#define MIN_USER_FD 2 + typedef int pid_t; void syscall_init (void); From b64434fb9d726945f6eb82befbe3d1de5062da9a Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Fri, 8 Nov 2024 15:41:35 +0000 Subject: [PATCH 15/43] Move definition of maximum file name length from to file.h --- src/filesys/file.h | 3 +++ src/lib/user/syscall.c | 2 +- src/lib/user/syscall.h | 6 ++---- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/filesys/file.h b/src/filesys/file.h index 43088db..cfa320a 100644 --- a/src/filesys/file.h +++ b/src/filesys/file.h @@ -4,6 +4,9 @@ #include "filesys/off_t.h" #include +/* The maximum length of a file name in PintOS. */ +#define FNAME_MAX_LEN 14 + struct inode; /* Opening and closing files. */ diff --git a/src/lib/user/syscall.c b/src/lib/user/syscall.c index a9c5bc8..b928b41 100644 --- a/src/lib/user/syscall.c +++ b/src/lib/user/syscall.c @@ -166,7 +166,7 @@ mkdir (const char *dir) } bool -readdir (int fd, char name[READDIR_MAX_LEN + 1]) +readdir (int fd, char name[FNAME_MAX_LEN + 1]) { return syscall2 (SYS_READDIR, fd, name); } diff --git a/src/lib/user/syscall.h b/src/lib/user/syscall.h index 32265bb..305c7af 100644 --- a/src/lib/user/syscall.h +++ b/src/lib/user/syscall.h @@ -3,6 +3,7 @@ #include #include +#include "../../filesys/file.h" /* Process identifier. */ typedef int pid_t; @@ -12,9 +13,6 @@ typedef int pid_t; typedef int mapid_t; #define MAP_FAILED ((mapid_t) -1) -/* Maximum characters in a filename written by readdir(). */ -#define READDIR_MAX_LEN 14 - /* Typical return values from main() and arguments to exit(). */ #define EXIT_SUCCESS 0 /* Successful execution. */ #define EXIT_FAILURE 1 /* Unsuccessful execution. */ @@ -41,7 +39,7 @@ void munmap (mapid_t); /* Task 4 only. */ bool chdir (const char *dir); bool mkdir (const char *dir); -bool readdir (int fd, char name[READDIR_MAX_LEN + 1]); +bool readdir (int fd, char name[FNAME_MAX_LEN + 1]); bool isdir (int fd); int inumber (int fd); From 5424276603219adf560c4748713ccf5377775f34 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 15:50:48 +0000 Subject: [PATCH 16/43] Add a helper function to get a file from its descriptor (FD), w/ E --- src/userprog/syscall.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index f89e2f9..ee91f43 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -281,6 +281,22 @@ fd_less (const struct hash_elem *a_, const struct hash_elem *b_, return a->fd < b->fd; } +/* Gets a file from its descriptor (FD number). If there is no file with the fd + FD it returns NULL. */ +static struct file * +fd_get_file (int fd) +{ + /* We have to set up a fake open_file in order to be able to search the hash + table. See hash.h. */ + struct open_file *fake_file_info; + fake_file_info->fd = fd; + + struct file *file + = hash_find (thread_current ()->open_files, fake_file_info->elem); + + return file; +} + /* 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. From a80084e9077abe7a936872a526be0c1eea725bf5 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 15:54:28 +0000 Subject: [PATCH 17/43] Fix Bug in fd_get_file declaration use open_file instead of file, w/ E --- src/userprog/syscall.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index ee91f43..faeac2d 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -44,6 +44,7 @@ static void syscall_seek (int fd, unsigned position); 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); /* A struct defining a syscall_function pointer along with its arity. */ @@ -283,7 +284,7 @@ fd_less (const struct hash_elem *a_, const struct hash_elem *b_, /* Gets a file from its descriptor (FD number). If there is no file with the fd FD it returns NULL. */ -static struct file * +static struct open_file * fd_get_file (int fd) { /* We have to set up a fake open_file in order to be able to search the hash @@ -291,10 +292,10 @@ fd_get_file (int fd) struct open_file *fake_file_info; fake_file_info->fd = fd; - struct file *file - = hash_find (thread_current ()->open_files, fake_file_info->elem); + struct hash_elem *e + = hash_find (&thread_current ()->open_files, &fake_file_info->elem); - return file; + return hash_entry (e, struct open_file, elem); } /* Validates if a block of memory starting at PTR and of size SIZE bytes is From b866fa88cd3b40d82c4f6cbe171d480e764f10c8 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Fri, 8 Nov 2024 15:54:47 +0000 Subject: [PATCH 18/43] Refactor process.c to use FNAME_MAX_LEN constant --- src/userprog/process.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 6c8ef2d..7dced36 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -81,12 +81,12 @@ start_process (void *file_name_) char *saveptr; char *arg = strtok_r (file_name_, " ", &saveptr); - char file_name[15]; - strlcpy (file_name, arg, 15); + char file_name[FNAME_MAX_LEN + 1]; + strlcpy (file_name, arg, FNAME_MAX_LEN + 1); /* TODO: Move naming of thread to process_execute, so start tokenizing there. */ - strlcpy (thread_current ()->name, file_name, 15); + strlcpy (thread_current ()->name, file_name, FNAME_MAX_LEN + 1); /* Initialize interrupt frame and load executable. */ memset (&if_, 0, sizeof if_); From 75bd3fbde07969666254b9c70a921455bcd1c45e Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 8 Nov 2024 16:02:51 +0000 Subject: [PATCH 19/43] Implement syscall for close() and fix typing bug in fd_get_file w/ S. --- src/userprog/syscall.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index faeac2d..57a5018 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -1,6 +1,7 @@ #include "userprog/syscall.h" #include "devices/shutdown.h" #include "devices/input.h" +#include "filesys/file.h" #include "filesys/filesys.h" #include "threads/vaddr.h" #include "threads/interrupt.h" @@ -261,9 +262,18 @@ syscall_tell (int fd UNUSED) } static void -syscall_close (int fd UNUSED) +syscall_close (int fd) { - //TODO + struct open_file *file_info = fd_get_file (fd); + if (file_info != NULL) + { + hash_delete (&thread_current ()->open_files, &file_info->elem); + + lock_acquire (&filesys_lock); + file_close (file_info->file); + lock_release (&filesys_lock); + free (file_info); + } } unsigned @@ -289,11 +299,11 @@ fd_get_file (int fd) { /* We have to set up a fake open_file in order to be able to search the hash table. See hash.h. */ - struct open_file *fake_file_info; - fake_file_info->fd = fd; + struct open_file fake_file_info; + fake_file_info.fd = fd; struct hash_elem *e - = hash_find (&thread_current ()->open_files, &fake_file_info->elem); + = hash_find (&thread_current ()->open_files, &fake_file_info.elem); return hash_entry (e, struct open_file, elem); } From 3cfbe198e08695002e53392d2f4ab02cacaf5113 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 8 Nov 2024 16:10:46 +0000 Subject: [PATCH 20/43] Implement syscall for seek w/ S. --- src/userprog/syscall.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 57a5018..102458f 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -249,9 +249,15 @@ syscall_write (int fd, const void *buffer, unsigned size) } static void -syscall_seek (int fd UNUSED, unsigned position UNUSED) +syscall_seek (int fd, unsigned position) { - //TODO + struct open_file *file_info = fd_get_file (fd); + if (file_info != NULL) + { + lock_acquire (&filesys_lock); + file_seek (file_info->file, position); + lock_release (&filesys_lock); + } } static unsigned From 2a1cc3c3613e9d344ddf7e915de338bebbe11e88 Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 16:14:41 +0000 Subject: [PATCH 21/43] Implement filesize and tell system calls, w/ E --- src/userprog/syscall.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 102458f..f0aaf47 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -193,10 +193,17 @@ syscall_open (const char *file) } static int -syscall_filesize (int fd UNUSED) +syscall_filesize (int fd) { - //TODO - return 0; + struct open_file *file_info = fd_get_file (fd); + if (file_info == NULL) + return -1; + + lock_acquire (&filesys_lock); + int bytes = file_length (file_info->file); + lock_release (&filesys_lock); + + return bytes; } static int @@ -261,10 +268,17 @@ syscall_seek (int fd, unsigned position) } static unsigned -syscall_tell (int fd UNUSED) +syscall_tell (int fd) { - //TODO - return 0; + struct open_file *file_info = fd_get_file (fd); + if (file_info == NULL) + return 0; + + lock_acquire (&filesys_lock); + unsigned pos = file_tell (file_info->file); + lock_release (&filesys_lock); + + return pos; } static void From 18694d7b629bdb13ced060f78ce897ac614c6ab0 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 8 Nov 2024 16:25:49 +0000 Subject: [PATCH 22/43] Implement file reading syscall and fix fd validation w/ S. --- src/userprog/syscall.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index f0aaf47..9a42214 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -211,7 +211,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) + if (fd < 0 || fd == STDOUT_FILENO) return -1; validate_user_pointer (buffer, size); @@ -228,7 +228,14 @@ syscall_read (int fd, void *buffer, unsigned size) else { /* Reading from a file. */ - return 0; // TODO: Implement Write to Files + struct open_file *file_info = fd_get_file (fd); + if (file_info == NULL) + return -1; + + lock_acquire (&filesys_lock); + int bytes_written = file_read (file_info->file, buffer, size); + lock_release (&filesys_lock); + return bytes_written; } } From 8912ef466027bc1b665874383046bec487fffc0d Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 16:26:40 +0000 Subject: [PATCH 23/43] Implement writing to file system files in the write system call, w/ E --- src/userprog/syscall.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 9a42214..4bd8732 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -258,7 +258,15 @@ syscall_write (int fd, const void *buffer, unsigned size) else { /* Writing to a file. */ - return 0; // TODO: Implement Write to Files + struct open_file *file_info = fd_get_file (fd); + if (file_info == NULL) + return 0; + + lock_acquire (&filesys_lock); + int bytes = file_write (file_info->file, buffer, size); + lock_release (&filesys_lock); + + return bytes; } } From a19f02fad7f09dc1054c93888b89ddd4c50b380f Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Fri, 8 Nov 2024 16:46:18 +0000 Subject: [PATCH 24/43] Move user process stack initialization into a helper function --- src/userprog/process.c | 189 ++++++++++++++++++++++------------------- 1 file changed, 102 insertions(+), 87 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 7dced36..48c067c 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -68,6 +68,9 @@ push_to_stack (void **esp, void *data, size_t data_size) return *esp; } +static void process_init_stack (void *file_name_, char *saveptr, void **esp, + char *file_name); + /* A thread function that loads a user process and starts it running. */ static void @@ -76,13 +79,12 @@ start_process (void *file_name_) struct intr_frame if_; bool success; - /* Retrieve first argument of command, which is the file name + /* Retrieve first argument of command, which is the file name of the process. */ char *saveptr; - char *arg = strtok_r (file_name_, " ", &saveptr); - - char file_name[FNAME_MAX_LEN + 1]; - strlcpy (file_name, arg, FNAME_MAX_LEN + 1); + char *file_name = strtok_r (file_name_, " ", &saveptr); + if (strlen (file_name) > FNAME_MAX_LEN) + file_name[FNAME_MAX_LEN + 1] = '\n'; /* TODO: Move naming of thread to process_execute, so start tokenizing there. */ @@ -95,90 +97,15 @@ start_process (void *file_name_) if_.eflags = FLAG_IF | FLAG_MBS; success = load (file_name, &if_.eip, &if_.esp); - /* 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 - of pointers to the argument data within a linked list. */ - struct list arg_list; - list_init (&arg_list); - - int arg_count = 0; - while (arg != NULL) - { - push_to_stack (&if_.esp, arg, (strlen (arg) + 1) * sizeof (char)); - - struct arg_elem *arg_elem = malloc (sizeof (struct arg_elem)); - if (arg_elem == NULL) - { - printf("ERROR: Couldn't allocate argument pointer memory for %s!\n", - file_name); - palloc_free_page (file_name_); - if (success) process_exit (); - thread_exit (); - } - - arg_elem->arg = if_.esp; - list_push_front (&arg_list, &arg_elem->elem); - - arg_count++; - arg = strtok_r (NULL, " ", &saveptr); - } - - /* Calculate the remaining number of bytes that need to be written - to the user process stack in order to check for possible overflow. */ - size_t align_size = ((unsigned int) if_.esp % 4) * sizeof (uint8_t); - size_t argv_data_size = (arg_count + 1) * sizeof (char *); - size_t argv_size = sizeof (char **); - size_t argc_size = sizeof (int); - size_t return_addr_size = sizeof (void *); - size_t remaining_size = align_size + argv_data_size + argv_size + argc_size - + return_addr_size; - - /* If pushing the rest of the data required for the stack would cause - overflow, allocate an extra page. */ - if (PHYS_BASE - if_.esp + remaining_size > PGSIZE) - { - /* TODO: Allocate an extra page for the rest of the process stack. */ - } - - /* Align stack pointer to word size before pushing argv elements for - performance. */ - if_.esp -= align_size; - - /* Push a null pointer sentinel inside argv. */ - if_.esp -= sizeof (char *); - *(char *) if_.esp = 0; - - /* Push pointer to the process file name to the stack. */ - char **argv; - - /* Push pointers to process arguments from argument linked list */ - struct list_elem *e = list_begin (&arg_list); - struct list_elem *tail = list_tail (&arg_list); - while (e != tail) - { - struct arg_elem *arg_elem = list_entry (e, struct arg_elem, elem); - - argv = push_to_stack (&if_.esp, &arg_elem->arg, sizeof (arg_elem->arg)); - - e = list_next (e); - free (arg_elem); - } - - /* Push pointer to the start of argv array. */ - push_to_stack (&if_.esp, &argv, sizeof(argv)); - - /* Push the number of arguments to the stack. */ - push_to_stack (&if_.esp, &arg_count, sizeof (arg_count)); - - /* Push fake return address (null pointer). */ - if_.esp -= sizeof (char *); - *(char *) if_.esp = 0; - /* If load failed, quit. */ - palloc_free_page (file_name_); if (!success) - thread_exit (); + { + palloc_free_page (file_name_); + thread_exit (); + } + + process_init_stack (file_name_, saveptr, &if_.esp, file_name); + palloc_free_page (file_name_); /* Start the user process by simulating a return from an interrupt, implemented by intr_exit (in @@ -190,6 +117,94 @@ start_process (void *file_name_) NOT_REACHED (); } +/* Helper function that initializes the stack of a newly created + user process. */ +static void +process_init_stack (void *file_name_, char *saveptr, void **esp, + char *file_name) + { + /* 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 + of pointers to the argument data within a linked list. */ + struct list arg_list; + list_init (&arg_list); + + char *arg = file_name; + int arg_count = 0; + while (arg != NULL) + { + push_to_stack (esp, arg, (strlen (arg) + 1) * sizeof (char)); + + struct arg_elem *arg_elem = malloc (sizeof (struct arg_elem)); + if (arg_elem == NULL) + { + printf("ERROR: Couldn't allocate argument pointer memory for %s!\n", + thread_current ()->name); + palloc_free_page (file_name_); + process_exit (); + thread_exit (); + } + + arg_elem->arg = *esp; + list_push_front (&arg_list, &arg_elem->elem); + + arg_count++; + arg = strtok_r (NULL, " ", &saveptr); + } + + /* Calculate the remaining number of bytes that need to be written + to the user process stack in order to check for possible overflow. */ + size_t align_size = ((unsigned int) *esp % 4) * sizeof (uint8_t); + size_t argv_data_size = (arg_count + 1) * sizeof (char *); + size_t argv_size = sizeof (char **); + size_t argc_size = sizeof (int); + size_t return_addr_size = sizeof (void *); + size_t remaining_size = align_size + argv_data_size + argv_size + argc_size + + return_addr_size; + + /* If pushing the rest of the data required for the stack would cause + overflow, allocate an extra page. */ + if (PHYS_BASE - *esp + remaining_size > PGSIZE) + { + /* TODO: Allocate an extra page for the rest of the process stack. */ + } + + /* Align stack pointer to word size before pushing argv elements for + performance. */ + *esp -= align_size; + + /* Push a null pointer sentinel inside argv. */ + *esp -= sizeof (char *); + *(char *) *esp = 0; + + /* Push pointer to the process file name to the stack. */ + char **argv; + + /* Push pointers to process arguments from argument linked list */ + struct list_elem *e = list_begin (&arg_list); + struct list_elem *tail = list_tail (&arg_list); + while (e != tail) + { + struct arg_elem *arg_elem = list_entry (e, struct arg_elem, elem); + + argv = push_to_stack (esp, &arg_elem->arg, sizeof (arg_elem->arg)); + + e = list_next (e); + free (arg_elem); + } + + /* Push pointer to the start of argv array. */ + push_to_stack (esp, &argv, sizeof(argv)); + + /* Push the number of arguments to the stack. */ + push_to_stack (esp, &arg_count, sizeof (arg_count)); + + /* Push fake return address (null pointer). */ + *esp -= sizeof (char *); + *(char *) *esp = 0; + } + /* Waits for thread TID to die and returns its exit status. * If it was terminated by the kernel (i.e. killed due to an exception), * returns -1. From e40794e672668ea20f68b8c892a22758a3392f1c Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 16:48:19 +0000 Subject: [PATCH 25/43] Fix Bug in fd_get_file: In case fd not found, then returns NULL, w/ E --- src/userprog/syscall.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 4bd8732..96c099d 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -340,6 +340,9 @@ fd_get_file (int fd) struct hash_elem *e = hash_find (&thread_current ()->open_files, &fake_file_info.elem); + if (e == NULL) + return NULL; + return hash_entry (e, struct open_file, elem); } From 5ed999bc9c91bd709fccb1c3e6aa57fea3733ac5 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Fri, 8 Nov 2024 16:53:30 +0000 Subject: [PATCH 26/43] Refactor push_to_stack helper to match style of other helper functions --- src/userprog/process.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 48c067c..6d6fef7 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -60,16 +60,9 @@ process_execute (const char *file_name) return tid; } -static void * -push_to_stack (void **esp, void *data, size_t data_size) -{ - *esp -= data_size; - memcpy (*esp, data, data_size); - return *esp; -} - static void process_init_stack (void *file_name_, char *saveptr, void **esp, char *file_name); +static void *push_to_stack (void **esp, void *data, size_t data_size); /* A thread function that loads a user process and starts it running. */ @@ -205,6 +198,17 @@ process_init_stack (void *file_name_, char *saveptr, void **esp, *(char *) *esp = 0; } +/* Helper function that pushes the first 'data_size' bytes stored in the + address '*data' into the stack given a pointer to the stack pointer + '**esp'. */ +static void * +push_to_stack (void **esp, void *data, size_t data_size) +{ + *esp -= data_size; + memcpy (*esp, data, data_size); + return *esp; +} + /* Waits for thread TID to die and returns its exit status. * If it was terminated by the kernel (i.e. killed due to an exception), * returns -1. From 115c650c55256b92290c6ae8ce84f7bf0bd610cd Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 8 Nov 2024 19:10:42 +0000 Subject: [PATCH 27/43] Fix Bug in thread initialisation: only init hash if USERPROG is defined --- src/threads/thread.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 037f309..1877dff 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -238,7 +238,10 @@ thread_create (const char *name, int priority, struct thread *parent_thread = thread_current (); init_thread (t, name, parent_thread->nice, priority, parent_thread->recent_cpu); tid = t->tid = allocate_tid (); - hash_init (&t->open_files, fd_hash, fd_less, NULL); + + #ifdef USERPROG + hash_init (&t->open_files, fd_hash, fd_less, NULL); + #endif /* Prepare thread for first run by initializing its stack. Do this atomically so intermediate values for the 'stack' From 7778e05aa42e7fa8c6f0e2aee1924f3c2df64ec8 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Sat, 9 Nov 2024 11:06:36 +0000 Subject: [PATCH 28/43] Fix deadlock by release of lock and semaphore in the wrong order --- src/userprog/process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 370a216..3943b21 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -262,8 +262,8 @@ process_exit (void) struct process_result *, and may be waiting so call sema_up */ else { - sema_up (&cur->result->sema); lock_release (&cur->result->lock); + sema_up (&cur->result->sema); } } @@ -283,8 +283,8 @@ process_exit (void) /* Child is still alive, signal via sema that parent has died. */ else { - sema_up (&result->sema); lock_release (&result->lock); + sema_up (&result->sema); } } From 1bfd73b2020b7beb4c91c67fb35c8c3a1847b57b Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Sun, 10 Nov 2024 01:32:48 +0000 Subject: [PATCH 29/43] Comment syscall functions and handlers --- src/userprog/syscall.c | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 96c099d..ccb02f3 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -79,6 +79,9 @@ static const syscall_arguments syscall_lookup[] = static const int LOOKUP_SIZE = sizeof (syscall_lookup) / sizeof (syscall_arguments); + +/* Initialises the syscall handling system, as well as a global lock to + synchronise all file access between processes. */ void syscall_init (void) { @@ -86,6 +89,9 @@ syscall_init (void) lock_init (&filesys_lock); } +/* Function that takes a 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) { @@ -111,6 +117,8 @@ syscall_handler (struct intr_frame *f) f->eax = syscall.function (args[0], args[1], args[2]); } +/* Called upon a "halt" syscall, resulting in a complete shutdown of the + process, via shutdown_power_off (); */ static void syscall_halt (void) { @@ -126,6 +134,10 @@ syscall_exit (int status) thread_exit (); } +/* 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 + */ static pid_t syscall_exec (const char *cmd_line) { @@ -138,12 +150,17 @@ syscall_exec (const char *cmd_line) return pid; } +/* Handles the syscall of wait. Effectively a wrapper for process_wait as the + necessary validation and such all happens in process_wait anyway. */ static int syscall_wait (pid_t pid) { return process_wait (pid); } +/* 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) { @@ -156,6 +173,9 @@ syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) return status; } +/* Handles the syscall for file removal. First validates the user file pointer. + Acquires the file system lock to prevent synchronisation issues, and then + uses FILESYS_REMOVE to remove the file, returning the same success status */ static bool syscall_remove (const char *file) { @@ -168,6 +188,10 @@ syscall_remove (const char *file) return status; } +/* Handles the syscall for opening a file connection. First, validates the file + pointer. Then it acquires a lock for the file system, in order to open the + connection without synchronisation issues. It then maps a new fd to this file + in the hash table before returning the fd. */ static int syscall_open (const char *file) { @@ -179,19 +203,27 @@ syscall_open (const char *file) if (ptr == NULL) return -1; + /* 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; + /* Populate the above struct, with a unique FD and the current open file */ file_info->fd = fd_counter++; file_info->file = ptr; + /* Add the new FD->file mapping to the hashtable for the current thread */ hash_insert (&thread_current ()->open_files, &file_info->elem); + /* Return the new FD */ return file_info->fd; } +/* Handles the syscall for getting a file's size. Converts a provided FD into + the asssociated file struct. Acquire the lock for the filesystem and use + FILE_LENGTH to calculate the length for return. */ static int syscall_filesize (int fd) { @@ -206,6 +238,10 @@ syscall_filesize (int fd) return bytes; } +/* Handles the syscall for reading SIZE bytes from a file referenced by FD. + If the FD references the console, use input_getc (), otherwise convert the + FD to its associated file struct, acquire the filesystem lock, read up to + SIZE bytes and then return the number of bytes read.*/ static int syscall_read (int fd, void *buffer, unsigned size) { @@ -239,6 +275,10 @@ syscall_read (int fd, void *buffer, unsigned size) } } +/* Handles the syscall for writing SIZE bytes to a file referenced by FD. + If the FD references the console, use put_buf (), otherwise convert the + FD to its associated file struct, acquire the filesystem lock, write up to + SIZE bytes and then return the number of bytes written.*/ static int syscall_write (int fd, const void *buffer, unsigned size) { @@ -270,6 +310,10 @@ syscall_write (int fd, const void *buffer, unsigned size) } } +/* Handles the syscall for seeking to POSITION bytes in a file referenced by + FD. Converts the FD to its associated file struct, acquires the filesystem + lock and then uses file_seek to adjust the cursor to a specific position in + the file.*/ static void syscall_seek (int fd, unsigned position) { @@ -282,6 +326,9 @@ syscall_seek (int fd, unsigned position) } } +/* Handles the syscall for returning the next byte in a file referenced by + FD. Converts the FD to its associated file struct, acquires the filesystem + lock and then uses file_tell to read the next byte.*/ static unsigned syscall_tell (int fd) { @@ -296,6 +343,9 @@ syscall_tell (int fd) return pos; } +/* Handles the syscall for closing a connection to a file. Converts the FD to + its associated file struct. If it exists, it removes it from the hash table, + acquires the filesystem lock, and uses file_close to close the connection.*/ static void syscall_close (int fd) { @@ -311,12 +361,16 @@ syscall_close (int fd) } } +/* Hashing function needed for the open_file table. Returns a hash for an entry, + based on its FD. */ unsigned fd_hash (const struct hash_elem *element, void *aux UNUSED) { return hash_int (hash_entry (element, struct open_file, elem)->fd); } +/* Comparator function for the open_file table. Compares two entries based on + the FDs. */ bool fd_less (const struct hash_elem *a_, const struct hash_elem *b_, void *aux UNUSED) From b37f205334a0ef4a36fedd284434d51c21ba17ee Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 11:13:11 +0000 Subject: [PATCH 30/43] Update process_init_stack to return success, refactoring error handling to occur inside start_process --- src/userprog/process.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 6d6fef7..adffb00 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -60,8 +60,7 @@ process_execute (const char *file_name) return tid; } -static void process_init_stack (void *file_name_, char *saveptr, void **esp, - char *file_name); +static bool process_init_stack (char *saveptr, void **esp, char *file_name); static void *push_to_stack (void **esp, void *data, size_t data_size); /* A thread function that loads a user process and starts it @@ -97,8 +96,17 @@ start_process (void *file_name_) thread_exit (); } - process_init_stack (file_name_, saveptr, &if_.esp, file_name); + /* Initialize user process stack and free page used to store the + command that executed the process. */ + success = process_init_stack (saveptr, &if_.esp, file_name); palloc_free_page (file_name_); + + /* If stack initialization failed, free resources and quit. */ + if (!success) + { + process_exit (); + thread_exit (); + } /* Start the user process by simulating a return from an interrupt, implemented by intr_exit (in @@ -111,10 +119,9 @@ start_process (void *file_name_) } /* Helper function that initializes the stack of a newly created - user process. */ -static void -process_init_stack (void *file_name_, char *saveptr, void **esp, - char *file_name) + user process. Returns true if successful, false otherwise. */ +static bool +process_init_stack (char *saveptr, void **esp, char *file_name) { /* Load command line argument *data* to user process stack. This can't cause overflow due to enforcing that the size of @@ -134,9 +141,7 @@ process_init_stack (void *file_name_, char *saveptr, void **esp, { printf("ERROR: Couldn't allocate argument pointer memory for %s!\n", thread_current ()->name); - palloc_free_page (file_name_); - process_exit (); - thread_exit (); + return false; } arg_elem->arg = *esp; @@ -196,11 +201,12 @@ process_init_stack (void *file_name_, char *saveptr, void **esp, /* Push fake return address (null pointer). */ *esp -= sizeof (char *); *(char *) *esp = 0; + + return true; } /* Helper function that pushes the first 'data_size' bytes stored in the - address '*data' into the stack given a pointer to the stack pointer - '**esp'. */ + address '*data' into the stack given a pointer to the stack pointer '**esp'. */ static void * push_to_stack (void **esp, void *data, size_t data_size) { From a165107f5fce2186091200a5b7a957f66b18accf Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 11:17:22 +0000 Subject: [PATCH 31/43] Update process execution related function argument names to be more accurate --- src/userprog/process.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index adffb00..9d4b3fc 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -38,7 +38,7 @@ static bool load (const char *cmdline, void (**eip) (void), void **esp); before process_execute() returns. Returns the new process's thread id, or TID_ERROR if the thread cannot be created. */ tid_t -process_execute (const char *file_name) +process_execute (const char *cmd) { char *fn_copy; tid_t tid; @@ -51,10 +51,10 @@ process_execute (const char *file_name) /* Imposing implicit limit that the command line arguments including the user program name fit within a single page. */ - strlcpy (fn_copy, file_name, PGSIZE); + strlcpy (fn_copy, cmd, PGSIZE); /* Create a new thread to execute FILE_NAME. */ - tid = thread_create (file_name, PRI_DEFAULT, start_process, fn_copy); + tid = thread_create (cmd, PRI_DEFAULT, start_process, fn_copy); if (tid == TID_ERROR) palloc_free_page (fn_copy); return tid; @@ -66,7 +66,7 @@ static void *push_to_stack (void **esp, void *data, size_t data_size); /* A thread function that loads a user process and starts it running. */ static void -start_process (void *file_name_) +start_process (void *cmd) { struct intr_frame if_; bool success; @@ -74,7 +74,7 @@ start_process (void *file_name_) /* Retrieve first argument of command, which is the file name of the process. */ char *saveptr; - char *file_name = strtok_r (file_name_, " ", &saveptr); + char *file_name = strtok_r (cmd, " ", &saveptr); if (strlen (file_name) > FNAME_MAX_LEN) file_name[FNAME_MAX_LEN + 1] = '\n'; @@ -92,14 +92,14 @@ start_process (void *file_name_) /* If load failed, quit. */ if (!success) { - palloc_free_page (file_name_); + palloc_free_page (cmd); thread_exit (); } /* Initialize user process stack and free page used to store the command that executed the process. */ success = process_init_stack (saveptr, &if_.esp, file_name); - palloc_free_page (file_name_); + palloc_free_page (cmd); /* If stack initialization failed, free resources and quit. */ if (!success) From f0dae74cf3e30843d6963c898a754e1fca515740 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 11:24:51 +0000 Subject: [PATCH 32/43] Update stack initialization alignment calculation to use WORD_SIZE constant for clarity --- src/userprog/process.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 9d4b3fc..b43123d 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -22,6 +22,10 @@ #include "threads/synch.h" #include "devices/timer.h" +/* Defines the native number of bytes processed by the processor + (for the purposes of alignment). */ +#define WORD_SIZE 4 + /* Keeps track of the position of pointers to user program arguments within a linked list. */ struct arg_elem @@ -153,7 +157,7 @@ process_init_stack (char *saveptr, void **esp, char *file_name) /* Calculate the remaining number of bytes that need to be written to the user process stack in order to check for possible overflow. */ - size_t align_size = ((unsigned int) *esp % 4) * sizeof (uint8_t); + size_t align_size = ((unsigned int) *esp % WORD_SIZE) * sizeof (uint8_t); size_t argv_data_size = (arg_count + 1) * sizeof (char *); size_t argv_size = sizeof (char **); size_t argc_size = sizeof (int); From 6018c0f6ec63c699e044c283089fb515dc723af7 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 11:25:20 +0000 Subject: [PATCH 33/43] Refactor process_init_stack to not surpass 80 character limit --- src/userprog/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index b43123d..a77d9df 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -162,7 +162,7 @@ process_init_stack (char *saveptr, void **esp, char *file_name) size_t argv_size = sizeof (char **); size_t argc_size = sizeof (int); size_t return_addr_size = sizeof (void *); - size_t remaining_size = align_size + argv_data_size + argv_size + argc_size + size_t remaining_size = align_size + argv_data_size + argv_size + argc_size + return_addr_size; /* If pushing the rest of the data required for the stack would cause From 795d81b7adb44876de5b59384a1618c234112536 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 11:33:29 +0000 Subject: [PATCH 34/43] Update process_init_tack saveptr argument name to cmd_savetpr for clarity --- src/userprog/process.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index a77d9df..accd108 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -64,7 +64,7 @@ process_execute (const char *cmd) return tid; } -static bool process_init_stack (char *saveptr, void **esp, char *file_name); +static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name); static void *push_to_stack (void **esp, void *data, size_t data_size); /* A thread function that loads a user process and starts it @@ -125,7 +125,7 @@ start_process (void *cmd) /* Helper function that initializes the stack of a newly created user process. Returns true if successful, false otherwise. */ static bool -process_init_stack (char *saveptr, void **esp, char *file_name) +process_init_stack (char *cmd_saveptr, void **esp, char *file_name) { /* Load command line argument *data* to user process stack. This can't cause overflow due to enforcing that the size of @@ -152,7 +152,7 @@ process_init_stack (char *saveptr, void **esp, char *file_name) list_push_front (&arg_list, &arg_elem->elem); arg_count++; - arg = strtok_r (NULL, " ", &saveptr); + arg = strtok_r (NULL, " ", &cmd_saveptr); } /* Calculate the remaining number of bytes that need to be written From 0ac46db2e426b71367dc74a28d28fe39cf830691 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 13:41:08 +0000 Subject: [PATCH 35/43] Refactor process initialization to obtain name of process file in process_execute --- src/userprog/process.c | 69 +++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 25 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index accd108..6c5f364 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -34,6 +34,18 @@ struct arg_elem struct list_elem elem; }; +/* Holds the data required to be passed from a kernel thread to a thread + that executes process_start for the purpose of starting a user process. */ +struct process_start_data + { + char *cmd; /* Pointer to a copy of the command used to execute the process. + Allocated a page that must be freed by process_start. */ + char *cmd_saveptr; /* Value pointed to by 'saveptr' argument used by + successive calls to strtok_r to split 'cmd' into + tokens while maintaining state. */ + char *file_name; /* Name of the file of the process to be started. */ + }; + static thread_func start_process NO_RETURN; static bool load (const char *cmdline, void (**eip) (void), void **esp); @@ -44,66 +56,73 @@ static bool load (const char *cmdline, void (**eip) (void), void **esp); tid_t process_execute (const char *cmd) { - char *fn_copy; + char *cmd_copy; tid_t tid; + struct process_start_data data; - /* Make a copy of FILE_NAME. + /* Make a copy of command. Otherwise there's a race between the caller and load(). */ - fn_copy = palloc_get_page (0); - if (fn_copy == NULL) + cmd_copy = palloc_get_page (0); + if (cmd_copy == NULL) return TID_ERROR; /* Imposing implicit limit that the command line arguments including the user program name fit within a single page. */ - strlcpy (fn_copy, cmd, PGSIZE); + strlcpy (cmd_copy, cmd, PGSIZE); - /* Create a new thread to execute FILE_NAME. */ - tid = thread_create (cmd, PRI_DEFAULT, start_process, fn_copy); + /* Retrieve first argument of command, which is the file name + of the process. */ + char *file_name = strtok_r (cmd_copy, " ", &data.cmd_saveptr); + if (strlen (file_name) > FNAME_MAX_LEN) + file_name[FNAME_MAX_LEN + 1] = '\n'; + + /* Create a new thread to execute the command, by initializing + it running the function 'start_process' with the appropriate + arguments. For details of arguments, see 'start_process'. */ + data.cmd = cmd_copy; + data.file_name = file_name; + + tid = thread_create (file_name, PRI_DEFAULT, start_process, &data); if (tid == TID_ERROR) - palloc_free_page (fn_copy); + palloc_free_page (cmd_copy); return tid; } static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name); static void *push_to_stack (void **esp, void *data, size_t data_size); -/* A thread function that loads a user process and starts it - running. */ +/* Make the current thread execute 'cmd', passing in a copy of the + command string used for processing, the saveptr used by strtok_r + (in order to further tokenize the same command and retrieve its + arguments), as well as the name of the file being executed. This + involves loading the specified file and starting it running. */ static void -start_process (void *cmd) +start_process (void *proc_start_data) { struct intr_frame if_; bool success; - /* Retrieve first argument of command, which is the file name - of the process. */ - char *saveptr; - char *file_name = strtok_r (cmd, " ", &saveptr); - if (strlen (file_name) > FNAME_MAX_LEN) - file_name[FNAME_MAX_LEN + 1] = '\n'; - - /* TODO: Move naming of thread to process_execute, so start - tokenizing there. */ - strlcpy (thread_current ()->name, file_name, FNAME_MAX_LEN + 1); + struct process_start_data *data = proc_start_data; + ASSERT (data->cmd_saveptr != NULL); /* Initialize interrupt frame and load executable. */ memset (&if_, 0, sizeof if_); if_.gs = if_.fs = if_.es = if_.ds = if_.ss = SEL_UDSEG; if_.cs = SEL_UCSEG; if_.eflags = FLAG_IF | FLAG_MBS; - success = load (file_name, &if_.eip, &if_.esp); + success = load (data->file_name, &if_.eip, &if_.esp); /* If load failed, quit. */ if (!success) { - palloc_free_page (cmd); + palloc_free_page (data->cmd); thread_exit (); } /* Initialize user process stack and free page used to store the command that executed the process. */ - success = process_init_stack (saveptr, &if_.esp, file_name); - palloc_free_page (cmd); + success = process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name); + palloc_free_page (data->cmd); /* If stack initialization failed, free resources and quit. */ if (!success) From 324301e7b3bebb77eb1bfcd52c7b709a3a7684de Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 13:43:22 +0000 Subject: [PATCH 36/43] Update process_execute function comment to reflect new function arguments --- src/userprog/process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 6c5f364..f8a0524 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -49,8 +49,8 @@ struct process_start_data static thread_func start_process NO_RETURN; static bool load (const char *cmdline, void (**eip) (void), void **esp); -/* Starts a new thread running a user program loaded from - FILENAME. The new thread may be scheduled (and may even exit) +/* Starts a new thread running a user program executed via + CMD. The new thread may be scheduled (and may even exit) before process_execute() returns. Returns the new process's thread id, or TID_ERROR if the thread cannot be created. */ tid_t From 8b2fc86b516daa2422489622e88b5dcd7da7d76c Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Sun, 10 Nov 2024 14:34:38 +0000 Subject: [PATCH 37/43] Refactor process_init_stack to reduce code duplication --- src/userprog/process.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index f8a0524..c935a07 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -90,6 +90,7 @@ process_execute (const char *cmd) static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name); static void *push_to_stack (void **esp, void *data, size_t data_size); +#define push_var_to_stack(esp, var) (push_to_stack (esp, &var, sizeof (var))) /* Make the current thread execute 'cmd', passing in a copy of the command string used for processing, the saveptr used by strtok_r @@ -196,11 +197,8 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) *esp -= align_size; /* Push a null pointer sentinel inside argv. */ - *esp -= sizeof (char *); - *(char *) *esp = 0; - - /* Push pointer to the process file name to the stack. */ - char **argv; + char *null_sentinel = NULL; + push_var_to_stack (esp, null_sentinel); /* Push pointers to process arguments from argument linked list */ struct list_elem *e = list_begin (&arg_list); @@ -209,21 +207,21 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) { struct arg_elem *arg_elem = list_entry (e, struct arg_elem, elem); - argv = push_to_stack (esp, &arg_elem->arg, sizeof (arg_elem->arg)); + push_var_to_stack(esp, arg_elem->arg); e = list_next (e); free (arg_elem); } /* Push pointer to the start of argv array. */ - push_to_stack (esp, &argv, sizeof(argv)); + char **argv = *esp; + push_var_to_stack(esp, argv); /* Push the number of arguments to the stack. */ - push_to_stack (esp, &arg_count, sizeof (arg_count)); + push_var_to_stack(esp, arg_count); /* Push fake return address (null pointer). */ - *esp -= sizeof (char *); - *(char *) *esp = 0; + push_var_to_stack (esp, null_sentinel); return true; } From b8d358ecb2472a9bc6ecdd00b1de5560dd8a5b2d Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Mon, 11 Nov 2024 13:13:21 +0000 Subject: [PATCH 38/43] Update stack initialization to handle overflow by allocating a second page for argument pointers --- src/userprog/process.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index c935a07..792fe76 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -88,6 +88,8 @@ process_execute (const char *cmd) return tid; } + +static bool install_page (void *upage, void *kpage, bool writable); static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name); static void *push_to_stack (void **esp, void *data, size_t data_size); #define push_var_to_stack(esp, var) (push_to_stack (esp, &var, sizeof (var))) @@ -186,10 +188,13 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) + return_addr_size; /* If pushing the rest of the data required for the stack would cause - overflow, allocate an extra page. */ + overflow, allocate an extra page that is contiguous within the + virtual address space (below the current address range). */ if (PHYS_BASE - *esp + remaining_size > PGSIZE) { - /* TODO: Allocate an extra page for the rest of the process stack. */ + uint8_t *kpage = palloc_get_page (PAL_USER | PAL_ZERO); + if (!install_page (((uint8_t *) PHYS_BASE) - PGSIZE * 2, kpage, true)) + return false; } /* Align stack pointer to word size before pushing argv elements for @@ -485,8 +490,6 @@ load (const char *file_name, void (**eip) (void), void **esp) /* load() helpers. */ -static bool install_page (void *upage, void *kpage, bool writable); - /* Checks whether PHDR describes a valid, loadable segment in FILE and returns true if so, false otherwise. */ static bool From 18c884234da852a56b894dee031a8d7008bd4f2c Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Mon, 11 Nov 2024 17:35:49 +0000 Subject: [PATCH 39/43] Fix race-condition in process result (memory leak), fix infinite loop in donors_list --- src/lib/kernel/list.c | 3 +++ src/threads/synch.c | 1 + src/userprog/process.c | 5 +++-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/lib/kernel/list.c b/src/lib/kernel/list.c index ac3d55d..f8f7fbb 100644 --- a/src/lib/kernel/list.c +++ b/src/lib/kernel/list.c @@ -170,6 +170,9 @@ list_insert (struct list_elem *before, struct list_elem *elem) { ASSERT (is_interior (before) || is_tail (before)); ASSERT (elem != NULL); + // Sanity checks to prevent (some) loop lists + ASSERT (before != elem); + ASSERT (before->prev != elem); elem->prev = before->prev; elem->next = before; diff --git a/src/threads/synch.c b/src/threads/synch.c index d706d19..c8bbd6d 100644 --- a/src/threads/synch.c +++ b/src/threads/synch.c @@ -341,6 +341,7 @@ 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/userprog/process.c b/src/userprog/process.c index 3943b21..4da68f8 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -262,11 +262,12 @@ process_exit (void) struct process_result *, and may be waiting so call sema_up */ else { - lock_release (&cur->result->lock); sema_up (&cur->result->sema); + lock_release (&cur->result->lock); } } + /* 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)) @@ -283,8 +284,8 @@ process_exit (void) /* Child is still alive, signal via sema that parent has died. */ else { - lock_release (&result->lock); sema_up (&result->sema); + lock_release (&result->lock); } } From 98a581840666de1fd45810ac1268301f9e2cd105 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Mon, 11 Nov 2024 17:55:24 +0000 Subject: [PATCH 40/43] add file_deny_write and file_allow_write to process creation and exiting to make executable read-only --- src/threads/thread.h | 1 + src/userprog/process.c | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/threads/thread.h b/src/threads/thread.h index 6c65dcb..33a5485 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -127,6 +127,7 @@ struct thread struct process_result *result; /* Result of the process. */ struct list child_results; /* List of children's of this thread process results. */ + struct file *exec_file; /* Thread's currently running file */ /* Shared between thread.c and synch.c. */ struct list_elem elem; /* List element. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index 3943b21..e11eff9 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -181,6 +181,10 @@ start_process (void *file_name_) if (!success) thread_exit (); + struct file *exec_file = filesys_open (file_name); + thread_current ()->exec_file = exec_file; + file_deny_write (exec_file); + /* 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 @@ -246,6 +250,7 @@ process_exit (void) uint32_t *pd; printf ("%s: exit(%d)\n", cur->name, cur->exit_status); + file_close (cur->exec_file); /* Update process result. */ if (cur->result != NULL) From 049fc5559c20f97fd03b4f669aeb48b0880d2d04 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Mon, 11 Nov 2024 21:20:53 +0000 Subject: [PATCH 41/43] Reformat stack initialization code to follow style for length and spacing --- src/userprog/process.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 792fe76..a507d5d 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -40,9 +40,9 @@ struct process_start_data { char *cmd; /* Pointer to a copy of the command used to execute the process. Allocated a page that must be freed by process_start. */ - char *cmd_saveptr; /* Value pointed to by 'saveptr' argument used by - successive calls to strtok_r to split 'cmd' into - tokens while maintaining state. */ + char *cmd_saveptr; /* Value pointed to by 'saveptr' argument used by + successive calls to strtok_r to split 'cmd' into + tokens while maintaining state. */ char *file_name; /* Name of the file of the process to be started. */ }; @@ -88,7 +88,6 @@ process_execute (const char *cmd) return tid; } - static bool install_page (void *upage, void *kpage, bool writable); static bool process_init_stack (char *cmd_saveptr, void **esp, char *file_name); static void *push_to_stack (void **esp, void *data, size_t data_size); @@ -231,8 +230,9 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name) return true; } -/* Helper function that pushes the first 'data_size' bytes stored in the - address '*data' into the stack given a pointer to the stack pointer '**esp'. */ +/* Helper function that pushes the first 'data_size' bytes stored + in the address '*data' into the stack given a pointer to the + stack pointer '**esp'. */ static void * push_to_stack (void **esp, void *data, size_t data_size) { From 52fdd47e0c9342abbaec1be9d5fd7539560559e7 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Mon, 11 Nov 2024 21:51:38 +0000 Subject: [PATCH 42/43] Fix race condition in the passing of data from thread executing process_execute to its child --- src/userprog/process.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index a507d5d..c960df9 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -43,7 +43,8 @@ struct process_start_data char *cmd_saveptr; /* Value pointed to by 'saveptr' argument used by successive calls to strtok_r to split 'cmd' into tokens while maintaining state. */ - char *file_name; /* Name of the file of the process to be started. */ + char file_name[FNAME_MAX_LEN]; /* Name of the file of the process to + be started. */ }; static thread_func start_process NO_RETURN; @@ -58,7 +59,12 @@ process_execute (const char *cmd) { char *cmd_copy; tid_t tid; - struct process_start_data data; + + struct process_start_data *data = malloc (sizeof (struct process_start_data)); + if (data == NULL) + { + return TID_ERROR; + } /* Make a copy of command. Otherwise there's a race between the caller and load(). */ @@ -72,17 +78,15 @@ process_execute (const char *cmd) /* Retrieve first argument of command, which is the file name of the process. */ - char *file_name = strtok_r (cmd_copy, " ", &data.cmd_saveptr); - if (strlen (file_name) > FNAME_MAX_LEN) - file_name[FNAME_MAX_LEN + 1] = '\n'; + char *file_name = strtok_r (cmd_copy, " ", &data->cmd_saveptr); /* Create a new thread to execute the command, by initializing it running the function 'start_process' with the appropriate arguments. For details of arguments, see 'start_process'. */ - data.cmd = cmd_copy; - data.file_name = file_name; - - tid = thread_create (file_name, PRI_DEFAULT, start_process, &data); + data->cmd = cmd_copy; + strlcpy (data->file_name, file_name, FNAME_MAX_LEN); + + tid = thread_create (file_name, PRI_DEFAULT, start_process, data); if (tid == TID_ERROR) palloc_free_page (cmd_copy); return tid; @@ -105,7 +109,6 @@ start_process (void *proc_start_data) bool success; struct process_start_data *data = proc_start_data; - ASSERT (data->cmd_saveptr != NULL); /* Initialize interrupt frame and load executable. */ memset (&if_, 0, sizeof if_); @@ -118,7 +121,7 @@ start_process (void *proc_start_data) if (!success) { palloc_free_page (data->cmd); - thread_exit (); + goto fail; } /* Initialize user process stack and free page used to store the @@ -130,7 +133,7 @@ start_process (void *proc_start_data) if (!success) { process_exit (); - thread_exit (); + goto fail; } /* Start the user process by simulating a return from an @@ -141,6 +144,11 @@ start_process (void *proc_start_data) and jump to it. */ 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); + thread_exit (); } /* Helper function that initializes the stack of a newly created From 14a4841772ddc52d9aeebabde1781841a5eb1832 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Mon, 11 Nov 2024 22:13:10 +0000 Subject: [PATCH 43/43] Fix bug where size of file name buffer was less than maximum file name size --- src/userprog/process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index c960df9..7e211c4 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -43,7 +43,7 @@ struct process_start_data char *cmd_saveptr; /* Value pointed to by 'saveptr' argument used by successive calls to strtok_r to split 'cmd' into tokens while maintaining state. */ - char file_name[FNAME_MAX_LEN]; /* Name of the file of the process to + char file_name[FNAME_MAX_LEN + 1]; /* Name of the file of the process to be started. */ }; @@ -84,7 +84,7 @@ process_execute (const char *cmd) it running the function 'start_process' with the appropriate arguments. For details of arguments, see 'start_process'. */ data->cmd = cmd_copy; - strlcpy (data->file_name, file_name, FNAME_MAX_LEN); + strlcpy (data->file_name, file_name, FNAME_MAX_LEN + 1); tid = thread_create (file_name, PRI_DEFAULT, start_process, data); if (tid == TID_ERROR)