Compare commits

..

5 Commits

Author SHA1 Message Date
Themis Demetriades
e1489e5244 Fix Bug: Free all entries in the fd hashtable when the process exits, (Saleh, Ethan) 2024-11-13 21:57:41 +00:00
Themis Demetriades
785f1a8d62 Fix child_results loop accessing after free() 2024-11-13 21:52:38 +00:00
Themis Demetriades
1368fa144a Add debugging messages for probing multi-oom test case 2024-11-13 18:57:02 +00:00
Themis Demetriades
446c50ea29 Remove superfluous process_exit () in start_process 2024-11-13 17:58:34 +00:00
Themis Demetriades
be68d81cf6 Fix memory leak in start_process due to not freeing proc_start_data when success in initializing stack 2024-11-13 17:21:42 +00:00
15 changed files with 108 additions and 228 deletions

View File

@@ -23,6 +23,11 @@ test_devices:
variables:
DIR: devices
test_filesys:
extends: .pintos_tests
variables:
DIR: filesys
test_threads:
extends: .pintos_tests
variables:

View File

@@ -9,14 +9,14 @@ sc-bad-arg sc-bad-num sc-boundary sc-boundary-2 halt exit create-normal \
create-empty create-null create-bad-ptr create-long create-exists \
create-bound open-normal open-missing open-boundary open-empty \
open-null open-bad-ptr open-twice close-normal close-twice close-stdin \
close-stdout close-bad-fd read-normal read-bad-ptr read-bad-buf read-boundary \
read-zero read-stdout read-bad-fd write-normal write-bad-ptr write-bad-buf \
close-stdout close-bad-fd read-normal read-bad-ptr read-boundary \
read-zero read-stdout read-bad-fd write-normal write-bad-ptr \
write-boundary write-zero write-stdin write-bad-fd exec-once exec-arg \
exec-large-arg exec-multiple exec-missing exec-over-arg exec-over-args \
exec-bad-ptr wait-simple wait-twice wait-killed wait-load-kill \
wait-bad-pid wait-bad-child multi-recurse multi-child-fd rox-simple \
rox-child rox-multichild bad-read bad-write bad-read2 bad-write2 \
bad-jump bad-jump2 bad-maths overflow-stack)
bad-jump bad-jump2 bad-maths)
tests/userprog_PROGS = $(tests/userprog_TESTS) $(addprefix \
tests/userprog/,child-simple child-args child-bad child-close child-rox exec-exit)
@@ -36,7 +36,6 @@ tests/userprog/bad-read2_SRC = tests/userprog/bad-read2.c tests/main.c
tests/userprog/bad-write2_SRC = tests/userprog/bad-write2.c tests/main.c
tests/userprog/bad-jump2_SRC = tests/userprog/bad-jump2.c tests/main.c
tests/userprog/bad-maths_SRC = tests/userprog/bad-maths.c tests/main.c
tests/userprog/overflow-stack_SRC = tests/userprog/overflow-stack.c tests/main.c
tests/userprog/sc-boundary_SRC = tests/userprog/sc-boundary.c \
tests/userprog/boundary.c tests/main.c
tests/userprog/sc-boundary-2_SRC = tests/userprog/sc-boundary-2.c \
@@ -67,7 +66,6 @@ tests/userprog/close-stdout_SRC = tests/userprog/close-stdout.c tests/main.c
tests/userprog/close-bad-fd_SRC = tests/userprog/close-bad-fd.c tests/main.c
tests/userprog/read-normal_SRC = tests/userprog/read-normal.c tests/main.c
tests/userprog/read-bad-ptr_SRC = tests/userprog/read-bad-ptr.c tests/main.c
tests/userprog/read-bad-buf_SRC = tests/userprog/read-bad-buf.c tests/main.c
tests/userprog/read-boundary_SRC = tests/userprog/read-boundary.c \
tests/userprog/boundary.c tests/main.c
tests/userprog/read-zero_SRC = tests/userprog/read-zero.c tests/main.c
@@ -75,7 +73,6 @@ tests/userprog/read-stdout_SRC = tests/userprog/read-stdout.c tests/main.c
tests/userprog/read-bad-fd_SRC = tests/userprog/read-bad-fd.c tests/main.c
tests/userprog/write-normal_SRC = tests/userprog/write-normal.c tests/main.c
tests/userprog/write-bad-ptr_SRC = tests/userprog/write-bad-ptr.c tests/main.c
tests/userprog/write-bad-buf_SRC = tests/userprog/write-bad-buf.c tests/main.c
tests/userprog/write-boundary_SRC = tests/userprog/write-boundary.c \
tests/userprog/boundary.c tests/main.c
tests/userprog/write-zero_SRC = tests/userprog/write-zero.c tests/main.c
@@ -125,12 +122,10 @@ tests/userprog/close-normal_PUTFILES += tests/userprog/sample.txt
tests/userprog/close-twice_PUTFILES += tests/userprog/sample.txt
tests/userprog/read-normal_PUTFILES += tests/userprog/sample.txt
tests/userprog/read-bad-ptr_PUTFILES += tests/userprog/sample.txt
tests/userprog/read-bad-buf_PUTFILES += tests/userprog/sample.txt
tests/userprog/read-boundary_PUTFILES += tests/userprog/sample.txt
tests/userprog/read-zero_PUTFILES += tests/userprog/sample.txt
tests/userprog/write-normal_PUTFILES += tests/userprog/sample.txt
tests/userprog/write-bad-ptr_PUTFILES += tests/userprog/sample.txt
tests/userprog/write-bad-buf_PUTFILES += tests/userprog/sample.txt
tests/userprog/write-boundary_PUTFILES += tests/userprog/sample.txt
tests/userprog/write-zero_PUTFILES += tests/userprog/sample.txt
tests/userprog/multi-child-fd_PUTFILES += tests/userprog/sample.txt

View File

@@ -1,9 +1,5 @@
Full robustness of argument passing and syscall handling code:
- Test user stack overflow robustness of "exec" system calls and user code.
Full robustness of argument passing code:
- Test user stack overflow robustness of "exec" system calls.
5 exec-over-arg
5 exec-over-args
5 overflow-stack
- Test syscall user provided buffer validity checks.
5 read-bad-buf
5 write-bad-buf

View File

@@ -2,7 +2,11 @@
use strict;
use warnings;
use tests::tests;
check_expected ([<<'EOF']);
check_expected ([<<'EOF', <<'EOF']);
(exec-bad-ptr) begin
(exec-bad-ptr) end
exec-bad-ptr: exit(0)
EOF
(exec-bad-ptr) begin
exec-bad-ptr: exit(-1)
EOF

View File

@@ -2,7 +2,11 @@
use strict;
use warnings;
use tests::tests;
check_expected ([<<'EOF']);
check_expected ([<<'EOF', <<'EOF']);
(open-bad-ptr) begin
(open-bad-ptr) end
open-bad-ptr: exit(0)
EOF
(open-bad-ptr) begin
open-bad-ptr: exit(-1)
EOF

View File

@@ -1,17 +0,0 @@
/* Attempt to overflow the user stack by allocating a 4kB buffer and writing into it.
The process must be terminated with -1 exit code until stack growth has been implemented in Task 3
*/
#include <string.h>
#include <syscall.h>
#include "tests/lib.h"
#include "tests/main.h"
void
test_main (void)
{
char stack_obj[4096];
memset (stack_obj, 'a', sizeof stack_obj);
memset (stack_obj+10, '\0', 1);
msg ("buffer: %s", stack_obj);
}

View File

@@ -1,14 +0,0 @@
# -*- perl -*-
use strict;
use warnings;
use tests::tests;
check_expected (IGNORE_USER_FAULTS => 1, [<<'EOF',<<'EOF']);
(overflow-stack) begin
overflow-stack: exit(-1)
EOF
(overflow-stack) begin
(overflow-stack) buffer: aaaaaaaaaa
(overflow-stack) end
overflow-stack: exit(0)
EOF
pass;

View File

@@ -1,17 +0,0 @@
/* Passes a buffer to the read system call that starts in valid memory, but runs into kernel space.
The process must be terminated with -1 exit code.
*/
#include <syscall.h>
#include "tests/lib.h"
#include "tests/main.h"
void
test_main (void)
{
int handle;
CHECK ((handle = open ("sample.txt")) > 1, "open \"sample.txt\"");
read (handle, (char *) 0xbfffffe0, 100);
fail ("should not have survived read()");
}

View File

@@ -1,10 +0,0 @@
# -*- perl -*-
use strict;
use warnings;
use tests::tests;
check_expected (IGNORE_KERNEL_FAULTS => 1, [<<'EOF']);
(read-bad-buf) begin
(read-bad-buf) open "sample.txt"
read-bad-buf: exit(-1)
EOF
pass;

View File

@@ -2,7 +2,12 @@
use strict;
use warnings;
use tests::tests;
check_expected ([<<'EOF']);
check_expected ([<<'EOF', <<'EOF']);
(read-bad-ptr) begin
(read-bad-ptr) open "sample.txt"
(read-bad-ptr) end
read-bad-ptr: exit(0)
EOF
(read-bad-ptr) begin
(read-bad-ptr) open "sample.txt"
read-bad-ptr: exit(-1)

View File

@@ -1,17 +0,0 @@
/* Passes a buffer to the write system call that starts in valid memory, but runs into kernel space.
The process must be terminated with -1 exit code.
*/
#include <syscall.h>
#include "tests/lib.h"
#include "tests/main.h"
void
test_main (void)
{
int handle;
CHECK ((handle = open ("sample.txt")) > 1, "open \"sample.txt\"");
write (handle, (char *) 0xbffffff0, 32);
fail ("should have exited with -1");
}

View File

@@ -1,10 +0,0 @@
# -*- perl -*-
use strict;
use warnings;
use tests::tests;
check_expected (IGNORE_KERNEL_FAULTS => 1, [<<'EOF']);
(write-bad-buf) begin
(write-bad-buf) open "sample.txt"
write-bad-buf: exit(-1)
EOF
pass;

View File

@@ -2,7 +2,12 @@
use strict;
use warnings;
use tests::tests;
check_expected ([<<'EOF']);
check_expected ([<<'EOF', <<'EOF']);
(write-bad-ptr) begin
(write-bad-ptr) open "sample.txt"
(write-bad-ptr) end
write-bad-ptr: exit(0)
EOF
(write-bad-ptr) begin
(write-bad-ptr) open "sample.txt"
write-bad-ptr: exit(-1)

View File

@@ -47,9 +47,9 @@ struct process_start_data
tokens while maintaining state. */
char file_name[FNAME_MAX_LEN + 1]; /* Name of the file of the process to
be started. */
bool success; /* Indicates whether the process was successfully loaded. */
struct semaphore loaded; /* Semaphore used to signal that the process has
been loaded. */
struct semaphore load_sema;
bool success;
};
static thread_func start_process NO_RETURN;
@@ -65,13 +65,7 @@ process_execute (const char *cmd)
char *cmd_copy;
tid_t tid;
struct process_start_data *data = malloc (sizeof (struct process_start_data));
if (data == NULL)
{
return TID_ERROR;
}
sema_init (&data->loaded, 0);
data->success = false;
struct process_start_data data;
/* Make a copy of command.
Otherwise there's a race between the caller and load(). */
@@ -85,7 +79,7 @@ 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);
char *file_name = strtok_r (cmd_copy, " ", &data.cmd_saveptr);
/* NOTE: Currently, the file being executed is closed in load () and then
reopened here. Because load is an exported public function, this
@@ -100,19 +94,24 @@ process_execute (const char *cmd)
/* 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;
strlcpy (data->file_name, file_name, FNAME_MAX_LEN + 1);
data.cmd = cmd_copy;
strlcpy (data.file_name, file_name, FNAME_MAX_LEN + 1);
sema_init (&data.load_sema, 0);
data.success = false;
tid = thread_create (file_name, PRI_DEFAULT, start_process, &data);
tid = thread_create (file_name, PRI_DEFAULT, start_process, data);
if (tid == TID_ERROR)
palloc_free_page (cmd_copy);
{
palloc_free_page (cmd_copy);
}
else
{
sema_down (&data->loaded);
if (!data->success)
sema_down (&data.load_sema);
if (!data.success)
tid = TID_ERROR;
}
free (data);
return tid;
}
@@ -130,7 +129,6 @@ static void
start_process (void *proc_start_data)
{
struct intr_frame if_;
bool success;
struct process_start_data *data = proc_start_data;
@@ -144,37 +142,33 @@ start_process (void *proc_start_data)
/* Prevent writing to the file being executed. */
struct file *exec_file = filesys_open (data->file_name);
if (exec_file == NULL)
{
lock_release (&filesys_lock);
goto fail;
}
thread_current ()->exec_file = exec_file;
file_deny_write (exec_file);
lock_release (&filesys_lock);
success = load (data->file_name, &if_.eip, &if_.esp);
data->success = load (data->file_name, &if_.eip, &if_.esp);
/* If load failed, quit. */
if (!success)
/* If load failed, free process startup data and quit. */
if (!data->success)
{
palloc_free_page (data->cmd);
goto fail;
sema_up (&data->load_sema);
thread_exit ();
}
/* Initialize user process stack and free page used to store the
command that executed the process. */
success = process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name);
bool success = process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name);
palloc_free_page (data->cmd);
data->success = success;
sema_up (&data->load_sema);
/* If stack initialization failed, free resources and quit. */
/* If stack initialization failed, free process resources and quit. */
if (!success)
{
goto fail;
thread_exit ();
}
data->success = true;
sema_up (&data->loaded);
/* Start the user process by simulating a return from an
interrupt, implemented by intr_exit (in
threads/intr-stubs.S). Because intr_exit takes all of its
@@ -183,12 +177,6 @@ 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, exit. */
fail:
data->success = false;
sema_up (&data->loaded);
thread_exit ();
}
/* Helper function that initializes the stack of a newly created
@@ -319,7 +307,9 @@ process_wait (tid_t child_tid UNUSED)
break;
}
if (child_result == NULL)
return -1;
{
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
@@ -333,6 +323,7 @@ process_wait (tid_t child_tid UNUSED)
int exit_status = child_result->exit_status;
lock_release (&child_result->lock);
free (child_result);
return exit_status;
}
@@ -340,6 +331,8 @@ process_wait (tid_t child_tid UNUSED)
void
process_exit (void)
{
struct thread *cur = thread_current ();
uint32_t *pd;

View File

@@ -13,9 +13,6 @@
#include <stdio.h>
#include <syscall-nr.h>
#define MAX_SYSCALL_ARGS 3
#define EXIT_FAILURE -1
static unsigned fd_counter = MIN_USER_FD;
struct open_file
@@ -48,19 +45,18 @@ 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 *start, size_t size);
static void validate_user_string (const char *str);
static void *validate_user_pointer (const void *ptr, size_t size);
/* A struct defining a syscall_function pointer along with its arity. */
struct syscall_arguments
typedef struct
{
syscall_function function; /* Function pointer. */
int arity; /* Number of arguments of the function. */
};
} syscall_arguments;
/* A look-up table mapping numbers to system call functions with their number of
arguments. */
static const struct syscall_arguments syscall_lookup[] =
static const syscall_arguments syscall_lookup[] =
{
[SYS_HALT] = {(syscall_function) syscall_halt, 0},
[SYS_EXIT] = {(syscall_function) syscall_exit, 1},
@@ -80,7 +76,8 @@ static const struct syscall_arguments syscall_lookup[] =
/* The number of syscall functions (i.e, number of elements) within the
syscall_lookup table. */
static const int LOOKUP_SIZE
= sizeof (syscall_lookup) / sizeof (struct syscall_arguments);
= sizeof (syscall_lookup) / sizeof (syscall_arguments);
/* Initialises the syscall handling system, as well as a global lock to
synchronise all file access between processes. */
@@ -91,29 +88,28 @@ syscall_init (void)
lock_init (&filesys_lock);
}
/* Function that takes an interrupt frame containing a syscall and its args.
/* 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)
{
/* First, read the system call number from the stack. */
validate_user_pointer (f->esp, sizeof (uintptr_t));
uintptr_t syscall_number = *(int *) f->esp;
validate_user_pointer (f->esp, 1);
unsigned syscall_number = *(int *) f->esp;
/* Ensures the number corresponds to a system call that can be handled. */
if (syscall_number >= LOOKUP_SIZE)
syscall_exit (EXIT_FAILURE);
thread_exit ();
struct syscall_arguments syscall = syscall_lookup[syscall_number];
syscall_arguments syscall = syscall_lookup[syscall_number];
/* Next, read and copy the arguments from the stack pointer. */
validate_user_pointer (f->esp + sizeof (uintptr_t),
syscall.arity * sizeof (uintptr_t));
uintptr_t args[MAX_SYSCALL_ARGS] = {0};
for (int i = 0; i < syscall.arity && i < MAX_SYSCALL_ARGS; i++)
args[i] = *(uintptr_t *) (f->esp + sizeof (uintptr_t) * (i + 1));
uintptr_t args[3] = {0};
for (int i=0; i < syscall.arity; i++)
args[i] = *(uintptr_t *) (f->esp + sizeof (uintptr_t) * (i + 1));
/* Call the function that handles this system call with the arguments. When
there is a return value it is stored in f->eax. */
@@ -138,13 +134,17 @@ syscall_exit (int status)
}
/* Executes a given command with the relevant args, by calling process_execute.
Returns PID for the process that is running the CMD_LINE. */
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)
{
validate_user_string (cmd_line);
validate_user_pointer (cmd_line, 1);
return process_execute (cmd_line); /* Returns the PID of the new process */
pid_t pid = process_execute(cmd_line);
return pid;
}
/* Handles the syscall of wait. Effectively a wrapper for process_wait as the
@@ -152,16 +152,16 @@ syscall_exec (const char *cmd_line)
static int
syscall_wait (pid_t pid)
{
return process_wait (pid); /* Returns the exit status of the waited process */
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, unsigned initial_size)
syscall_create (const char *file UNUSED, unsigned initial_size UNUSED)
{
validate_user_string (file);
validate_user_pointer (file, 1);
lock_acquire (&filesys_lock);
bool status = filesys_create (file, initial_size);
@@ -176,7 +176,7 @@ syscall_create (const char *file, unsigned initial_size)
static bool
syscall_remove (const char *file)
{
validate_user_string (file);
validate_user_pointer (file, 1);
lock_acquire (&filesys_lock);
bool status = filesys_remove (file);
@@ -192,23 +192,20 @@ syscall_remove (const char *file)
static int
syscall_open (const char *file)
{
validate_user_string (file);
validate_user_pointer (file, 1);
lock_acquire (&filesys_lock);
struct file *ptr = filesys_open (file);
lock_release (&filesys_lock);
if (ptr == NULL)
return EXIT_FAILURE;
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)
{
file_close (ptr);
return EXIT_FAILURE;
}
return -1;
/* Populate the above struct, with a unique FD and the current open file */
file_info->fd = fd_counter++;
@@ -229,7 +226,7 @@ syscall_filesize (int fd)
{
struct open_file *file_info = fd_get_file (fd);
if (file_info == NULL)
return EXIT_FAILURE;
return -1;
lock_acquire (&filesys_lock);
int bytes = file_length (file_info->file);
@@ -248,7 +245,7 @@ syscall_read (int fd, void *buffer, unsigned size)
/* Only console (fd = 0) or other files, not including STDOUT, (fd > 1) are
allowed. */
if (fd < 0 || fd == STDOUT_FILENO)
return EXIT_FAILURE;
return -1;
validate_user_pointer (buffer, size);
@@ -266,7 +263,7 @@ syscall_read (int fd, void *buffer, unsigned size)
/* Reading from a file. */
struct open_file *file_info = fd_get_file (fd);
if (file_info == NULL)
return EXIT_FAILURE;
return -1;
lock_acquire (&filesys_lock);
int bytes_written = file_read (file_info->file, buffer, size);
@@ -414,57 +411,18 @@ fd_get_file (int fd)
return hash_entry (e, struct open_file, elem);
}
/* Validates if a block of memory starting at START and of size SIZE bytes is
fully contained within user virtual memory. Kills the thread (by exiting with
failure) if the memory is invalid. Otherwise, returns (nothing) normally.
If the size is 0, the function does no checks and returns the given ptr. */
static void
validate_user_pointer (const void *start, size_t size)
/* Validates if a block of memory starting at PTR and of size SIZE bytes is
fully contained within user virtual memory. Kills the thread (by calling
thread_exit) if the memory is invalid. Otherwise, returns the PTR given.
If the size is 0, the function does no checks and returns PTR.*/
static void *
validate_user_pointer (const void *ptr, size_t size)
{
if (size == 0)
return;
if (size > 0 && (ptr == NULL ||
!is_user_vaddr (ptr) ||
!is_user_vaddr (ptr + size - 1) ||
pagedir_get_page (thread_current()->pagedir, ptr) == NULL))
thread_exit ();
const void *end = start + size - 1;
if (start == NULL || !is_user_vaddr (start) || !is_user_vaddr (end))
syscall_exit (EXIT_FAILURE);
/* We now need to check if the entire memory block is mapped to physical
memory by the page table. */
for (const void *ptr = start; ptr <= end; ptr += PGSIZE)
if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL)
syscall_exit (EXIT_FAILURE);
}
/* Validates if a string is fully contained within user virtual memory. Kills
the thread (by exiting with failure) if the memory is invalid. Otherwise,
returns (nothing) normally. */
static void
validate_user_string (const char *str)
{
if (str == NULL || !is_user_vaddr (str))
syscall_exit (EXIT_FAILURE);
size_t offset = (uintptr_t) str % PGSIZE;
/* We move page by page, checking if the page is mapped to physical memory. */
for (;;)
{
void *page = pg_round_down (str);
if (!is_user_vaddr(page) ||
pagedir_get_page (thread_current ()->pagedir, page) == NULL)
syscall_exit (EXIT_FAILURE);
while (offset < PGSIZE)
{
if (*str == '\0')
return; /* We reached the end of the string without issues. */
str++;
offset++;
}
offset = 0; /* Next page will start at the beginning. */
}
return (void *) ptr;
}