Merge "Add 2 compat flags to strictly enable/disable scoped storage" 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/TEST_MAPPING b/TEST_MAPPING
index 06b2f38..7af0ca8 100644
--- a/TEST_MAPPING
+++ b/TEST_MAPPING
@@ -22,6 +22,9 @@
},
{
"name": "FuseDaemonHostTest"
+ },
+ {
+ "name": "fuse_node_test"
}
]
}
diff --git a/jni/FuseDaemon.cpp b/jni/FuseDaemon.cpp
index 58d3f71..42fdd05 100644
--- a/jni/FuseDaemon.cpp
+++ b/jni/FuseDaemon.cpp
@@ -74,10 +74,8 @@
using std::vector;
// logging macros to avoid duplication.
-#define TRACE LOG(DEBUG)
-#define TRACE_VERBOSE LOG(VERBOSE)
-#define TRACE_FUSE(__fuse) TRACE << "[" << __fuse->path << "] "
-#define TRACE_FUSE_VERBOSE(__fuse) TRACE_VERBOSE << "[" << __fuse->path << "] "
+#define TRACE_NODE(__node) \
+ LOG(DEBUG) << __FUNCTION__ << " : " << #__node << " = [" << safe_name(__node) << "] "
#define ATRACE_NAME(name) ScopedTrace ___tracer(name)
#define ATRACE_CALL() ATRACE_NAME(__FUNCTION__)
@@ -237,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; }
@@ -258,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 {
@@ -276,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;
@@ -315,12 +287,10 @@
/* const */ char* zero_addr;
FAdviser fadviser;
-
- std::unordered_set<const node*> inode_tracker_;
};
-static inline const char* safe_name(node* n) {
- return n ? n->GetName().c_str() : "?";
+static inline string safe_name(node* n) {
+ return n ? n->BuildSafePath() : "?";
}
static inline __u64 ptr_to_id(void* ptr) {
@@ -336,7 +306,6 @@
*/
static int set_file_lock(int fd, bool for_read, const std::string& path) {
std::string lock_str = (for_read ? "read" : "write");
- TRACE_VERBOSE << "Setting " << lock_str << " lock for path " << path;
struct flock fl{};
fl.l_type = for_read ? F_RDLCK : F_WRLCK;
@@ -344,10 +313,9 @@
int res = fcntl(fd, F_OFD_SETLK, &fl);
if (res) {
- PLOG(ERROR) << "Failed to set " << lock_str << " lock on path " << path;
+ PLOG(WARNING) << "Failed to set lock: " << lock_str;
return res;
}
- TRACE_VERBOSE << "Successfully set " << lock_str << " lock on path " << path;
return res;
}
@@ -361,20 +329,17 @@
* Returns true if fd may have a lock, false otherwise
*/
static bool is_file_locked(int fd, const std::string& path) {
- TRACE_VERBOSE << "Checking if file is locked " << path;
-
struct flock fl{};
fl.l_type = F_WRLCK;
fl.l_whence = SEEK_SET;
int res = fcntl(fd, F_OFD_GETLK, &fl);
if (res) {
- PLOG(ERROR) << "Failed to check lock for file " << path;
+ PLOG(WARNING) << "Failed to check lock";
// Assume worst
return true;
}
bool locked = fl.l_type != F_UNLCK;
- TRACE_VERBOSE << "File " << path << " is " << (locked ? "locked" : "unlocked");
return locked;
}
@@ -420,10 +385,10 @@
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);
// This FS is not being exported via NFS so just a fixed generation number
// for now. If we do need this, we need to increment the generation ID each
// time the fuse daemon restarts because that's what it takes for us to
@@ -474,12 +439,10 @@
const struct fuse_ctx* ctx = fuse_req_ctx(req);
node* parent_node = fuse->FromInode(parent);
string parent_path = parent_node->BuildPath();
-
- TRACE_FUSE_VERBOSE(fuse) << "LOOKUP " << name << " @ " << parent << " ("
- << safe_name(parent_node) << ")";
-
string child_path = parent_path + "/" + name;
+ TRACE_NODE(parent_node);
+
std::smatch match;
std::regex_search(child_path, match, storage_emulated_regex);
if (match.size() == 2 && std::to_string(getuid() / PER_USER_RANGE) != match[1].str()) {
@@ -505,14 +468,12 @@
static void do_forget(struct fuse* fuse, fuse_ino_t ino, uint64_t nlookup) {
node* node = fuse->FromInode(ino);
- TRACE_FUSE(fuse) << "FORGET #" << nlookup << " @ " << ino << " (" << safe_name(node) << ")";
+ TRACE_NODE(node);
if (node) {
// 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));
}
}
@@ -545,7 +506,7 @@
const struct fuse_ctx* ctx = fuse_req_ctx(req);
node* node = fuse->FromInode(ino);
string path = node->BuildPath();
- TRACE_FUSE(fuse) << "GETATTR @ " << ino << " (" << safe_name(node) << ")";
+ TRACE_NODE(node);
if (!node) fuse_reply_err(req, ENOENT);
@@ -570,7 +531,7 @@
string path = node->BuildPath();
struct timespec times[2];
- TRACE_FUSE(fuse) << "SETATTR valid=" << to_set << " @ " << ino << "(" << safe_name(node) << ")";
+ TRACE_NODE(node);
if (!node) {
fuse_reply_err(req, ENOENT);
@@ -611,8 +572,8 @@
// times[1].tv_nsec = attr->st_mtime.tv_nsec;
}
}
- TRACE_FUSE(fuse) << "Calling utimensat on " << path << " with atime " << times[0].tv_sec
- << " mtime=" << times[1].tv_sec;
+
+ TRACE_NODE(node);
if (utimensat(-1, path.c_str(), times, 0) < 0) {
fuse_reply_err(req, errno);
return;
@@ -646,8 +607,7 @@
node* parent_node = fuse->FromInode(parent);
string parent_path = parent_node->BuildPath();
- TRACE_FUSE(fuse) << "MKNOD " << name << " 0" << std::oct << mode << " @ " << parent << " ("
- << safe_name(parent_node) << ")";
+ TRACE_NODE(parent_node);
if (!parent_node) {
fuse_reply_err(req, ENOENT);
@@ -681,8 +641,7 @@
node* parent_node = fuse->FromInode(parent);
const string parent_path = parent_node->BuildPath();
- TRACE_FUSE(fuse) << "MKDIR " << name << " 0" << std::oct << mode << " @ " << parent << " ("
- << safe_name(parent_node) << ")";
+ TRACE_NODE(parent_node);
const string child_path = parent_path + "/" + name;
@@ -715,7 +674,7 @@
node* parent_node = fuse->FromInode(parent);
const string parent_path = parent_node->BuildPath();
- TRACE_FUSE(fuse) << "UNLINK " << name << " @ " << parent << "(" << safe_name(parent_node) << ")";
+ TRACE_NODE(parent_node);
const string child_path = parent_path + "/" + name;
@@ -726,6 +685,7 @@
}
node* child_node = parent_node->LookupChildByName(name, false /* acquire */);
+ TRACE_NODE(child_node);
if (child_node) {
child_node->SetDeleted();
}
@@ -740,7 +700,7 @@
node* parent_node = fuse->FromInode(parent);
const string parent_path = parent_node->BuildPath();
- TRACE_FUSE(fuse) << "RMDIR " << name << " @ " << parent << "(" << safe_name(parent_node) << ")";
+ TRACE_NODE(parent_node);
const string child_path = parent_path + "/" + name;
@@ -756,6 +716,7 @@
}
node* child_node = parent_node->LookupChildByName(name, false /* acquire */);
+ TRACE_NODE(child_node);
if (child_node) {
child_node->SetDeleted();
}
@@ -776,7 +737,6 @@
const struct fuse_ctx* ctx = fuse_req_ctx(req);
if (flags != 0) {
- LOG(ERROR) << "One or more rename flags not supported";
return EINVAL;
}
@@ -792,11 +752,11 @@
return 0;
}
- TRACE_FUSE(fuse) << "RENAME " << name << " -> " << new_name << " @ " << parent << " ("
- << safe_name(old_parent_node) << ") -> " << new_parent << " ("
- << safe_name(new_parent_node) << ")";
+ TRACE_NODE(old_parent_node);
+ TRACE_NODE(new_parent_node);
node* child_node = old_parent_node->LookupChildByName(name, true /* acquire */);
+ TRACE_NODE(child_node) << "old_child";
const string old_child_path = child_node->BuildPath();
const string new_child_path = new_parent_path + "/" + new_name;
@@ -808,6 +768,7 @@
if (res == 0) {
child_node->Rename(new_name, new_parent_node);
}
+ TRACE_NODE(child_node) << "new_child";
child_node->Release(1);
return res;
@@ -834,8 +795,7 @@
node* node = fuse->FromInode(ino);
const string path = node->BuildPath();
- TRACE_FUSE(fuse) << "OPEN 0" << std::oct << fi->flags << " @ " << ino << " (" << safe_name(node)
- << ")";
+ TRACE_NODE(node) << (is_requesting_write(fi->flags) ? "write" : "read");
if (!node) {
fuse_reply_err(req, ENOENT);
@@ -847,7 +807,6 @@
fi->direct_io = true;
}
- TRACE_FUSE(fuse) << "OPEN " << path;
int status = fuse->mp->IsOpenAllowed(path, ctx->uid, is_requesting_write(fi->flags));
if (status) {
fuse_reply_err(req, status);
@@ -896,14 +855,7 @@
// b. Reading from a FUSE fd with caching enabled may not see the latest writes using the
// lower fs fd because those writes did not go through the FUSE layer and reads from FUSE
// after that write may be served from cache
- if (ri->isRedactionNeeded()) {
- TRACE_FUSE(fuse) << "Using direct io for " << path << " because redaction is needed.";
- } else {
- TRACE_FUSE(fuse) << "Using direct io for " << path << " because the file is locked.";
- }
fi->direct_io = true;
- } else {
- TRACE_FUSE(fuse) << "Using cache for " << path;
}
handle* h = new handle(path, fd, ri.release(), /*owner_uid*/ -1, !fi->direct_io);
@@ -1097,7 +1049,7 @@
struct fuse_file_info* fi) {
ATRACE_CALL();
struct fuse* fuse = get_fuse(req);
- TRACE_FUSE(fuse) << "FLUSH is a noop";
+ TRACE_NODE(nullptr) << "noop";
fuse_reply_err(req, 0);
}
@@ -1109,14 +1061,15 @@
node* node = fuse->FromInode(ino);
handle* h = reinterpret_cast<handle*>(fi->fh);
- TRACE_FUSE(fuse) << "RELEASE "
- << "0" << std::oct << fi->flags << " " << h << "(" << h->fd << ")";
+ TRACE_NODE(node);
fuse->fadviser.Close(h->fd);
if (node) {
- if (h->owner_uid != -1) {
- fuse->mp->ScanFile(h->path, h->owner_uid);
- }
+ // TODO(b/145737191) Legacy apps don't expect FuseDaemon to update database.
+ // Inserting/deleting the database entry might break app's functionality.
+ // if (h->owner_uid != -1) {
+ // fuse->mp->ScanFile(h->path, h->owner_uid);
+ // }
node->DestroyHandle(h);
}
@@ -1160,7 +1113,7 @@
node* node = fuse->FromInode(ino);
const string path = node->BuildPath();
- TRACE_FUSE(fuse) << "OPENDIR @ " << ino << " (" << safe_name(node) << ")" << path;
+ TRACE_NODE(node);
if (!node) {
fuse_reply_err(req, ENOENT);
@@ -1207,7 +1160,7 @@
node* node = fuse->FromInode(ino);
const string path = node->BuildPath();
- TRACE_FUSE(fuse) << "READDIR @" << ino << " " << path << " at offset " << off;
+ TRACE_NODE(node);
// Get all directory entries from MediaProvider on first readdir() call of
// directory handle. h->next_off = 0 indicates that current readdir() call
// is first readdir() call for the directory handle, Avoid multiple JNI calls
@@ -1294,7 +1247,7 @@
node* node = fuse->FromInode(ino);
dirhandle* h = reinterpret_cast<dirhandle*>(fi->fh);
- TRACE_FUSE(fuse) << "RELEASEDIR " << h;
+ TRACE_NODE(node);
if (node) {
node->DestroyDirHandle(h);
}
@@ -1342,7 +1295,7 @@
node* node = fuse->FromInode(ino);
const string path = node->BuildPath();
- TRACE_FUSE_VERBOSE(fuse) << "ACCESS " << path;
+ TRACE_NODE(node);
int res = access(path.c_str(), F_OK);
fuse_reply_err(req, res ? errno : 0);
@@ -1359,14 +1312,12 @@
node* parent_node = fuse->FromInode(parent);
const string parent_path = parent_node->BuildPath();
- TRACE_FUSE(fuse) << "CREATE " << name << " 0" << std::oct << fi->flags << " @ " << parent
- << " (" << safe_name(parent_node) << ")";
+ TRACE_NODE(parent_node);
const string child_path = parent_path + "/" + name;
int mp_return_code = fuse->mp->InsertFile(child_path.c_str(), ctx->uid);
if (mp_return_code) {
- PLOG(DEBUG) << "Could not create file: " << child_path;
fuse_reply_err(req, mp_return_code);
return;
}
@@ -1401,6 +1352,7 @@
int error_code = 0;
struct fuse_entry_param e;
node* node = make_node_entry(req, parent_node, name, child_path, &e, &error_code);
+ TRACE_NODE(node);
if (node) {
node->AddHandle(h);
fuse_reply_create(req, &e, fi);
@@ -1462,21 +1414,17 @@
*/
static struct fuse_lowlevel_ops ops{
- .init = pf_init,
- .destroy = pf_destroy,
- .lookup = pf_lookup, .forget = pf_forget, .getattr = pf_getattr,
- .setattr = pf_setattr,
- .canonical_path = pf_canonical_path,
- .mknod = pf_mknod, .mkdir = pf_mkdir, .unlink = pf_unlink,
- .rmdir = pf_rmdir,
+ .init = pf_init, .destroy = pf_destroy, .lookup = pf_lookup, .forget = pf_forget,
+ .getattr = pf_getattr, .setattr = pf_setattr, .canonical_path = pf_canonical_path,
+ .mknod = pf_mknod, .mkdir = pf_mkdir, .unlink = pf_unlink, .rmdir = pf_rmdir,
/*.symlink = pf_symlink,*/
.rename = pf_rename,
/*.link = pf_link,*/
.open = pf_open, .read = pf_read,
/*.write = pf_write,*/
- .flush = pf_flush, .release = pf_release, .fsync = pf_fsync,
- .opendir = pf_opendir, .readdir = pf_readdir, .releasedir = pf_releasedir,
- .fsyncdir = pf_fsyncdir, .statfs = pf_statfs,
+ .flush = pf_flush,
+ .release = pf_release, .fsync = pf_fsync, .opendir = pf_opendir, .readdir = pf_readdir,
+ .releasedir = pf_releasedir, .fsyncdir = pf_fsyncdir, .statfs = pf_statfs,
/*.setxattr = pf_setxattr,
.getxattr = pf_getxattr,
.listxattr = pf_listxattr,
@@ -1517,32 +1465,26 @@
}
bool FuseDaemon::ShouldOpenWithFuse(int fd, bool for_read, const std::string& path) {
- TRACE_VERBOSE << "Checking if file should be opened with FUSE " << path;
bool use_fuse = false;
if (active.load(std::memory_order_acquire)) {
const node* node = node::LookupAbsolutePath(fuse->root, path);
if (node && node->HasCachedHandle()) {
- TRACE << "Should open " << path << " with FUSE. Reason: cache";
use_fuse = true;
} else {
// If we are unable to set a lock, we should use fuse since we can't track
// when all fd references (including dups) are closed. This can happen when
// we try to set a write lock twice on the same file
use_fuse = set_file_lock(fd, for_read, path);
- TRACE << "Should open " << path << (use_fuse ? " with" : " without")
- << " FUSE. Reason: lock";
}
} else {
- TRACE << "FUSE daemon is inactive. Should not open " << path << " with FUSE";
+ LOG(WARNING) << "FUSE daemon is inactive. Cannot open file with FUSE";
}
return use_fuse;
}
void FuseDaemon::InvalidateFuseDentryCache(const std::string& path) {
- TRACE_VERBOSE << "Invalidating dentry for path " << path;
-
if (active.load(std::memory_order_acquire)) {
string name;
fuse_ino_t parent;
@@ -1558,10 +1500,10 @@
if (!name.empty() &&
fuse_lowlevel_notify_inval_entry(fuse->se, parent, name.c_str(), name.size())) {
- LOG(ERROR) << "Failed to invalidate dentry for path " << path;
+ LOG(WARNING) << "Failed to invalidate dentry for path";
}
} else {
- TRACE << "FUSE daemon is inactive. Cannot invalidate dentry for " << path;
+ LOG(WARNING) << "FUSE daemon is inactive. Cannot invalidate dentry";
}
}
@@ -1577,12 +1519,12 @@
struct stat stat;
if (lstat(path.c_str(), &stat)) {
- LOG(ERROR) << "ERROR: failed to stat source " << path;
+ PLOG(ERROR) << "ERROR: failed to stat source " << path;
return;
}
if (!S_ISDIR(stat.st_mode)) {
- LOG(ERROR) << "ERROR: source is not a directory";
+ PLOG(ERROR) << "ERROR: source is not a directory";
return;
}
diff --git a/jni/node-inl.h b/jni/node-inl.h
index 921dfe2..5098316 100644
--- a/jni/node-inl.h
+++ b/jni/node-inl.h
@@ -22,7 +22,9 @@
#include <list>
#include <memory>
#include <mutex>
+#include <sstream>
#include <string>
+#include <unordered_set>
#include <vector>
#include "libfuse_jni/ReaddirHelper.h"
@@ -63,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.
@@ -82,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));
}
@@ -114,6 +170,10 @@
// associated with its descendants.
std::string BuildPath() const;
+ // Builds the full PII safe path associated with this node, including all path segments
+ // associated with its descendants.
+ std::string BuildSafePath() const;
+
// Looks up a direct descendant of this node by name. If |acquire| is true,
// also Acquire the node before returning a reference to it.
node* LookupChildByName(const std::string& name, bool acquire) const {
@@ -213,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.
@@ -262,9 +328,9 @@
}
}
- // A helper function to recursively construct the absolute path of a given
- // node.
- static void BuildPathForNodeRecursive(node* node, std::string* path);
+ // A helper function to recursively construct the absolute path of a given node.
+ // If |safe| is true, builds a PII safe path instead
+ void BuildPathForNodeRecursive(bool safe, const node* node, std::stringstream* path) const;
// The name of this node. Non-const because it can change during renames.
std::string name_;
@@ -282,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 8898b7b..e17a9e8 100644
--- a/jni/node.cpp
+++ b/jni/node.cpp
@@ -41,20 +41,37 @@
namespace fuse {
// Assumes that |node| has at least one child.
-void node::BuildPathForNodeRecursive(node* node, std::string* path) {
- if (node->parent_) BuildPathForNodeRecursive(node->parent_, path);
+void node::BuildPathForNodeRecursive(bool safe, const node* node, std::stringstream* path) const {
+ if (node->parent_) {
+ BuildPathForNodeRecursive(safe, node->parent_, path);
+ }
- (*path) += node->GetName() + "/";
+ if (safe && node->parent_) {
+ (*path) << reinterpret_cast<uintptr_t>(node);
+ } else {
+ (*path) << node->GetName();
+ }
+
+ if (node != this) {
+ // Must not add a '/' to the last segment
+ (*path) << "/";
+ }
}
std::string node::BuildPath() const {
std::lock_guard<std::recursive_mutex> guard(*lock_);
- std::string path;
+ std::stringstream path;
- path.reserve(PATH_MAX);
- if (parent_) BuildPathForNodeRecursive(parent_, &path);
- path += name_;
- return path;
+ BuildPathForNodeRecursive(false, this, &path);
+ return path.str();
+}
+
+std::string node::BuildSafePath() const {
+ std::lock_guard<std::recursive_mutex> guard(*lock_);
+ std::stringstream path;
+
+ BuildPathForNodeRecursive(true, this, &path);
+ return path.str();
}
const node* node::LookupAbsolutePath(const node* root, const std::string& absolute_path) {
@@ -80,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/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index 69ff88c..4965381 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -1183,6 +1183,9 @@
boolean retryUpdateWithReplace = false;
try {
+ // TODO(b/146777893): System gallery apps can rename a media directory containing
+ // non-media files. This update doesn't support updating non-media files that are not
+ // owned by system gallery app.
count = qbForUpdate.update(helper, values, selection, new String[]{oldPath});
} catch (SQLiteConstraintException e) {
Log.w(TAG, "Database update failed while renaming " + oldPath, e);
@@ -1229,6 +1232,27 @@
}
/**
+ * Gets all files in the given {@code path} and subdirectories of the given {@code path}.
+ */
+ private ArrayList<String> getAllFilesForRenameDirectory(String oldPath) {
+ final String selection = MediaColumns.RELATIVE_PATH + " REGEXP '^" +
+ extractRelativePathForDirectory(oldPath) + "/?.*' and mime_type not like 'null'";
+ ArrayList<String> fileList = new ArrayList<>();
+
+ final LocalCallingIdentity token = clearLocalCallingIdentity();
+ try (final Cursor c = query(Files.getContentUriForPath(oldPath),
+ new String[] {MediaColumns.DATA}, selection, null, null)) {
+ while (c.moveToNext()) {
+ final String filePath = c.getString(0).replaceFirst("^" + oldPath + "/(.*)", "$1");
+ fileList.add(filePath);
+ }
+ } finally {
+ restoreLocalCallingIdentity(token);
+ }
+ return fileList;
+ }
+
+ /**
* Gets files in the given {@code path} and subdirectories of the given {@code path} for which
* calling package has write permissions.
*
@@ -1236,7 +1260,8 @@
* files for which calling package doesn't have write permission or if file type is not
* supported in {@code newPath}
*/
- private ArrayList<String> getWritableFilesForRenameDirectory(String oldPath, String newPath) {
+ private ArrayList<String> getWritableFilesForRenameDirectory(String oldPath, String newPath)
+ throws IllegalArgumentException {
final int countAllFilesInDirectory;
final String selection = MediaColumns.RELATIVE_PATH + " REGEXP '^" +
extractRelativePathForDirectory(oldPath) + "/?.*' and mime_type not like 'null'";
@@ -1318,16 +1343,21 @@
* write permission.
* This method can also return errno returned from {@code Os.rename} function.
*/
- private int renameDirectoryForFuse(String oldPath, String newPath) {
+ private int renameDirectoryCheckedForFuse(String oldPath, String newPath) {
final ArrayList<String> fileList;
try {
fileList = getWritableFilesForRenameDirectory(oldPath, newPath);
- } catch(IllegalArgumentException e) {
+ } catch (IllegalArgumentException e) {
final String errorMessage = "Rename " + oldPath + " to " + newPath + " failed. ";
Log.e(TAG, errorMessage, e);
return OsConstants.EPERM;
}
+ return renameDirectoryUncheckedForFuse(oldPath, newPath, fileList);
+ }
+
+ private int renameDirectoryUncheckedForFuse(String oldPath, String newPath,
+ ArrayList<String> fileList) {
final DatabaseHelper helper;
try {
helper = getDatabaseForUri(Files.getContentUriForPath(oldPath));
@@ -1378,13 +1408,17 @@
* {@code oldPath} or {@code newPath}, or file type is not supported by {@code newPath}.
* This method can also return errno returned from {@code Os.rename} function.
*/
- private int renameFileForFuse(String oldPath, String newPath) {
+ private int renameFileCheckedForFuse(String oldPath, String newPath) {
// Check if new mime type is supported in new path.
final String newMimeType = MimeUtils.resolveMimeType(new File(newPath));
if (!isMimeTypeSupportedInPath(newPath, newMimeType)) {
return OsConstants.EPERM;
}
+ return renameFileUncheckedForFuse(oldPath, newPath);
+ }
+ private int renameFileUncheckedForFuse(String oldPath, String newPath) {
+ final String newMimeType = MimeUtils.resolveMimeType(new File(newPath));
final DatabaseHelper helper;
try {
helper = getDatabaseForUri(Files.getContentUriForPath(oldPath));
@@ -1416,19 +1450,22 @@
}
/**
- * Renames file or directory in lower file system and deletes {@code oldPath} from database.
+ * Rename file/directory without imposing any restrictions.
+ *
+ * We don't impose any rename restrictions for apps that bypass scoped storage restrictions.
+ * However, we update database entries for renamed files to keep the database consistent.
*/
private int renameUncheckedForFuse(String oldPath, String newPath) {
- final int result = renameInLowerFs(oldPath, newPath);
- if (result == 0) {
- LocalCallingIdentity token = clearLocalCallingIdentity();
- try {
- scanFile(new File(oldPath), REASON_DEMAND);
- } finally {
- restoreLocalCallingIdentity(token);
- }
- }
- return result;
+
+ return renameInLowerFs(oldPath, newPath);
+ // TODO(b/145737191) Legacy apps don't expect FuseDaemon to update database.
+ // Inserting/deleting the database entry might break app functionality.
+ //if (new File(oldPath).isFile()) {
+ // return renameFileUncheckedForFuse(oldPath, newPath);
+ // } else {
+ // return renameDirectoryUncheckedForFuse(oldPath, newPath,
+ // getAllFilesForRenameDirectory(oldPath));
+ // }
}
/**
@@ -1525,9 +1562,9 @@
// Continue renaming files/directories if rename of oldPath to newPath is allowed.
if (new File(oldPath).isFile()) {
- return renameFileForFuse(oldPath, newPath);
+ return renameFileCheckedForFuse(oldPath, newPath);
} else {
- return renameDirectoryForFuse(oldPath, newPath);
+ return renameDirectoryCheckedForFuse(oldPath, newPath);
}
} finally {
restoreLocalCallingIdentity(token);
@@ -5610,6 +5647,12 @@
final LocalCallingIdentity token =
clearLocalCallingIdentity(LocalCallingIdentity.fromExternal(getContext(), uid));
try {
+ if (shouldBypassFuseRestrictions(/*forWrite*/ true, path)) {
+ // TODO(b/145737191) Legacy apps don't expect FuseDaemon to update database.
+ // Inserting/deleting the database entry might break app functionality.
+ return deleteFileUnchecked(path);
+ }
+
// Check if app is deleting a file under an app specific directory
final String appSpecificDir = extractPathOwnerPackageName(path);
@@ -5623,12 +5666,9 @@
}
}
- final boolean shouldBypass = shouldBypassFuseRestrictions(/*forWrite*/ true,
- path);
-
// Legacy apps that made is this far don't have the right storage permission and hence
// are not allowed to access anything other than their external app directory
- if (!shouldBypass && isCallingPackageRequestingLegacy()) {
+ if (isCallingPackageRequestingLegacy()) {
return OsConstants.EPERM;
}
@@ -5642,12 +5682,6 @@
final String[] whereArgs = {sanitizedPath};
if (delete(contentUri, where, whereArgs) == 0) {
- if (shouldBypass) {
- // For apps that bypass scoped storage restrictions, delete() should only fail
- // if the file is not in database. This shouldn't stop these apps from deleting
- // a file, hence delete the file in the lower file system.
- return deleteFileUnchecked(path);
- }
return OsConstants.ENOENT;
} else if (!path.equals(sanitizedPath)) {
// delete() doesn't delete the file in lower file system if sanitized path is
diff --git a/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/FuseDaemonHostTest.java b/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/FuseDaemonHostTest.java
index 6af8a3a..a4ab22b 100644
--- a/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/FuseDaemonHostTest.java
+++ b/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/FuseDaemonHostTest.java
@@ -24,6 +24,7 @@
import org.junit.After;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -118,6 +119,7 @@
runDeviceTest("testListFilesFromExternalMediaDirectory");
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testListUnsupportedFileType() throws Exception {
final ITestDevice device = getDevice();
@@ -194,21 +196,25 @@
runDeviceTest("testSystemGalleryAppHasNoFullAccessToAudio");
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testSystemGalleryCanRenameImagesAndVideos() throws Exception {
runDeviceTest("testSystemGalleryCanRenameImagesAndVideos");
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testManageExternalStorageCanCreateFilesAnywhere() throws Exception {
runDeviceTest("testManageExternalStorageCanCreateFilesAnywhere");
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testManageExternalStorageCanDeleteOtherAppsContents() throws Exception {
runDeviceTest("testManageExternalStorageCanDeleteOtherAppsContents");
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testManageExternalStorageCanRenameOtherAppsContents() throws Exception {
runDeviceTest("testManageExternalStorageCanRenameOtherAppsContents");
diff --git a/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java b/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java
index 1f1b8c7..852ab0e 100644
--- a/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java
+++ b/tests/jni/FuseDaemonTest/host/src/com/android/tests/fused/host/LegacyAccessHostTest.java
@@ -25,6 +25,7 @@
import org.junit.After;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -107,14 +108,14 @@
@Test
public void testCreateFilesInRandomPlaces_hasW() throws Exception {
revokePermissions("android.permission.READ_EXTERNAL_STORAGE");
- executeShellCommand("mkdir -p /sdcard/Android/data/com.android.shell");
+ executeShellCommand("mkdir -p /sdcard/Android/data/com.android.shell -m 2770");
runDeviceTest("testCreateFilesInRandomPlaces_hasW");
}
@Test
public void testMkdirInRandomPlaces_hasW() throws Exception {
revokePermissions("android.permission.READ_EXTERNAL_STORAGE");
- executeShellCommand("mkdir -p /sdcard/Android/data/com.android.shell");
+ executeShellCommand("mkdir -p /sdcard/Android/data/com.android.shell -m 2770");
runDeviceTest("testMkdirInRandomPlaces_hasW");
}
@@ -153,6 +154,7 @@
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testCanRename_hasRW() throws Exception {
runDeviceTest("testCanRename_hasRW");
@@ -161,7 +163,12 @@
@Test
public void testCantRename_hasR() throws Exception {
revokePermissions("android.permission.WRITE_EXTERNAL_STORAGE");
- runDeviceTest("testCantRename_hasR");
+ createFileAsShell(SHELL_FILE, /*bypassFuse*/ true);
+ try {
+ runDeviceTest("testCantRename_hasR");
+ } finally {
+ executeShellCommand("rm " + SHELL_FILE);
+ }
}
@@ -177,11 +184,13 @@
}
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testCanDeleteAllFiles_hasRW() throws Exception {
runDeviceTest("testCanDeleteAllFiles_hasRW");
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testLegacyAppCanOwnAFile_hasW() throws Exception {
runDeviceTest("testLegacyAppCanOwnAFile_hasW");
diff --git a/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java b/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java
index 49609d2..8dfbb4d 100644
--- a/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java
+++ b/tests/jni/FuseDaemonTest/legacy/src/com/android/tests/fused/legacy/LegacyFileAccessTest.java
@@ -16,6 +16,9 @@
package com.android.tests.fused.legacy;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameFile;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameDirectory;
+import static com.android.tests.fused.lib.TestUtils.assertCantRenameFile;
import static com.android.tests.fused.lib.TestUtils.createFileAs;
@@ -50,6 +53,7 @@
import com.android.tests.fused.lib.ReaddirTestHelper;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -274,6 +278,7 @@
* Test that rename for legacy app with WRITE_EXTERNAL_STORAGE permission bypasses rename
* restrictions imposed by MediaProvider
*/
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testCanRename_hasRW() throws Exception {
pollForPermission(Manifest.permission.READ_EXTERNAL_STORAGE, /*granted*/ true);
@@ -294,16 +299,16 @@
try {
// can rename a file to root directory.
assertThat(musicFile1.createNewFile()).isTrue();
- assertCanRename(musicFile1, musicFile2);
+ assertCanRenameFile(musicFile1, musicFile2);
// can rename a music file to Movies directory.
- assertCanRename(musicFile2, musicFile3);
+ assertCanRenameFile(musicFile2, musicFile3);
assertThat(nonMediaDir1.mkdir()).isTrue();
assertThat(pdfFile1.createNewFile()).isTrue();
// can rename directory to root directory.
- assertCanRename(nonMediaDir1, nonMediaDir2);
- assertThat(pdfFile2.exists()).isTrue();
+ assertCanRenameDirectory(nonMediaDir1, nonMediaDir2, new File[]{pdfFile1},
+ new File[]{pdfFile2});
} finally {
musicFile1.delete();
musicFile2.delete();
@@ -335,13 +340,14 @@
getExternalMediaDirs()[0], "LegacyFileAccessTest2");
try {
// app can't rename shell file.
- assertThat(shellFile1.renameTo(shellFile2)).isFalse();
+ assertCantRenameFile(shellFile1, shellFile2);
// app can't move shell file to its media directory.
- assertThat(mediaFile1.renameTo(shellFile1)).isFalse();
+ assertCantRenameFile(shellFile1, mediaFile1);
// However, even without permissions, app can rename files in its own external media
// directory.
assertThat(mediaFile1.createNewFile()).isTrue();
- assertCanRename(mediaFile1, mediaFile2);
+ assertThat(mediaFile1.renameTo(mediaFile2)).isTrue();
+ assertThat(mediaFile2.exists()).isTrue();
} finally {
mediaFile1.delete();
mediaFile2.delete();
@@ -367,13 +373,14 @@
getExternalMediaDirs()[0], "LegacyFileAccessTest2");
try {
// app can't rename shell file.
- assertThat(shellFile1.renameTo(shellFile2)).isFalse();
+ assertCantRenameFile(shellFile1, shellFile2);
// app can't move shell file to its media directory.
- assertThat(mediaFile1.renameTo(shellFile1)).isFalse();
+ assertCantRenameFile(shellFile1, mediaFile1);
// However, even without permissions, app can rename files in its own external media
// directory.
assertThat(mediaFile1.createNewFile()).isTrue();
- assertCanRename(mediaFile1, mediaFile2);
+ assertThat(mediaFile1.renameTo(mediaFile2)).isTrue();
+ assertThat(mediaFile2.exists()).isTrue();
} finally {
mediaFile1.delete();
mediaFile2.delete();
@@ -384,6 +391,7 @@
* Test that legacy app with WRITE_EXTERNAL_STORAGE can delete all files, and corresponding
* database entry is deleted on deleting the file.
*/
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testCanDeleteAllFiles_hasRW() throws Exception {
pollForPermission(Manifest.permission.READ_EXTERNAL_STORAGE, /*granted*/ true);
@@ -423,6 +431,7 @@
* Test that file created by legacy app is inserted to MediaProvider database. And,
* MediaColumns.OWNER_PACKAGE_NAME is updated with calling package's name.
*/
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testLegacyAppCanOwnAFile_hasW() throws Exception {
pollForPermission(Manifest.permission.READ_EXTERNAL_STORAGE, /*granted*/ true);
@@ -480,10 +489,4 @@
dir.delete();
}
}
-
- private static void assertCanRename(File oldPath, File newPath) {
- assertThat(oldPath.renameTo(newPath)).isTrue();
- assertThat(oldPath.exists()).isFalse();
- assertThat(newPath.exists()).isTrue();
- }
}
diff --git a/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java b/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java
index eb34939..89863ed 100644
--- a/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java
+++ b/tests/jni/FuseDaemonTest/libs/FuseDaemonTestLib/src/com/android/tests/fused/lib/TestUtils.java
@@ -404,6 +404,46 @@
return path.delete();
}
+ public static void assertCanRenameFile(File oldFile, File newFile) {
+ assertThat(oldFile.renameTo(newFile)).isTrue();
+ assertThat(oldFile.exists()).isFalse();
+ assertThat(newFile.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(-1);
+ assertThat(getFileRowIdFromDatabase(newFile)).isNotEqualTo(-1);
+ }
+
+ public static void assertCantRenameFile(File oldFile, File newFile) {
+ final int rowId = getFileRowIdFromDatabase(oldFile);
+ assertThat(oldFile.renameTo(newFile)).isFalse();
+ assertThat(oldFile.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(rowId);
+ }
+
+ public static void assertCanRenameDirectory(File oldDirectory, File newDirectory,
+ @Nullable File[] oldFilesList, @Nullable File[] newFilesList) {
+ assertThat(oldDirectory.renameTo(newDirectory)).isTrue();
+ assertThat(oldDirectory.exists()).isFalse();
+ assertThat(newDirectory.exists()).isTrue();
+ for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
+ assertThat(file.exists()).isFalse();
+ assertThat(getFileRowIdFromDatabase(file)).isEqualTo(-1);
+ }
+ for (File file : newFilesList != null ? newFilesList : new File[0]) {
+ assertThat(file.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
+ };
+ }
+
+ public static void assertCantRenameDirectory(File oldDirectory, File newDirectory,
+ @Nullable File[] oldFilesList) {
+ assertThat(oldDirectory.renameTo(newDirectory)).isFalse();
+ assertThat(oldDirectory.exists()).isTrue();
+ for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
+ assertThat(file.exists()).isTrue();
+ assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
+ }
+ }
+
public static void pollForExternalStorageState() throws Exception {
for (int i = 0; i < POLLING_TIMEOUT_MILLIS / POLLING_SLEEP_MILLIS; i++) {
if(Environment.getExternalStorageState(Environment.getExternalStorageDirectory())
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 8f47edd..b905bf5 100644
--- a/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
+++ b/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
@@ -25,8 +25,12 @@
import static com.android.tests.fused.lib.RedactionTestHelper.assertExifMetadataMismatch;
import static com.android.tests.fused.lib.RedactionTestHelper.getExifMetadata;
import static com.android.tests.fused.lib.RedactionTestHelper.getExifMetadataFromRawResource;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameFile;
+import static com.android.tests.fused.lib.TestUtils.assertCanRenameDirectory;
import static com.android.tests.fused.lib.TestUtils.adoptShellPermissionIdentity;
import static com.android.tests.fused.lib.TestUtils.allowAppOpsToUid;
+import static com.android.tests.fused.lib.TestUtils.assertCantRenameDirectory;
+import static com.android.tests.fused.lib.TestUtils.assertCantRenameFile;
import static com.android.tests.fused.lib.TestUtils.assertThrows;
import static com.android.tests.fused.lib.TestUtils.createFileAs;
import static com.android.tests.fused.lib.TestUtils.deleteFileAs;
@@ -56,7 +60,6 @@
import static org.junit.Assume.assumeTrue;
-import android.Manifest;
import android.app.AppOpsManager;
import android.content.ContentResolver;
import android.database.Cursor;
@@ -71,7 +74,6 @@
import android.system.OsConstants;
import android.util.Log;
-import androidx.annotation.Nullable;
import androidx.test.runner.AndroidJUnit4;
import com.android.cts.install.lib.TestApp;
@@ -80,6 +82,7 @@
import com.google.common.io.ByteStreams;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -134,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 {
@@ -661,6 +666,7 @@
/**
* Test that readdir lists unsupported file types in default directories.
*/
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testListUnsupportedFileType() throws Exception {
final File pdfFile = new File(DCIM_DIR, NONMEDIA_FILE_NAME);
@@ -967,6 +973,7 @@
}
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testSystemGalleryCanRenameImagesAndVideos() throws Exception {
final File otherAppVideoFile = new File(DCIM_DIR, "other_" + VIDEO_FILE_NAME);
@@ -989,21 +996,19 @@
assertFileContent(otherAppVideoFile, BYTES_DATA1);
// Assert we can rename the file and ensure the file has the same content
- assertThat(otherAppVideoFile.renameTo(videoFile)).isTrue();
- assertThat(otherAppVideoFile.exists()).isFalse();
+ assertCanRenameFile(otherAppVideoFile, videoFile);
assertFileContent(videoFile, BYTES_DATA1);
// We can even move it to the top level directory
- assertThat(videoFile.renameTo(topLevelVideoFile)).isTrue();
- assertThat(videoFile.exists()).isFalse();
+ assertCanRenameFile(videoFile, topLevelVideoFile);
assertFileContent(topLevelVideoFile, BYTES_DATA1);
// And we can even convert it into an image file, because why not?
- assertThat(topLevelVideoFile.renameTo(imageFile)).isTrue();
- assertThat(topLevelVideoFile.exists()).isFalse();
+ assertCanRenameFile(topLevelVideoFile, imageFile);
assertFileContent(imageFile, BYTES_DATA1);
- // However, we can't convert it to a music file, because system gallery has full access
- // to images and video only
- assertThat(imageFile.renameTo(musicFile)).isFalse();
+ // We can convert it to a music file, but we won't have access to music file after
+ // renaming.
+ assertThat(imageFile.renameTo(musicFile)).isTrue();
+ assertThat(getFileRowIdFromDatabase(musicFile)).isEqualTo(-1);
} finally {
deleteFileAsNoThrow(TEST_APP_A, otherAppVideoFile.getAbsolutePath());
uninstallApp(TEST_APP_A);
@@ -1253,13 +1258,14 @@
}
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testManageExternalStorageCanCreateFilesAnywhere() throws Exception {
final File topLevelPdf = new File(EXTERNAL_STORAGE_DIR, NONMEDIA_FILE_NAME);
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
@@ -1268,7 +1274,7 @@
// It can even create a music file in Pictures
assertCanCreateFile(musicFileInMovies);
} finally {
- dropShellPermissionIdentity();
+ denyAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
}
}
@@ -1293,6 +1299,7 @@
}
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testManageExternalStorageCanDeleteOtherAppsContents() throws Exception {
final File otherAppPdf = new File(DOWNLOAD_DIR, "other" + NONMEDIA_FILE_NAME);
@@ -1306,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();
@@ -1319,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());
@@ -1327,6 +1332,7 @@
}
}
+ @Ignore("Re-enable as part of b/145737191")
@Test
public void testManageExternalStorageCanRenameOtherAppsContents() throws Exception {
final File otherAppPdf = new File(DOWNLOAD_DIR, "other" + NONMEDIA_FILE_NAME);
@@ -1341,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)) {
@@ -1352,30 +1356,25 @@
assertFileContent(otherAppPdf, BYTES_DATA1);
// Assert we can rename the file and ensure the file has the same content
- assertThat(otherAppPdf.renameTo(pdf)).isTrue();
- assertThat(otherAppPdf.exists()).isFalse();
+ assertCanRenameFile(otherAppPdf, pdf);
assertFileContent(pdf, BYTES_DATA1);
// We can even move it to the top level directory
- assertThat(pdf.renameTo(topLevelPdf)).isTrue();
- assertThat(pdf.exists()).isFalse();
+ assertCanRenameFile(pdf, topLevelPdf);
assertFileContent(topLevelPdf, BYTES_DATA1);
// And even rename to a place where PDFs don't belong, because we're an omnipotent
// external storage manager
- assertThat(topLevelPdf.renameTo(pdfInObviouslyWrongPlace)).isTrue();
- assertThat(topLevelPdf.exists()).isFalse();
+ assertCanRenameFile(topLevelPdf, pdfInObviouslyWrongPlace);
assertFileContent(pdfInObviouslyWrongPlace, BYTES_DATA1);
// And we can even convert it into a music file, because why not?
- assertThat(pdfInObviouslyWrongPlace.renameTo(musicFile)).isTrue();
- assertThat(pdfInObviouslyWrongPlace.exists()).isFalse();
+ assertCanRenameFile(pdfInObviouslyWrongPlace, musicFile);
assertFileContent(musicFile, BYTES_DATA1);
-
} finally {
pdf.delete();
pdfInObviouslyWrongPlace.delete();
topLevelPdf.delete();
musicFile.delete();
- dropShellPermissionIdentity();
+ denyAppOpsToUid(Process.myUid(), OPSTR_MANAGE_EXTERNAL_STORAGE);
deleteFileAsNoThrow(TEST_APP_A, otherAppPdf.getAbsolutePath());
uninstallApp(TEST_APP_A);
}
@@ -1406,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);
}
@@ -1542,46 +1541,6 @@
}
}
- private static void assertCanRenameFile(File oldFile, File newFile) {
- assertThat(oldFile.renameTo(newFile)).isTrue();
- assertThat(oldFile.exists()).isFalse();
- assertThat(newFile.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(-1);
- assertThat(getFileRowIdFromDatabase(newFile)).isNotEqualTo(-1);
- }
-
- private static void assertCantRenameFile(File oldFile, File newFile) {
- final int rowId = getFileRowIdFromDatabase(oldFile);
- assertThat(oldFile.renameTo(newFile)).isFalse();
- assertThat(oldFile.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(oldFile)).isEqualTo(rowId);
- }
-
- private static void assertCanRenameDirectory(File oldDirectory, File newDirectory,
- @Nullable File[] oldFilesList, @Nullable File[] newFilesList) {
- assertThat(oldDirectory.renameTo(newDirectory)).isTrue();
- assertThat(oldDirectory.exists()).isFalse();
- assertThat(newDirectory.exists()).isTrue();
- for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
- assertThat(file.exists()).isFalse();
- assertThat(getFileRowIdFromDatabase(file)).isEqualTo(-1);
- }
- for (File file : newFilesList != null ? newFilesList : new File[0]) {
- assertThat(file.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
- };
- }
-
- private static void assertCantRenameDirectory(File oldDirectory, File newDirectory,
- @Nullable File[] oldFilesList) {
- assertThat(oldDirectory.renameTo(newDirectory)).isFalse();
- assertThat(oldDirectory.exists()).isTrue();
- for (File file : oldFilesList != null ? oldFilesList : new File[0]) {
- assertThat(file.exists()).isTrue();
- assertThat(getFileRowIdFromDatabase(file)).isNotEqualTo(-1);
- }
- }
-
/**
* Asserts the entire content of the file equals exactly {@code expectedContent}.
*/