From f53503438c28910a36fe2b155d6623c9af41d951 Mon Sep 17 00:00:00 2001
From: Andrew Gaul <andrew@gaul.org>
Date: Mon, 4 Mar 2019 17:57:03 +0900
Subject: [PATCH] Increase FdEntity reference count when returning

Previously s3fs had a race condition where one client could delete
FdEntity that another client was using.  Add a simple concurrent test
which previously failed but now succeeds.  Fixes #964.
---
 src/fdcache.cpp               | 11 +++++++++++
 src/fdcache.h                 |  1 +
 src/s3fs.cpp                  |  3 +++
 test/integration-test-main.sh | 15 +++++++++++++++
 4 files changed, 30 insertions(+)

diff --git a/src/fdcache.cpp b/src/fdcache.cpp
index 0c8cd94..557052a 100644
--- a/src/fdcache.cpp
+++ b/src/fdcache.cpp
@@ -700,6 +700,9 @@ void FdEntity::Close()
 
     if(0 < refcnt){
       refcnt--;
+    }else{
+      S3FS_PRN_EXIT("reference count underflow");
+      abort();
     }
     if(0 == refcnt){
       if(!cachepath.empty()){
@@ -2058,6 +2061,7 @@ FdEntity* FdManager::GetFdEntity(const char* path, int existfd)
 
   fdent_map_t::iterator iter = fent.find(string(path));
   if(fent.end() != iter && (-1 == existfd || (*iter).second->GetFd() == existfd)){
+    iter->second->Dup();
     return (*iter).second;
   }
 
@@ -2066,6 +2070,7 @@ FdEntity* FdManager::GetFdEntity(const char* path, int existfd)
       if((*iter).second && (*iter).second->GetFd() == existfd){
         // found opened fd in map
         if(0 == strcmp((*iter).second->GetPath(), path)){
+          iter->second->Dup();
           return (*iter).second;
         }
         // found fd, but it is used another file(file descriptor is recycled)
@@ -2084,6 +2089,7 @@ FdEntity* FdManager::Open(const char* path, headers_t* pmeta, ssize_t size, time
   if(!path || '\0' == path[0]){
     return NULL;
   }
+  bool close = false;
   FdEntity* ent;
   {
     AutoLock auto_lock(&FdManager::fd_manager_lock);
@@ -2107,6 +2113,8 @@ FdEntity* FdManager::Open(const char* path, headers_t* pmeta, ssize_t size, time
     if(fent.end() != iter){
       // found
       ent = (*iter).second;
+      ent->Dup();
+      close = true;
 
     }else if(is_create){
       // not found
@@ -2142,6 +2150,9 @@ FdEntity* FdManager::Open(const char* path, headers_t* pmeta, ssize_t size, time
   if(0 != ent->Open(pmeta, size, time, no_fd_lock_wait)){
     return NULL;
   }
+  if(close){
+    ent->Close();
+  }
   return ent;
 }
 
diff --git a/src/fdcache.h b/src/fdcache.h
index 2e9622e..84d6f56 100644
--- a/src/fdcache.h
+++ b/src/fdcache.h
@@ -225,6 +225,7 @@ class FdManager
     static void FreeReservedDiskSpace(size_t size);
     bool ReserveDiskSpace(size_t size);
 
+    // Return FdEntity associated with path, returning NULL on error.  This operation increments the reference count; callers must decrement via Close after use.
     FdEntity* GetFdEntity(const char* path, int existfd = -1);
     FdEntity* Open(const char* path, headers_t* pmeta = NULL, ssize_t size = -1, time_t time = -1, bool force_tmpfile = false, bool is_create = true, bool no_fd_lock_wait = false);
     FdEntity* ExistOpen(const char* path, int existfd = -1, bool ignore_existfd = false);
diff --git a/src/s3fs.cpp b/src/s3fs.cpp
index 6438b56..6353b1d 100644
--- a/src/s3fs.cpp
+++ b/src/s3fs.cpp
@@ -2332,6 +2332,9 @@ static int s3fs_release(const char* _path, struct fuse_file_info* fi)
   if(ent->GetFd() != static_cast<int>(fi->fh)){
     S3FS_PRN_WARN("different fd(%d - %llu)", ent->GetFd(), (unsigned long long)(fi->fh));
   }
+
+  // Once for the implicit refcnt from GetFdEntity and again for release
+  ent->Close();
   FdManager::get()->Close(ent);
 
   // check - for debug
diff --git a/test/integration-test-main.sh b/test/integration-test-main.sh
index 764c74c..96d1feb 100755
--- a/test/integration-test-main.sh
+++ b/test/integration-test-main.sh
@@ -522,6 +522,20 @@ function test_overwrite_existing_file_range {
     rm -f ${TEST_TEXT_FILE}
 }
 
+function test_concurrency {
+    for i in `seq 10`; do echo foo > $i; done
+    for process in `seq 2`; do
+        for i in `seq 100`; do
+            file=$(ls | sed -n "$(($RANDOM % 10 + 1)){p;q}")
+            cat $file >/dev/null || true
+            rm -f $file
+            echo foo > $i || true
+        done &
+    done
+    wait
+}
+
+
 function add_all_tests {
     add_tests test_append_file 
     add_tests test_truncate_file 
@@ -546,6 +560,7 @@ function add_all_tests {
     add_tests test_rm_rf_dir
     add_tests test_write_after_seek_ahead
     add_tests test_overwrite_existing_file_range
+    add_tests test_concurrency
 }
 
 init_suite