fix: clear shared pages to avoid freeing them

src/userprog/exception.c

@@ -187,15 +187,11 @@ try_fetch_page (void *upage, bool write)
   bool success = false;
   switch (page->type) {
     case PAGE_EXECUTABLE:
-      success = page_load (page, write);
+      success = page_load (page);
       break;
     default:
       return false;
-  }
+    }
-
-  if (success && write &&
-      !pagedir_is_writable(thread_current()->pagedir, upage))
-    pagedir_set_writable(thread_current()->pagedir, upage, true);
 
   return success;
 }

src/vm/page.c

@@ -5,6 +5,7 @@
 #include "threads/malloc.h"
 #include "threads/palloc.h"
 #include "threads/vaddr.h"
+#include "userprog/pagedir.h"
 #include "userprog/process.h"
 #include "vm/frame.h"
 
@@ -41,18 +42,28 @@ struct page_entry *
 page_insert (struct file *file, off_t ofs, void *upage, uint32_t read_bytes,
              uint32_t zero_bytes, bool writable, enum page_type type)
 {
+  /* If page exists, just update it. */
+  struct page_entry *existing = page_get (upage);
+  if (existing != NULL)
+    {
+      existing->read_bytes = read_bytes;
+      existing->zero_bytes = zero_bytes;
+      existing->writable = writable;
+      return existing;
+    }
+
+  /* Otherwise allocate a new one. */
   struct page_entry *page = malloc(sizeof (struct page_entry));
   if (page == NULL)
     return NULL;
-
   page->file = file;
   page->offset = ofs;
   page->upage = upage;
   page->read_bytes = read_bytes;
   page->zero_bytes = zero_bytes;
   page->writable = writable;
+  page->shared = false;
   page->type = type;
-
   hash_insert (&thread_current ()->pages, &page->elem);
   return page;
 }
@@ -75,20 +86,23 @@ page_get (void *upage)
 }
 
 bool
-page_load (struct page_entry *page, bool writable)
+page_load (struct page_entry *page)
 {
   /* If the page is read-only, we want to check if it is a shared page already
      loaded into memory. If it is, we can just map the page to the frame. */
-  if (!page->writable && !writable)
+  if (!page->writable)
     {
+      lock_acquire (&shared_files_lock);
       struct shared_page_entry *shared_page =
         shared_page_get (page->file, page->upage);
 
+      /* Mark page as shared and install the shared frame. */
       if (shared_page != NULL)
         {
-          /* Map the page to the shared frame for this read-only portion. */
+          lock_release (&shared_files_lock);
           if (!install_page (page->upage, shared_page->frame, false))
             return false;
+          page->shared = true;
           return true;
         }
     }
@@ -100,22 +114,20 @@ page_load (struct page_entry *page, bool writable)
   if (frame == NULL)
     PANIC ("Could not allocate a frame to load page into memory.");
 
+  /* Ensure page is not marked as shared while it doesn't exist in the
+     shared_files table, to avoid memory leaks. */
+  page->shared = false;
+
   /* Map the page to the frame. */
   if (!install_page (page->upage, frame, page->writable))
-    {
+    goto fail;
-      frame_free (frame);
-      return false;
-    }
 
   /* Move the file pointer to the correct location in the file. Then, read the
      data from the file into the frame. Checks that we were able to read the
      expected number of bytes. */
   file_seek (page->file, page->offset);
   if (file_read (page->file, frame, page->read_bytes) != (int) page->read_bytes)
-  {
+    goto fail;
-    frame_free (frame);
-    return false;
-  }
 
   /* Zero out the remaining bytes in the frame. */
   memset (frame + page->read_bytes, 0, page->zero_bytes);
@@ -123,15 +135,22 @@ page_load (struct page_entry *page, bool writable)
   /* If the page is read-only, we need to add it to the shared pages table. */
   if (!page->writable)
     {
-      if (shared_page_insert (page->file, page->upage, frame) == NULL)
+      struct shared_page_entry *shared_page
-        {
+          = shared_page_insert (page->file, page->upage, frame);
-          frame_free (frame);
+      if (shared_page == NULL)
-          return false;
+        goto fail;
-        }
+      lock_release (&shared_files_lock);
+      page->shared = true;
     }
 
   /* Mark the page as loaded successfully. */
   return true;
+
+fail:
+  if (!page->writable)
+    lock_release (&shared_files_lock);
+  frame_free (frame);
+  return false;
 }
 
 /* Function to clean up a page_entry. Given the elem of that page_entry, frees
@@ -139,7 +158,13 @@ page_load (struct page_entry *page, bool writable)
 void
 page_cleanup (struct hash_elem *e, void *aux UNUSED)
 {
-  free (hash_entry (e, struct page_entry, elem));
+  struct page_entry *page = hash_entry (e, struct page_entry, elem);
+
+  /* If page is shared then mark it as not present to avoid it being freed. */
+  uint32_t *pd = thread_current ()->pagedir;
+  if (pd != NULL && page->shared)
+    pagedir_clear_page (pd, page->upage);
+  free (page);
 }
 
 /* Hashing function needed for the shared_file table. Returns a hash for an

src/vm/page.h

@@ -23,6 +23,7 @@ struct page_entry {
   uint32_t read_bytes; /* Number of bytes to read within the page. */
   uint32_t zero_bytes; /* Number of bytes to zero within the page. */
   bool writable;       /* Flag for whether this page is writable or not. */
+  bool shared;         /* Flag for whether this page is shared or not. */
 
   struct hash_elem elem; /* An elem for the hash table. */
 };
@@ -49,7 +50,7 @@ struct page_entry *page_insert (struct file *file, off_t ofs, void *upage,
                                 uint32_t read_bytes, uint32_t zero_bytes,
                                 bool writable, enum page_type type);
 struct page_entry *page_get (void *upage);
-bool page_load (struct page_entry *page, bool writable);
+bool page_load (struct page_entry *page);
 void page_cleanup (struct hash_elem *e, void *aux);
 
 unsigned shared_file_hash (const struct hash_elem *e, void *aux);