From 84174c560d7542436067dfbfe1f697368ad7d4a1 Mon Sep 17 00:00:00 2001
From: Andrew Gaul <andrew@gaul.org>
Date: Sat, 29 May 2021 00:11:55 +0900
Subject: [PATCH] Fix data races caused by incorrect locking (#1668)

Found via Threadsanitizer.  Fixes #1471.
---
 src/fdcache.cpp        |  2 +-
 src/fdcache_entity.cpp | 27 +++++++++++++--------------
 src/fdcache_entity.h   |  5 +++--
 src/s3fs.cpp           |  2 +-
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/fdcache.cpp b/src/fdcache.cpp
index 5d106cc..b02d712 100644
--- a/src/fdcache.cpp
+++ b/src/fdcache.cpp
@@ -512,7 +512,7 @@ FdEntity* FdManager::Open(const char* path, headers_t* pmeta, off_t size, time_t
     }
 
     // open
-    if(0 != ent->Open(pmeta, size, time, no_fd_lock_wait)){
+    if(0 != ent->Open(pmeta, size, time, no_fd_lock_wait ? AutoLock::NO_WAIT : AutoLock::NONE)){
         if(close){
             ent->Close();
         }
diff --git a/src/fdcache_entity.cpp b/src/fdcache_entity.cpp
index 67fbd36..b328dd4 100644
--- a/src/fdcache_entity.cpp
+++ b/src/fdcache_entity.cpp
@@ -310,9 +310,9 @@ int FdEntity::OpenMirrorFile()
     return mirrorfd;
 }
 
-int FdEntity::Open(headers_t* pmeta, off_t size, time_t time, bool no_fd_lock_wait)
+int FdEntity::Open(headers_t* pmeta, off_t size, time_t time, AutoLock::Type type)
 {
-    AutoLock auto_lock(&fdent_lock, no_fd_lock_wait ? AutoLock::NO_WAIT : AutoLock::NONE);
+    AutoLock auto_lock(&fdent_lock, type);
 
     S3FS_PRN_DBG("[path=%s][fd=%d][size=%lld][time=%lld]", path.c_str(), fd, static_cast<long long>(size), static_cast<long long>(time));
 
@@ -554,7 +554,7 @@ bool FdEntity::OpenAndLoadAll(headers_t* pmeta, off_t* size, bool force_load)
     S3FS_PRN_INFO3("[path=%s][fd=%d]", path.c_str(), fd);
 
     if(-1 == fd){
-        if(0 != Open(pmeta)){
+        if(0 != Open(pmeta, /*size=*/ -1, /*time=*/ -1, AutoLock::ALREADY_LOCKED)){
             return false;
         }
     }
@@ -1178,7 +1178,8 @@ int FdEntity::NoCacheCompleteMultipartPost()
     return 0;
 }
 
-off_t FdEntity::BytesModified() const {
+off_t FdEntity::BytesModified() {
+    AutoLock auto_lock(&fdent_data_lock);
     return pagelist.BytesModified();
 }
 
@@ -1186,20 +1187,16 @@ int FdEntity::RowFlush(const char* tpath, bool force_sync)
 {
     int result = 0;
 
-    std::string tmppath;
-    headers_t tmporgmeta;
-    {
-        AutoLock auto_lock(&fdent_lock);
-        tmppath = path;
-        tmporgmeta = orgmeta;
-    }
+    AutoLock auto_lock(&fdent_lock);
+    std::string tmppath = path;
+    headers_t tmporgmeta = orgmeta;
 
     S3FS_PRN_INFO3("[tpath=%s][path=%s][fd=%d]", SAFESTRPTR(tpath), tmppath.c_str(), fd);
 
     if(-1 == fd){
         return -EBADF;
     }
-    AutoLock auto_lock(&fdent_data_lock);
+    AutoLock auto_lock2(&fdent_data_lock);
 
     if(!force_sync && !pagelist.IsModified()){
         // nothing to update.
@@ -1394,7 +1391,8 @@ ssize_t FdEntity::Read(char* bytes, off_t start, size_t size, bool force_load)
     if(-1 == fd){
         return -EBADF;
     }
-    AutoLock auto_lock(&fdent_data_lock);
+    AutoLock auto_lock(&fdent_lock);
+    AutoLock auto_lock2(&fdent_data_lock);
 
     if(force_load){
         pagelist.SetPageLoadedStatus(start, size, PageList::PAGE_NOT_LOAD_MODIFIED);
@@ -1458,7 +1456,8 @@ ssize_t FdEntity::Write(const char* bytes, off_t start, size_t size)
     if(FdManager::IsCacheDir() && !FdManager::IsSafeDiskSpace(NULL, size)){
         FdManager::get()->CleanupCacheDir();
     }
-    AutoLock auto_lock(&fdent_data_lock);
+    AutoLock auto_lock(&fdent_lock);
+    AutoLock auto_lock2(&fdent_data_lock);
 
     // check file size
     if(pagelist.Size() < start){
diff --git a/src/fdcache_entity.h b/src/fdcache_entity.h
index 276a5d2..971877f 100644
--- a/src/fdcache_entity.h
+++ b/src/fdcache_entity.h
@@ -21,6 +21,7 @@
 #ifndef S3FS_FDCACHE_ENTITY_H_
 #define S3FS_FDCACHE_ENTITY_H_
 
+#include "autolock.h"
 #include "fdcache_page.h"
 #include "metaheader.h"
 
@@ -74,7 +75,7 @@ class FdEntity
 
         void Close();
         bool IsOpen() const { return (-1 != fd); }
-        int Open(headers_t* pmeta = NULL, off_t size = -1, time_t time = -1, bool no_fd_lock_wait = false);
+        int Open(headers_t* pmeta, off_t size, time_t time, AutoLock::Type type);
         bool OpenAndLoadAll(headers_t* pmeta = NULL, off_t* size = NULL, bool force_load = false);
         int Dup(bool lock_already_held = false);
         int GetRefCnt() const { return refcnt; }                // [NOTE] Use only debugging
@@ -109,7 +110,7 @@ class FdEntity
         int NoCacheMultipartPost(int tgfd, off_t start, off_t size);
         int NoCacheCompleteMultipartPost();
 
-        off_t BytesModified() const;
+        off_t BytesModified();
         int RowFlush(const char* tpath, bool force_sync = false);
         int Flush(bool force_sync = false) { return RowFlush(NULL, force_sync); }
 
diff --git a/src/s3fs.cpp b/src/s3fs.cpp
index 1132444..fc93708 100644
--- a/src/s3fs.cpp
+++ b/src/s3fs.cpp
@@ -2292,7 +2292,7 @@ static int s3fs_open(const char* _path, struct fuse_file_info* fi)
     if (needs_flush){
         time_t now = time(NULL);
         struct timespec ts = {now, 0};
-        ent->SetMCtime(ts, ts, true);
+        ent->SetMCtime(ts, ts);
         if(0 != (result = ent->RowFlush(path, true))){
             S3FS_PRN_ERR("could not upload file(%s): result=%d", path, result);
             StatCache::getStatCacheData()->DelStat(path);