From 9a4ea2007ff4a42601e2ed5beebf1c37c32b6157 Mon Sep 17 00:00:00 2001
From: Jakob Unterwurzacher <jakobunt@gmail.com>
Date: Sat, 22 Jul 2017 20:53:11 +0200
Subject: [PATCH] withFileNode: move canary check to its own function

The do_op closure is complicated enough. Get the canary check out
of the way to not make it even more complicated.

Also, use an explicitely atomic type for the canary.
---
 encfs/Context.cpp |  2 ++
 encfs/FileNode.h  |  5 ++++-
 encfs/encfs.cpp   | 30 ++++++++++++++++++++----------
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/encfs/Context.cpp b/encfs/Context.cpp
index 4557884..b4153c2 100644
--- a/encfs/Context.cpp
+++ b/encfs/Context.cpp
@@ -114,6 +114,8 @@ FileNode *EncFS_Context::putNode(const char *path,
   return list.front().get();
 }
 
+// eraseNode is called by encfs_release in response to the RELEASE
+// FUSE-command we get from the kernel.
 void EncFS_Context::eraseNode(const char *path, FileNode *pl) {
   Lock lock(contextMutex);
 
diff --git a/encfs/FileNode.h b/encfs/FileNode.h
index 017cbee..0ff677b 100644
--- a/encfs/FileNode.h
+++ b/encfs/FileNode.h
@@ -27,6 +27,7 @@
 #include <stdint.h>
 #include <string>
 #include <sys/types.h>
+#include <atomic>
 
 #include "CipherKey.h"
 #include "FSConfig.h"
@@ -49,7 +50,9 @@ class FileNode {
            const char *cipherName);
   ~FileNode();
 
-  uint32_t canary;
+  // Use an atomic type. The canary is accessed without holding any
+  // locks.
+  std::atomic<std::uint32_t> canary;
 
   const char *plaintextName() const;
   const char *cipherName() const;
diff --git a/encfs/encfs.cpp b/encfs/encfs.cpp
index 029e83f..6aee9dd 100644
--- a/encfs/encfs.cpp
+++ b/encfs/encfs.cpp
@@ -111,6 +111,25 @@ static int withCipherPath(const char *opName, const char *path,
   return res;
 }
 
+static void checkCanary(std::shared_ptr<FileNode> fnode) {
+  if(fnode->canary == CANARY_OK) {
+    return;
+  }
+  if(fnode->canary == CANARY_RELEASED) {
+    // "fnode" may have been released after it was retrieved by
+    // lookupFuseFh. This is not an error. std::shared_ptr will release
+    // the memory only when all operations on the FileNode have been
+    // completed.
+    return;
+  }
+  if(fnode->canary == CANARY_DESTROYED) {
+    RLOG(ERROR) << "canary=CANARY_DESTROYED. FileNode accessed after it was destroyed.";
+  } else {
+    RLOG(ERROR) << "canary=0x" << std::hex << fnode->canary << ". Memory corruption?";
+  }
+  throw Error("dead canary");
+}
+
 // helper function -- apply a functor to a node
 static int withFileNode(const char *opName, const char *path,
                         struct fuse_file_info *fi,
@@ -125,16 +144,7 @@ static int withFileNode(const char *opName, const char *path,
 
     auto do_op = [&FSRoot, opName, &op](FileNode *fnode) {
       rAssert(fnode != nullptr);
-      if(fnode->canary != CANARY_OK) {
-        if(fnode->canary == CANARY_RELEASED) {
-          RLOG(ERROR) << "canary=CANARY_RELEASED. File node accessed after it was released.";
-        } else if(fnode->canary == CANARY_DESTROYED) {
-          RLOG(ERROR) << "canary=CANARY_DESTROYED. File node accessed after it was destroyed.";
-        } else {
-          RLOG(ERROR) << "canary=0x" << std::hex << fnode->canary << ". Corruption?";
-        }
-        throw Error("dead canary");
-      }
+      checkCanary(fnode);
       VLOG(1) << "op: " << opName << " : " << fnode->cipherName();
 
       // check that we're not recursing into the mount point itself