Merge "Create test directory with the correct permissions." into rvc-dev
diff --git a/OWNERS b/OWNERS
index cde070b..9156e6b 100644
--- a/OWNERS
+++ b/OWNERS
@@ -2,4 +2,5 @@
maco@google.com
marcone@google.com
nandana@google.com
+shafik@google.com
zezeozue@google.com
diff --git a/jni/FuseDaemon.cpp b/jni/FuseDaemon.cpp
index f960a9a..42fdd05 100644
--- a/jni/FuseDaemon.cpp
+++ b/jni/FuseDaemon.cpp
@@ -235,15 +235,14 @@
const size_t target_ = 32 * 1024 * 1024;
};
-// Whether inode tracking is enabled or not. When enabled, we maintain a
-// separate mapping from inode numbers to "live" nodes so we can detect when
-// we receive a request to a node that has been deleted.
-static constexpr bool kEnableInodeTracking = true;
-
/* Single FUSE mount */
struct fuse {
explicit fuse(const std::string& _path)
- : path(_path), root(node::CreateRoot(_path, &lock)), mp(0), zero_addr(0) {}
+ : path(_path),
+ tracker(mediaprovider::fuse::NodeTracker(&lock)),
+ root(node::CreateRoot(_path, &lock, &tracker)),
+ mp(0),
+ zero_addr(0) {}
inline bool IsRoot(const node* node) const { return node == root; }
@@ -256,14 +255,7 @@
return root;
}
- node* node = node::FromInode(inode);
-
- if (kEnableInodeTracking) {
- std::lock_guard<std::recursive_mutex> guard(lock);
- CHECK(inode_tracker_.find(node) != inode_tracker_.end());
- }
-
- return node;
+ return node::FromInode(inode, &tracker);
}
inline __u64 ToInode(node* node) const {
@@ -274,28 +266,10 @@
return node::ToInode(node);
}
- // Notify this FUSE instance that one of its nodes has been deleted.
- void NodeDeleted(const node* node) {
- if (kEnableInodeTracking) {
- LOG(INFO) << "Node: " << node << " deleted.";
-
- std::lock_guard<std::recursive_mutex> guard(lock);
- inode_tracker_.erase(node);
- }
- }
-
- // Notify this FUSE instance that a new nodes has been created.
- void NodeCreated(const node* node) {
- if (kEnableInodeTracking) {
- LOG(INFO) << "Node: " << node << " created.";
-
- std::lock_guard<std::recursive_mutex> guard(lock);
- inode_tracker_.insert(node);
- }
- }
-
std::recursive_mutex lock;
const string path;
+ // The Inode tracker associated with this FUSE instance.
+ mediaprovider::fuse::NodeTracker tracker;
node* const root;
struct fuse_session* se;
@@ -313,8 +287,6 @@
/* const */ char* zero_addr;
FAdviser fadviser;
-
- std::unordered_set<const node*> inode_tracker_;
};
static inline string safe_name(node* n) {
@@ -413,8 +385,7 @@
node = parent->LookupChildByName(name, true /* acquire */);
if (!node) {
- node = ::node::Create(parent, name, &fuse->lock);
- fuse->NodeCreated(node);
+ node = ::node::Create(parent, name, &fuse->lock, &fuse->tracker);
}
TRACE_NODE(node);
@@ -502,9 +473,7 @@
// This is a narrowing conversion from an unsigned 64bit to a 32bit value. For
// some reason we only keep 32 bit refcounts but the kernel issues
// forget requests with a 64 bit counter.
- if (node->Release(static_cast<uint32_t>(nlookup))) {
- fuse->NodeDeleted(node);
- }
+ node->Release(static_cast<uint32_t>(nlookup));
}
}
diff --git a/jni/node-inl.h b/jni/node-inl.h
index 2aa3b13..5098316 100644
--- a/jni/node-inl.h
+++ b/jni/node-inl.h
@@ -24,6 +24,7 @@
#include <mutex>
#include <sstream>
#include <string>
+#include <unordered_set>
#include <vector>
#include "libfuse_jni/ReaddirHelper.h"
@@ -64,17 +65,70 @@
~dirhandle() { closedir(d); }
};
+// Whether inode tracking is enabled or not. When enabled, we maintain a
+// separate mapping from inode numbers to "live" nodes so we can detect when
+// we receive a request to a node that has been deleted.
+static constexpr bool kEnableInodeTracking = true;
+
+class node;
+
+// Tracks the set of active nodes associated with a FUSE instance so that we
+// can assert that we only ever return an active node in response to a lookup.
+class NodeTracker {
+ public:
+ NodeTracker(std::recursive_mutex* lock) : lock_(lock) {}
+
+ void CheckTracked(__u64 ino) const {
+ if (kEnableInodeTracking) {
+ const node* node = reinterpret_cast<const class node*>(ino);
+ std::lock_guard<std::recursive_mutex> guard(*lock_);
+ CHECK(active_nodes_.find(node) != active_nodes_.end());
+ }
+ }
+
+ void NodeDeleted(const node* node) {
+ if (kEnableInodeTracking) {
+ std::lock_guard<std::recursive_mutex> guard(*lock_);
+ LOG(DEBUG) << "Node: " << reinterpret_cast<uintptr_t>(node) << " deleted.";
+
+ CHECK(active_nodes_.find(node) != active_nodes_.end());
+ active_nodes_.erase(node);
+ }
+ }
+
+ void NodeCreated(const node* node) {
+ if (kEnableInodeTracking) {
+ std::lock_guard<std::recursive_mutex> guard(*lock_);
+ LOG(DEBUG) << "Node: " << reinterpret_cast<uintptr_t>(node) << " created.";
+
+ CHECK(active_nodes_.find(node) == active_nodes_.end());
+ active_nodes_.insert(node);
+ }
+ }
+
+ private:
+ std::recursive_mutex* lock_;
+ std::unordered_set<const node*> active_nodes_;
+};
+
class node {
public:
// Creates a new node with the specified parent, name and lock.
- static node* Create(node* parent, const std::string& name, std::recursive_mutex* lock) {
- return new node(parent, name, lock);
+ static node* Create(node* parent, const std::string& name, std::recursive_mutex* lock,
+ NodeTracker* tracker) {
+ // Place the entire constructor under a critical section to make sure
+ // node creation, tracking (if enabled) and the addition to a parent are
+ // atomic.
+ std::lock_guard<std::recursive_mutex> guard(*lock);
+ return new node(parent, name, lock, tracker);
}
// Creates a new root node. Root nodes have no parents by definition
// and their "name" must signify an absolute path.
- static node* CreateRoot(const std::string& path, std::recursive_mutex* lock) {
- node* root = new node(nullptr, path, lock);
+ static node* CreateRoot(const std::string& path, std::recursive_mutex* lock,
+ NodeTracker* tracker) {
+ std::lock_guard<std::recursive_mutex> guard(*lock);
+ node* root = new node(nullptr, path, lock, tracker);
// The root always has one extra reference to avoid it being
// accidentally collected.
@@ -83,7 +137,8 @@
}
// Maps an inode to its associated node.
- static inline node* FromInode(__u64 ino) {
+ static inline node* FromInode(__u64 ino, const NodeTracker* tracker) {
+ tracker->CheckTracked(ino);
return reinterpret_cast<node*>(static_cast<uintptr_t>(ino));
}
@@ -218,8 +273,14 @@
static const node* LookupAbsolutePath(const node* root, const std::string& absolute_path);
private:
- node(node* parent, const std::string& name, std::recursive_mutex* lock)
- : name_(name), refcount_(0), parent_(nullptr), deleted_(false), lock_(lock) {
+ node(node* parent, const std::string& name, std::recursive_mutex* lock, NodeTracker* tracker)
+ : name_(name),
+ refcount_(0),
+ parent_(nullptr),
+ deleted_(false),
+ lock_(lock),
+ tracker_(tracker) {
+ tracker_->NodeCreated(this);
Acquire();
// This is a special case for the root node. All other nodes will have a
// non-null parent.
@@ -287,11 +348,15 @@
bool deleted_;
std::recursive_mutex* lock_;
+ NodeTracker* const tracker_;
+
~node() {
RemoveFromParent();
handles_.clear();
dirhandles_.clear();
+
+ tracker_->NodeDeleted(this);
}
friend class ::NodeTest;
diff --git a/jni/node.cpp b/jni/node.cpp
index 6187774..e17a9e8 100644
--- a/jni/node.cpp
+++ b/jni/node.cpp
@@ -97,13 +97,15 @@
std::lock_guard<std::recursive_mutex> guard(*tree->lock_);
if (tree) {
- for (node* child : tree->children_) {
+ // Make a copy of the list of children because calling Delete tree
+ // will modify the list of children, which will cause issues while
+ // iterating over them.
+ std::vector<node*> children(tree->children_.begin(), tree->children_.end());
+ for (node* child : children) {
DeleteTree(child);
}
- tree->children_.clear();
- LOG(DEBUG) << "DELETE node " << tree->GetName();
- tree->RemoveFromParent();
+ CHECK(tree->children_.empty());
delete tree;
}
}
diff --git a/jni/node_test.cpp b/jni/node_test.cpp
index ca0405c..423af3d 100644
--- a/jni/node_test.cpp
+++ b/jni/node_test.cpp
@@ -9,15 +9,19 @@
using mediaprovider::fuse::dirhandle;
using mediaprovider::fuse::handle;
using mediaprovider::fuse::node;
+using mediaprovider::fuse::NodeTracker;
// Listed as a friend class to struct node so it can observe implementation
// details if required. The only implementation detail that is worth writing
// tests around at the moment is the reference count.
class NodeTest : public ::testing::Test {
public:
+ NodeTest() : tracker_(NodeTracker(&lock_)) {}
+
uint32_t GetRefCount(node* node) { return node->refcount_; }
std::recursive_mutex lock_;
+ NodeTracker tracker_;
// Forward destruction here, as NodeTest is a friend class.
static void destroy(node* node) { delete node; }
@@ -27,7 +31,7 @@
typedef std::unique_ptr<node, decltype(&NodeTest::destroy)> unique_node_ptr;
unique_node_ptr CreateNode(node* parent, const std::string& path) {
- return unique_node_ptr(node::Create(parent, path, &lock_), &NodeTest::destroy);
+ return unique_node_ptr(node::Create(parent, path, &lock_, &tracker_), &NodeTest::destroy);
}
};
@@ -53,7 +57,7 @@
}
TEST_F(NodeTest, TestRelease) {
- node* node = node::Create(nullptr, "/path", &lock_);
+ node* node = node::Create(nullptr, "/path", &lock_, &tracker_);
acquire(node);
acquire(node);
ASSERT_EQ(3, GetRefCount(node));
@@ -152,10 +156,10 @@
unique_node_ptr parent = CreateNode(nullptr, "/path");
// This is the tree that we intend to delete.
- node* child = node::Create(parent.get(), "subdir", &lock_);
- node::Create(child, "s1", &lock_);
- node* subchild2 = node::Create(child, "s2", &lock_);
- node::Create(subchild2, "sc2", &lock_);
+ node* child = node::Create(parent.get(), "subdir", &lock_, &tracker_);
+ node::Create(child, "s1", &lock_, &tracker_);
+ node* subchild2 = node::Create(child, "s2", &lock_, &tracker_);
+ node::Create(subchild2, "sc2", &lock_, &tracker_);
ASSERT_EQ(child, parent->LookupChildByName("subdir", false /* acquire */));
node::DeleteTree(child);
diff --git a/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java b/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
index 579794d..b905bf5 100644
--- a/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
+++ b/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
@@ -60,7 +60,6 @@
import static org.junit.Assume.assumeTrue;
-import android.Manifest;
import android.app.AppOpsManager;
import android.content.ContentResolver;
import android.database.Cursor;
@@ -138,6 +137,8 @@
"com.android.tests.fused.testapp.B", 1, false, "TestAppB.apk");
private static final String[] SYSTEM_GALERY_APPOPS = { AppOpsManager.OPSTR_WRITE_MEDIA_IMAGES,
AppOpsManager.OPSTR_WRITE_MEDIA_VIDEO };
+ //TODO(b/150115615): used AppOpsManager#OPSTR_MANAGE_EXTERNAL_STORAGE once it's public API
+ private static final String OPSTR_MANAGE_EXTERNAL_STORAGE = "android:manage_external_storage";
@Before
public void setUp() throws Exception {
@@ -1264,7 +1265,7 @@
final File musicFileInMovies = new File(MOVIES_DIR, MUSIC_FILE_NAME);
final File imageFileInDcim = new File(DCIM_DIR, IMAGE_FILE_NAME);
try {
- adoptShellPermissionIdentity(Manifest.permission.MANAGE_EXTERNAL_STORAGE);
+ allowAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
// Nothing special about this, anyone can create an image file in DCIM
assertCanCreateFile(imageFileInDcim);
// This is where we see the special powers of MANAGE_EXTERNAL_STORAGE, because it can
@@ -1273,7 +1274,7 @@
// It can even create a music file in Pictures
assertCanCreateFile(musicFileInMovies);
} finally {
- dropShellPermissionIdentity();
+ denyAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
}
}
@@ -1312,9 +1313,7 @@
assertThat(createFileAs(TEST_APP_A, otherAppImage.getPath())).isTrue();
assertThat(createFileAs(TEST_APP_A, otherAppMusic.getPath())).isTrue();
- // Now we get the permission, since some earlier method calls drop shell permission
- // identity.
- adoptShellPermissionIdentity(Manifest.permission.MANAGE_EXTERNAL_STORAGE);
+ allowAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
assertThat(otherAppPdf.delete()).isTrue();
assertThat(otherAppPdf.exists()).isFalse();
@@ -1325,7 +1324,7 @@
assertThat(otherAppMusic.delete()).isTrue();
assertThat(otherAppMusic.exists()).isFalse();
} finally {
- dropShellPermissionIdentity();
+ denyAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
deleteFileAsNoThrow(TEST_APP_A, otherAppPdf.getAbsolutePath());
deleteFileAsNoThrow(TEST_APP_A, otherAppImage.getAbsolutePath());
deleteFileAsNoThrow(TEST_APP_A, otherAppMusic.getAbsolutePath());
@@ -1348,9 +1347,7 @@
assertThat(createFileAs(TEST_APP_A, otherAppPdf.getPath())).isTrue();
assertThat(otherAppPdf.exists()).isTrue();
- // Now we get the permission, since some earlier method calls drop shell permission
- // identity.
- adoptShellPermissionIdentity(Manifest.permission.MANAGE_EXTERNAL_STORAGE);
+ allowAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
// Write some data to the file
try (final FileOutputStream fos = new FileOutputStream(otherAppPdf)) {
@@ -1377,7 +1374,7 @@
pdfInObviouslyWrongPlace.delete();
topLevelPdf.delete();
musicFile.delete();
- dropShellPermissionIdentity();
+ denyAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
deleteFileAsNoThrow(TEST_APP_A, otherAppPdf.getAbsolutePath());
uninstallApp(TEST_APP_A);
}
@@ -1408,13 +1405,13 @@
// Once the test has permission to manage external storage, it can query for other apps'
// files and open them for read and write
- adoptShellPermissionIdentity(Manifest.permission.MANAGE_EXTERNAL_STORAGE);
+ allowAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
assertCanQueryAndOpenFile(otherAppPdf, "rw");
assertCanQueryAndOpenFile(otherAppImg, "rw");
assertCanQueryAndOpenFile(otherAppMusic, "rw");
} finally {
- dropShellPermissionIdentity();
+ denyAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
deleteFilesAs(TEST_APP_A, otherAppImg, otherAppMusic, otherAppPdf);
uninstallApp(TEST_APP_A);
}