Reassign FD

Modify AssignFD to Free then reassign, so that the Free path
happens atomically in the function.

See: https://codereview.chromium.org/12207031/

R=binji@chromium.org
NOTRY=true

Review URL: https://codereview.chromium.org/12217098

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@181623 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 28 insertions, 28 deletions

diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_object.cc b/native_client_sdk/src/libraries/nacl_io/kernel_object.cc
index f4ae9d5..fca4041 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_object.cc
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_object.cc
@@ -129,19 +129,30 @@ int KernelObject::AllocateFD(KernelHandle* handle) {
   return id;
 }
 
-void KernelObject::AssignFD(int fd, KernelHandle* handle) {
-  AutoLock lock(&process_lock_);
-
-  // Acquire the handle and its mount since we are about to track it with
-  // this FD.
-  handle->Acquire();
-  handle->mount_->Acquire();
+void KernelObject::FreeAndReassignFD(int fd, KernelHandle* handle) {
+  if (NULL == handle) {
+    FreeFD(fd);
+  } else {
+    AutoLock lock(&process_lock_);
 
-  if (fd >= handle_map_.size())
-    handle_map_.resize(fd + 1);
+    // Acquire the new handle first incase they are the same.
+    if (handle) {
+      handle->Acquire();
+      handle->mount_->Acquire();
+    }
 
-  assert(handle_map_[fd] == NULL);
-  handle_map_[fd] = handle;
+    // If the required FD is larger than the current set, grow the set
+    if (fd >= handle_map_.size()) {
+      handle_map_.resize(fd + 1);
+    } else {
+      KernelHandle* old_handle = handle_map_[fd];
+      if (NULL != old_handle) {
+        old_handle->mount_->Release();
+        old_handle->Release();
+      }
+    }
+    handle_map_[fd] = handle;
+  }
 }
 
 void KernelObject::FreeFD(int fd) {
diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_object.h b/native_client_sdk/src/libraries/nacl_io/kernel_object.h
index fc02c3f..f4c1b9e 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_object.h
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_object.h
@@ -38,7 +38,7 @@ class KernelObject {
   // Allocate a new fd and assign the handle to it, while
   // ref counting the handle and associated mount.
   int AllocateFD(KernelHandle* handle);
-  void AssignFD(int fd, KernelHandle* handle);
+  void FreeAndReassignFD(int fd, KernelHandle* handle);
   void FreeFD(int fd);
 
  protected:
diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc b/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc
index 77ff3f9..71a0e65 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc
@@ -124,19 +124,7 @@ int KernelProxy::dup2(int oldfd, int newfd) {
   KernelHandle* old_handle = AcquireHandle(oldfd);
   if (NULL == old_handle) return -1;
 
-  KernelHandle* new_handle = AcquireHandle(newfd);
-  if (NULL != new_handle) {
-    if (old_handle != new_handle) {
-      // Release the copy held by the descriptopr
-      ReleaseHandle(new_handle);
-      // Reassign the descriptor to the old handle
-      AssignFD(newfd, old_handle);
-    }
-
-    // Release this acquired copy
-    ReleaseHandle(new_handle);
-  }
-
+  FreeAndReassignFD(newfd, old_handle);
   ReleaseHandle(old_handle);
   return newfd;
 }
diff --git a/native_client_sdk/src/libraries/nacl_io_test/kernel_object_test.cc b/native_client_sdk/src/libraries/nacl_io_test/kernel_object_test.cc
index e2c9f39..01b0573 100644
--- a/native_client_sdk/src/libraries/nacl_io_test/kernel_object_test.cc
+++ b/native_client_sdk/src/libraries/nacl_io_test/kernel_object_test.cc
@@ -154,7 +154,7 @@ TEST_F(KernelObjectTest, Referencing) {
   EXPECT_EQ(0, mount_count);
 }
 
-TEST_F(KernelObjectTest, AssignFD) {
+TEST_F(KernelObjectTest, FreeAndReassignFD) {
   EXPECT_EQ(0, handle_count);
 
   KernelHandle* handle = new KernelHandleRefMock(mnt, NULL, 0);
@@ -162,7 +162,8 @@ TEST_F(KernelObjectTest, AssignFD) {
   EXPECT_EQ(1, handle_count);
   EXPECT_EQ(1, handle->RefCount());
 
-  proxy->AssignFD(2, handle);
+  // Assign to a non-existant FD
+  proxy->FreeAndReassignFD(2, handle);
   EXPECT_EQ((KernelHandle*)NULL, proxy->AcquireHandle(0));
   EXPECT_EQ((KernelHandle*)NULL, proxy->AcquireHandle(1));
   EXPECT_EQ(handle, proxy->AcquireHandle(2));
@@ -171,7 +172,7 @@ TEST_F(KernelObjectTest, AssignFD) {
   EXPECT_EQ(1, handle_count);
   EXPECT_EQ(2, handle->RefCount());
 
-  proxy->AssignFD(0, handle);
+  proxy->FreeAndReassignFD(0, handle);
   EXPECT_EQ(handle, proxy->AcquireHandle(0));
   EXPECT_EQ((KernelHandle*)NULL, proxy->AcquireHandle(1));
   EXPECT_EQ(handle, proxy->AcquireHandle(2));