use acquire/release to force order for pid=np->pid;np->state=RUNNING

for bug reported by [email protected] and [email protected]

3 files changed, 19 insertions, 11 deletions

diff --git a/TRICKS b/TRICKS
index 0338279..8d1439f 100644
--- a/TRICKS
+++ b/TRICKS
@@ -116,21 +116,25 @@ processors will need it.
 ---
 
 The code in fork needs to read np->pid before
-setting np->state to RUNNABLE.  
+setting np->state to RUNNABLE.  The following
+is not a correct way to do this:
 
 	int
 	fork(void)
 	{
 	  ...
-	  pid = np->pid;
 	  np->state = RUNNABLE;
-	  return pid;
+	  return np->pid; // oops
 	}
 
 After setting np->state to RUNNABLE, some other CPU
 might run the process, it might exit, and then it might
 get reused for a different process (with a new pid), all
-before the return statement.  So it's not safe to just do
-"return np->pid;".
-
-This works because proc.h marks the pid as volatile.
+before the return statement.  So it's not safe to just
+"return np->pid". Even saving a copy of np->pid before
+setting np->state isn't safe, since the compiler is
+allowed to re-order statements.
+
+The real code saves a copy of np->pid, then acquires a lock
+around the write to np->state. The acquire() prevents the
+compiler from re-ordering.
diff --git a/proc.c b/proc.c
index bcdbfea..e19539c 100644
--- a/proc.c
+++ b/proc.c
@@ -153,10 +153,16 @@ fork(void)
     if(proc->ofile[i])
       np->ofile[i] = filedup(proc->ofile[i]);
   np->cwd = idup(proc->cwd);
+
+  safestrcpy(np->name, proc->name, sizeof(proc->name));
  
   pid = np->pid;
+
+  // lock to force the compiler to emit the np->state write last.
+  acquire(&ptable.lock);
   np->state = RUNNABLE;
-  safestrcpy(np->name, proc->name, sizeof(proc->name));
+  release(&ptable.lock);
+  
   return pid;
 }
 
@@ -455,5 +461,3 @@ procdump(void)
     cprintf("\n");
   }
 }
-
-
diff --git a/proc.h b/proc.h
index 6561ad3..3b9c3ac 100644
--- a/proc.h
+++ b/proc.h
@@ -57,7 +57,7 @@ struct proc {
   pde_t* pgdir;                // Page table
   char *kstack;                // Bottom of kernel stack for this process
   enum procstate state;        // Process state
-  volatile int pid;            // Process ID
+  int pid;                     // Process ID
   struct proc *parent;         // Parent process
   struct trapframe *tf;        // Trap frame for current syscall
   struct context *context;     // swtch() here to run process