From 4dfa242234991ca0e17ba61c432cdbfc879a642a Mon Sep 17 00:00:00 2001 From: Hang Park Date: Fri, 14 Apr 2017 03:00:17 +0900 Subject: [PATCH] [#73] Avoid memory leak Free resources that had not been controlled. --- src/threads/thread.c | 5 +- src/userprog/process.c | 118 +++++++++++++++++++++++++---------------- src/userprog/syscall.c | 3 +- 3 files changed, 79 insertions(+), 47 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 09bb282..fb05349 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -197,7 +197,10 @@ thread_create (const char *name, int priority, new_proc_info = (struct process_info *) malloc (sizeof (struct process_info)); if (new_proc_info == NULL) - return TID_ERROR; + { + palloc_free_page (t); + return TID_ERROR; + } new_proc->info = new_proc_info; new_proc_info->pid = (pid_t) t->tid; diff --git a/src/userprog/process.c b/src/userprog/process.c index c4be183..576dc57 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -28,10 +28,10 @@ struct arguments char **argv; /* Array of arguments. */ }; +static struct arguments *parse_arguments(char *str_input); static thread_func start_process NO_RETURN; static bool load (struct arguments *args, void (**eip) (void), void **esp, struct file **exec_file); -static void argument_parser(char *str, struct arguments *args); /* Starts a new thread running a user program loaded from FILENAME. The new thread may be scheduled (and may even exit) @@ -51,36 +51,53 @@ process_execute (const char *file_name) return PID_ERROR; strlcpy (fn_copy, file_name, PGSIZE); - /* Parse argument into the structure. */ - args = (struct arguments *) malloc (sizeof (struct arguments)); - argument_parser (fn_copy, args); + /* Parse arguments into the structure. */ + args = parse_arguments (fn_copy); + if (args == NULL) + { + free (args); + palloc_free_page (fn_copy); + return PID_ERROR; + } /* Create a new thread to execute the given file name. */ pid = (pid_t) thread_create (args->argv[0], PRI_DEFAULT, start_process, args); if (pid == TID_ERROR) { - free(args->argv); + palloc_free_page (args->argv); free(args); palloc_free_page (fn_copy); } return pid; } -/* Parse arguments from the given string by replacing whole - adjacent spaces to the null character. Then store it in - given argument struct by pointer. */ -static void -argument_parser (char *str_input, struct arguments *args) +/* Parses arguments from the given string by replacing whole + adjacent spaces to the null character. Then stores it in + given arguments struct. Returns the pointer of arguments + struct if successful, NULL otherwise. */ +static struct arguments * +parse_arguments (char *str_input) { + struct arguments *args; char *leftover; char *curr; + args = (struct arguments *) malloc (sizeof (struct arguments)); + if (args == NULL) + return NULL; + args->argc = 0; - args->argv = (char **) calloc (strlen (str_input) + 1, sizeof (char)); - + args->argv = (char **) palloc_get_page (0); + if (args->argv == NULL) + { + free (args); + return NULL; + } + for (curr = strtok_r (str_input, " ", &leftover); curr != NULL; curr = strtok_r (NULL, " ", &leftover)) args->argv[args->argc ++] = curr; + return args; } /* A thread function that loads a user process and makes it start @@ -89,7 +106,7 @@ static void start_process (void *arguments) { struct arguments *args = arguments; - struct file *exec_file; + struct file *exec_file = NULL; struct intr_frame if_; bool success; @@ -102,21 +119,25 @@ start_process (void *arguments) /* Save load result. */ struct process *curr = process_current (); - if (success) - curr->info->status |= PROCESS_RUNNING; - else - curr->info->status |= PROCESS_FAIL; + if (curr->info != NULL) + { + if (success) + curr->info->status |= PROCESS_RUNNING; + else + curr->info->status |= PROCESS_FAIL; + } curr->exec_file = exec_file; curr->fd_next = FD_MIN; list_init (&curr->file_list); - /* If load failed, quit. */ + /* Free resources. */ + palloc_free_page (args->argv[0]); + palloc_free_page (args->argv); + free (args); + + /* If load failed, quit. */ if (!success) - { - free(args->argv); - free(args); - thread_exit (); - } + thread_exit (); /* Start the user process by simulating a return from an interrupt, implemented by intr_exit (in @@ -160,16 +181,21 @@ process_exit (void) /* Inform exit to child processes. */ struct list_elem *e; - for (e = list_begin (&proc->child_list); e != list_end (&proc->child_list); - e = list_next (e)) + for (e = list_begin (&proc->child_list); e != list_end (&proc->child_list);) { struct process_info *child = list_entry (e, struct process_info, elem); if (!(child->status & PROCESS_EXIT)) - child->process->parent = NULL; + { + child->process->parent = NULL; + child->process->info = NULL; + } + e = list_next (e); + free (child); } /* Update the process status and free resources. */ - proc->info->status |= PROCESS_EXIT; + if (proc->info != NULL) + proc->info->status |= PROCESS_EXIT; file_close (proc->exec_file); for (e = list_begin (&proc->file_list); e != list_end (&proc->file_list);) { @@ -336,12 +362,12 @@ struct Elf32_Phdr #define PF_W 2 /* Writable. */ #define PF_R 4 /* Readable. */ -static bool setup_stack (void **esp); +static bool setup_stack (struct arguments *args, void **esp); static bool validate_segment (const struct Elf32_Phdr *, struct file *); static bool load_segment (struct file *file, off_t ofs, uint8_t *upage, uint32_t read_bytes, uint32_t zero_bytes, bool writable); -static void push_args_on_stack(struct arguments *args, void **esp); +static void *push_args_on_stack(struct arguments *args); /* Loads an ELF executable from FILE_NAME into the current thread. Stores the executable's entry point into *EIP @@ -448,12 +474,9 @@ load (struct arguments *args, void (**eip) (void), void **esp, } /* Set up stack. */ - if (!setup_stack (esp)) + if (!setup_stack (args, esp)) goto fail; - /* Push arguments on stack. */ - push_args_on_stack (args, esp); - /* Start address. */ *eip = (void (*) (void)) ehdr.e_entry; @@ -579,7 +602,7 @@ load_segment (struct file *file, off_t ofs, uint8_t *upage, /* Create a minimal stack by mapping a zeroed page at the top of user virtual memory. */ static bool -setup_stack (void **esp) +setup_stack (struct arguments *args, void **esp) { uint8_t *kpage; bool success = false; @@ -589,8 +612,12 @@ setup_stack (void **esp) { success = install_page (((uint8_t *) PHYS_BASE) - PGSIZE, kpage, true); if (success) - *esp = PHYS_BASE; - else + { + *esp = push_args_on_stack (args); + if (*esp == NULL) + success = false; + } + if (!success) palloc_free_page (kpage); } return success; @@ -616,14 +643,15 @@ install_page (void *upage, void *kpage, bool writable) && pagedir_set_page (t->pagedir, upage, kpage, writable)); } -/* Push arguments on newly initialized stack, following calling conventions. - This invention does not use esp dynamically, just making it point bottom of - stack frame after all push is done.*/ -static void -push_args_on_stack(struct arguments *args, void **esp) +/* Push arguments on newly initialized stack. Returns pointer + that ESP should point to if successful, NULL otherwise. */ +static void * +push_args_on_stack(struct arguments *args) { uint32_t **arg_ptrs = (uint32_t **) calloc (args->argc, sizeof (uint32_t *)); - uint8_t *curr8 = (uint8_t *) *esp; + if (arg_ptrs == NULL) + return NULL; + uint8_t *curr8 = (uint8_t *) PHYS_BASE; int i; /* Push arguments on stack, save their address in arg_ptrs. */ @@ -649,7 +677,6 @@ push_args_on_stack(struct arguments *args, void **esp) until a pointer to argv[0] is pushed. */ for (i = args->argc - 1; i >= 0; i--) *--curr32 = arg_ptrs[i]; - free (arg_ptrs); /* Push address of argv on stack. */ *--curr32 = (uint32_t) (curr32 + 1); @@ -660,6 +687,7 @@ push_args_on_stack(struct arguments *args, void **esp) /* Push return address as 0 values. */ *--curr32 = 0; - /* ESP pointing bottom of the stack. */ - *esp = (void *) curr32; + /* Return result. */ + free (arg_ptrs); + return curr32; } diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 9cba93b..729b31a 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -174,7 +174,8 @@ void syscall_exit (int status) { /* Set exit code. */ - process_current ()->info->exit_code = status; + if (process_current ()->info != NULL) + process_current ()->info->exit_code = status; /* Print the termination message. */ printf ("%s: exit(%d)\n", thread_current ()->name, status);