Add fields to FileLookupResult and improve node invalidation logic

1. transforms_supported field replaces the FLAG_TRANSFORM_SUPPORTED
flag that was added to the transforms field to indicate that
transforms are supported on an inode but the node itself might not
need any transforms. This allows the FUSE daemon to avoid caching that
node for *any* uid. Extracting this flag is a cleaner design.
2. uid field represents the original uid requesting IO. This is useful
when the MediaProvider does a FUSE open on behalf of another app and
we need to know the original uid in the FUSE daemon. This change just
adds the field, the logic for the original uid will be added in later
cl.

3. Refactored the logic to decide if a node should be invalidated in
the VFS dentry cache. Now the node itself contains that information as
a combination of case-insensitivity and transforms_supported signals

Bug: 174655855
Test: presubmit
Change-Id: I3a1b3f5e239289f8d972642e00ec66e6908894bd

7 files changed, 100 insertions, 72 deletions

diff --git a/jni/FuseDaemon.cpp b/jni/FuseDaemon.cpp
index a75660c11..b30cffa15 100755
--- a/jni/FuseDaemon.cpp
+++ b/jni/FuseDaemon.cpp
@@ -390,12 +390,14 @@ static void fuse_inval(fuse_session* se, fuse_ino_t parent_ino, fuse_ino_t child
     }
 }
 
-static double get_attr_timeout(const string& path, bool should_inval, node* node,
-                               struct fuse* fuse) {
-    if (fuse->disable_dentry_cache || should_inval || is_package_owned_path(path, fuse->path)) {
+static double get_attr_timeout(const string& path, node* node, struct fuse* fuse) {
+    if (fuse->disable_dentry_cache || node->ShouldInvalidate() ||
+        is_package_owned_path(path, fuse->path)) {
         // We set dentry timeout to 0 for the following reasons:
         // 1. The dentry cache was completely disabled
-        // 2. Case-insensitive lookups need to invalidate other case-insensitive dentry matches
+        // 2.1 Case-insensitive lookups need to invalidate other case-insensitive dentry matches
+        // 2.2 Nodes supporting transforms need to be invalidated, so that subsequent lookups by a
+        // uid requiring a transform is guaranteed to come to the FUSE daemon.
         // 3. With app data isolation enabled, app A should not guess existence of app B from the
         // Android/{data,obb}/<package> paths, hence we prevent the kernel from caching that
         // information.
@@ -404,8 +406,7 @@ static double get_attr_timeout(const string& path, bool should_inval, node* node
     return std::numeric_limits<double>::max();
 }
 
-static double get_entry_timeout(const string& path, bool should_inval, node* node,
-                                struct fuse* fuse) {
+static double get_entry_timeout(const string& path, node* node, struct fuse* fuse) {
     string media_path = fuse->GetEffectiveRootPath() + "/Android/media";
     if (path.find(media_path, 0) == 0) {
         // Installd might delete Android/media/<package> dirs when app data is cleared.
@@ -413,7 +414,7 @@ static double get_entry_timeout(const string& path, bool should_inval, node* nod
         // dir via FUSE.
         return 0;
     }
-    return get_attr_timeout(path, should_inval, node, fuse);
+    return get_attr_timeout(path, node, fuse);
 }
 
 static std::string get_path(node* node) {
@@ -433,7 +434,7 @@ static node* make_node_entry(fuse_req_t req, node* parent, const string& name, c
         return NULL;
     }
 
-    bool should_inval = false;
+    bool should_invalidate = false;
     bool transforms_complete = true;
     int transforms = 0;
     string io_path;
@@ -441,7 +442,7 @@ static node* make_node_entry(fuse_req_t req, node* parent, const string& name, c
     if (S_ISREG(e->attr.st_mode)) {
         // Handle potential file transforms
         std::unique_ptr<mediaprovider::fuse::FileLookupResult> file_lookup_result =
-                fuse->mp->FileLookup(path, req->ctx.uid);
+                fuse->mp->FileLookup(path, req->ctx.uid, req->ctx.pid);
 
         if (!file_lookup_result) {
             // Fail lookup if we can't fetch FileLookupResult for path
@@ -453,37 +454,34 @@ static node* make_node_entry(fuse_req_t req, node* parent, const string& name, c
         transforms = file_lookup_result->transforms;
         io_path = file_lookup_result->io_path;
         transforms_complete = file_lookup_result->transforms_complete;
+        // Invalidate if the inode supports transforms so that we always get a lookup into userspace
+        should_invalidate = file_lookup_result->transforms_supported;
 
         // Update size with io_path size if io_path is not same as path
         if (!io_path.empty() && (io_path != path) && (lstat(io_path.c_str(), &e->attr) < 0)) {
             *error_code = errno;
             return NULL;
         }
-
-        // Invalidate if there are any transforms so that we always get a lookup into userspace
-        should_inval = should_inval || transforms;
     }
 
     node = parent->LookupChildByName(name, true /* acquire */, transforms);
     if (!node) {
-        node = ::node::Create(parent, name, io_path, transforms_complete, transforms, &fuse->lock,
-                              &fuse->tracker);
+        node = ::node::Create(parent, name, io_path, should_invalidate, transforms_complete,
+                              transforms, &fuse->lock, &fuse->tracker);
     } else if (!mediaprovider::fuse::containsMount(path, std::to_string(getuid() / PER_USER_RANGE))) {
-        should_inval = should_inval || node->HasCaseInsensitiveMatch();
         // Only invalidate a path if it does not contain mount.
         // Invalidate both names to ensure there's no dentry left in the kernel after the following
         // operations:
         // 1) touch foo, touch FOO, unlink *foo*
         // 2) touch foo, touch FOO, unlink *FOO*
         // Invalidating lookup_name fixes (1) and invalidating node_name fixes (2)
-        // |should_inval| invalidates lookup_name by using 0 timeout below and we explicitly
+        // SetShouldInvalidate invalidates lookup_name by using 0 timeout below and we explicitly
         // invalidate node_name if different case
         // Note that we invalidate async otherwise we will deadlock the kernel
         if (name != node->GetName()) {
-            should_inval = true;
             // Record that we have made a case insensitive lookup, this allows us invalidate nodes
             // correctly on subsequent lookups for the case of |node|
-            node->SetCaseInsensitiveMatch();
+            node->SetShouldInvalidate();
 
             // Make copies of the node name and path so we're not attempting to acquire
             // any node locks from the invalidation thread. Depending on timing, we may end
@@ -503,8 +501,8 @@ static node* make_node_entry(fuse_req_t req, node* parent, const string& name, c
     // reuse inode numbers.
     e->generation = 0;
     e->ino = fuse->ToInode(node);
-    e->entry_timeout = get_entry_timeout(path, should_inval, node, fuse);
-    e->attr_timeout = get_attr_timeout(path, should_inval, node, fuse);
+    e->entry_timeout = get_entry_timeout(path, node, fuse);
+    e->attr_timeout = get_attr_timeout(path, node, fuse);
     return node;
 }
 
@@ -693,10 +691,7 @@ static void pf_getattr(fuse_req_t req,
     if (lstat(path.c_str(), &s) < 0) {
         fuse_reply_err(req, errno);
     } else {
-        fuse_reply_attr(
-                req, &s,
-                get_attr_timeout(path, node->GetTransforms() || node->HasCaseInsensitiveMatch(),
-                                 node, fuse));
+        fuse_reply_attr(req, &s, get_attr_timeout(path, node, fuse));
     }
 }
 
@@ -791,9 +786,7 @@ static void pf_setattr(fuse_req_t req,
     }
 
     lstat(path.c_str(), attr);
-    fuse_reply_attr(req, attr,
-                    get_attr_timeout(path, node->GetTransforms() || node->HasCaseInsensitiveMatch(),
-                                     node, fuse));
+    fuse_reply_attr(req, attr, get_attr_timeout(path, node, fuse));
 }
 
 static void pf_canonical_path(fuse_req_t req, fuse_ino_t ino)
diff --git a/jni/MediaProviderWrapper.cpp b/jni/MediaProviderWrapper.cpp
index 27f93397e..b1926c136 100644
--- a/jni/MediaProviderWrapper.cpp
+++ b/jni/MediaProviderWrapper.cpp
@@ -295,7 +295,7 @@ MediaProviderWrapper::MediaProviderWrapper(JNIEnv* env, jobject media_provider)
                                  /*is_static*/ false);
     mid_file_lookup_ =
             CacheMethod(env, "onFileLookup",
-                        "(Ljava/lang/String;I)Lcom/android/providers/media/FileLookupResult;",
+                        "(Ljava/lang/String;II)Lcom/android/providers/media/FileLookupResult;",
                         /*is_static*/ false);
 
     file_lookup_result_class_ = env->FindClass("com/android/providers/media/FileLookupResult");
@@ -305,7 +305,9 @@ MediaProviderWrapper::MediaProviderWrapper(JNIEnv* env, jobject media_provider)
     file_lookup_result_class_ =
             reinterpret_cast<jclass>(env->NewGlobalRef(file_lookup_result_class_));
     fid_file_lookup_transforms_ = CacheFileLookupField(env, "transforms", "I");
+    fid_file_lookup_uid_ = CacheFileLookupField(env, "uid", "I");
     fid_file_lookup_transforms_complete_ = CacheFileLookupField(env, "transformsComplete", "Z");
+    fid_file_lookup_transforms_supported_ = CacheFileLookupField(env, "transformsSupported", "Z");
     fid_file_lookup_io_path_ = CacheFileLookupField(env, "ioPath", "Ljava/lang/String;");
 }
 
@@ -476,7 +478,7 @@ bool MediaProviderWrapper::IsAppCloneUser(uid_t userId) {
 }
 
 std::unique_ptr<FileLookupResult> MediaProviderWrapper::FileLookup(const std::string& path,
-                                                                   uid_t uid) {
+                                                                   uid_t uid, pid_t tid) {
     JNIEnv* env = MaybeAttachCurrentThread();
 
     ScopedLocalRef<jstring> j_path(env, env->NewStringUTF(path.c_str()));
@@ -489,15 +491,19 @@ std::unique_ptr<FileLookupResult> MediaProviderWrapper::FileLookup(const std::st
     }
 
     int transforms = env->GetIntField(j_res_file_lookup_object.get(), fid_file_lookup_transforms_);
+    int original_uid = env->GetIntField(j_res_file_lookup_object.get(), fid_file_lookup_uid_);
     bool transforms_complete = env->GetBooleanField(j_res_file_lookup_object.get(),
                                                     fid_file_lookup_transforms_complete_);
+    bool transforms_supported = env->GetBooleanField(j_res_file_lookup_object.get(),
+                                                     fid_file_lookup_transforms_supported_);
     ScopedLocalRef<jstring> j_io_path(
             env,
             (jstring)env->GetObjectField(j_res_file_lookup_object.get(), fid_file_lookup_io_path_));
     ScopedUtfChars j_io_path_utf(env, j_io_path.get());
 
-    std::unique_ptr<FileLookupResult> file_lookup_result = std::make_unique<FileLookupResult>(
-            transforms, transforms_complete, string(j_io_path_utf.c_str()));
+    std::unique_ptr<FileLookupResult> file_lookup_result =
+            std::make_unique<FileLookupResult>(transforms, original_uid, transforms_complete,
+                                               transforms_supported, string(j_io_path_utf.c_str()));
     return file_lookup_result;
 }
 
diff --git a/jni/MediaProviderWrapper.h b/jni/MediaProviderWrapper.h
index 31d9d1bc1..a807b9b5c 100644
--- a/jni/MediaProviderWrapper.h
+++ b/jni/MediaProviderWrapper.h
@@ -17,6 +17,7 @@
 #ifndef MEDIAPROVIDER_FUSE_MEDIAPROVIDERWRAPPER_H_
 #define MEDIAPROVIDER_FUSE_MEDIAPROVIDERWRAPPER_H_
 
+#include <android-base/logging.h>
 #include <jni.h>
 #include <sys/types.h>
 
@@ -40,15 +41,26 @@ namespace fuse {
  * status and the ioPath. Provided by MediaProvider.java via a JNI call.
  */
 struct FileLookupResult {
-    FileLookupResult(int transforms, bool transforms_complete, const std::string& io_path)
-        : transforms(transforms), transforms_complete(transforms_complete), io_path(io_path) {}
+    FileLookupResult(int transforms, uid_t uid, bool transforms_complete, bool transforms_supported,
+                     const std::string& io_path)
+        : transforms(transforms),
+          uid(uid),
+          transforms_complete(transforms_complete),
+          transforms_supported(transforms_supported),
+          io_path(io_path) {
+        if (transforms != 0) {
+            CHECK(transforms_supported);
+        }
+    }
 
     /**
      * This field is not to be interpreted, it is determined and populated from MediaProvider
      * via a JNI call.
      */
     const int transforms;
+    const uid_t uid;
     const bool transforms_complete;
+    const bool transforms_supported;
     const std::string io_path;
 };
 
@@ -193,7 +205,7 @@ class MediaProviderWrapper final {
     /**
      * Returns FileLookupResult to determine transform info for a path and uid.
      */
-    std::unique_ptr<FileLookupResult> FileLookup(const std::string& path, uid_t uid);
+    std::unique_ptr<FileLookupResult> FileLookup(const std::string& path, uid_t uid, pid_t tid);
 
     /** Transforms from src to dst file */
     bool Transform(const std::string& src, const std::string& dst, int transforms, uid_t uid);
@@ -244,7 +256,9 @@ class MediaProviderWrapper final {
     jmethodID mid_file_lookup_;
     /** Cached FileLookupResult field IDs **/
     jfieldID fid_file_lookup_transforms_;
+    jfieldID fid_file_lookup_uid_;
     jfieldID fid_file_lookup_transforms_complete_;
+    jfieldID fid_file_lookup_transforms_supported_;
     jfieldID fid_file_lookup_io_path_;
 
     /**
diff --git a/jni/node-inl.h b/jni/node-inl.h
index 1fa62b01a..2b577bf4a 100644
--- a/jni/node-inl.h
+++ b/jni/node-inl.h
@@ -119,13 +119,14 @@ class node {
   public:
     // Creates a new node with the specified parent, name and lock.
     static node* Create(node* parent, const std::string& name, const std::string& io_path,
-                        bool transforms_complete, const int transforms, std::recursive_mutex* lock,
-                        NodeTracker* tracker) {
+                        bool should_invalidate, bool transforms_complete, const int transforms,
+                        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, io_path, transforms_complete, transforms, lock, tracker);
+        return new node(parent, name, io_path, should_invalidate, transforms_complete, transforms,
+                        lock, tracker);
     }
 
     // Creates a new root node. Root nodes have no parents by definition
@@ -133,7 +134,8 @@ class node {
     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, path, true, 0, lock, tracker);
+        node* root = new node(nullptr, path, path, false /* should_invalidate */,
+                              true /* transforms_complete */, 0, lock, tracker);
 
         // The root always has one extra reference to avoid it being
         // accidentally collected.
@@ -303,14 +305,14 @@ class node {
         return false;
     }
 
-    bool HasCaseInsensitiveMatch() const {
+    bool ShouldInvalidate() const {
         std::lock_guard<std::recursive_mutex> guard(*lock_);
-        return has_case_insensitive_match_;
+        return should_invalidate_;
     }
 
-    void SetCaseInsensitiveMatch() {
+    void SetShouldInvalidate() {
         std::lock_guard<std::recursive_mutex> guard(*lock_);
-        has_case_insensitive_match_ = true;
+        should_invalidate_ = true;
     }
 
     bool HasRedactedCache() const {
@@ -346,8 +348,9 @@ class node {
     static const node* LookupAbsolutePath(const node* root, const std::string& absolute_path);
 
   private:
-    node(node* parent, const std::string& name, const std::string& io_path, bool transforms_complete,
-         const int transforms, std::recursive_mutex* lock, NodeTracker* tracker)
+    node(node* parent, const std::string& name, const std::string& io_path,
+         const bool should_invalidate, const bool transforms_complete, const int transforms,
+         std::recursive_mutex* lock, NodeTracker* tracker)
         : name_(name),
           io_path_(io_path),
           transforms_complete_(transforms_complete),
@@ -355,7 +358,7 @@ class node {
           refcount_(0),
           parent_(nullptr),
           has_redacted_cache_(false),
-          has_case_insensitive_match_(false),
+          should_invalidate_(should_invalidate),
           deleted_(false),
           lock_(lock),
           tracker_(tracker) {
@@ -366,6 +369,10 @@ class node {
         if (parent != nullptr) {
             AddToParent(parent);
         }
+        // If the node requires transforms, we MUST never cache it in the VFS
+        if (transforms) {
+            CHECK(should_invalidate_);
+        }
     }
 
     // Acquires a reference to a node. This maps to the "lookup count" specified
@@ -499,7 +506,7 @@ class node {
     // List of directory handles associated with this node. Guarded by |lock_|.
     std::vector<std::unique_ptr<dirhandle>> dirhandles_;
     bool has_redacted_cache_;
-    bool has_case_insensitive_match_;
+    bool should_invalidate_;
     bool deleted_;
     std::recursive_mutex* lock_;
 
diff --git a/jni/node_test.cpp b/jni/node_test.cpp
index 8c3406e3c..6496f8084 100644
--- a/jni/node_test.cpp
+++ b/jni/node_test.cpp
@@ -32,8 +32,9 @@ class NodeTest : public ::testing::Test {
     typedef std::unique_ptr<node, decltype(&NodeTest::destroy)> unique_node_ptr;
 
     unique_node_ptr CreateNode(node* parent, const std::string& path, const int transforms = 0) {
-        return unique_node_ptr(node::Create(parent, path, "", true, transforms, &lock_, &tracker_),
-                               &NodeTest::destroy);
+        return unique_node_ptr(
+                node::Create(parent, path, "", true, true, transforms, &lock_, &tracker_),
+                &NodeTest::destroy);
     }
 
     static class node* ForChild(class node* node, const std::string& name,
@@ -67,7 +68,7 @@ TEST_F(NodeTest, TestCreate_withParent) {
 }
 
 TEST_F(NodeTest, TestRelease) {
-    node* node = node::Create(nullptr, "/path", "", true, 0, &lock_, &tracker_);
+    node* node = node::Create(nullptr, "/path", "", false, true, 0, &lock_, &tracker_);
     acquire(node);
     acquire(node);
     ASSERT_EQ(3, GetRefCount(node));
@@ -277,10 +278,10 @@ TEST_F(NodeTest, DeleteTree) {
     unique_node_ptr parent = CreateNode(nullptr, "/path");
 
     // This is the tree that we intend to delete.
-    node* child = node::Create(parent.get(), "subdir", "", true, 0, &lock_, &tracker_);
-    node::Create(child, "s1", "", true, 0, &lock_, &tracker_);
-    node* subchild2 = node::Create(child, "s2", "", true, 0, &lock_, &tracker_);
-    node::Create(subchild2, "sc2", "", true, 0, &lock_, &tracker_);
+    node* child = node::Create(parent.get(), "subdir", "", false, true, 0, &lock_, &tracker_);
+    node::Create(child, "s1", "", false, true, 0, &lock_, &tracker_);
+    node* subchild2 = node::Create(child, "s2", "", false, true, 0, &lock_, &tracker_);
+    node::Create(subchild2, "sc2", "", false, true, 0, &lock_, &tracker_);
 
     ASSERT_EQ(child, parent->LookupChildByName("subdir", false /* acquire */));
     node::DeleteTree(child);
diff --git a/src/com/android/providers/media/FileLookupResult.java b/src/com/android/providers/media/FileLookupResult.java
index d5531fc61..4dc4d034c 100644
--- a/src/com/android/providers/media/FileLookupResult.java
+++ b/src/com/android/providers/media/FileLookupResult.java
@@ -22,12 +22,17 @@ package com.android.providers.media;
  */
 public final class FileLookupResult {
     public final int transforms;
+    public final int uid;
     public final boolean transformsComplete;
+    public final boolean transformsSupported;
     public final String ioPath;
 
-    public FileLookupResult(int transforms, boolean transformsComplete, String ioPath) {
+    public FileLookupResult(int transforms, int uid, boolean transformsComplete,
+            boolean transformsSupported, String ioPath) {
         this.transforms = transforms;
+        this.uid = uid;
         this.transformsComplete = transformsComplete;
+        this.transformsSupported = transformsSupported;
         this.ioPath = ioPath;
     }
 }
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index 5685dbbe6..55b9c40e5 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -241,10 +241,8 @@ public class MediaProvider extends ContentProvider {
     static final Pattern PATTERN_SELECTION_ID = Pattern.compile(
             "(?:image_id|video_id)\\s*=\\s*(\\d+)");
 
-    /** File supports transforms and uid requires transcoding */
+    /** File access by uid requires the transcoding transform */
     private static final int FLAG_TRANSFORM_TRANSCODING = 1;
-    /** File supports transforms */
-    private static final int FLAG_TRANSFORM_SUPPORTED = 1 << 30;
 
     /**
      * These directory names aren't declared in Environment as final variables, and so we need to
@@ -1387,20 +1385,29 @@ public class MediaProvider extends ContentProvider {
      * for transform lookup query for a file and uid.
      *
      * @param path file path to get transforms for
-     * @param uid app requesting IO
+     * @param uid app requesting IO form kernel
+     * @param tid FUSE thread id handling IO request from kernel
      *
      * Called from JNI in jni/MediaProviderWrapper.cpp
      */
     @Keep
-    public FileLookupResult onFileLookupForFuse(String path, int uid) {
+    public FileLookupResult onFileLookupForFuse(String path, int uid, int tid) {
         String ioPath = "";
         boolean transformsComplete = true;
-        int transforms = getTransformsForFuse(path, uid);
-        if (transforms != 0) {
-            ioPath = getIoPathForFuse(path, uid);
-            transformsComplete = Objects.equals(path, ioPath);
+        boolean transformsSupported = mTranscodeHelper.supportsTranscode(path);
+        int transforms = 0;
+
+        if (transformsSupported) {
+            // TODO(b/170974147): Avoid duplicate shouldTranscode calls in getTransformsForFuse and
+            // getIoPathForFuse
+            transforms = getTransformsForFuse(path, uid);
+            if (transforms != 0) {
+                ioPath = getIoPathForFuse(path, uid);
+                transformsComplete = false;
+            }
         }
-        return new FileLookupResult(transforms, transformsComplete, ioPath);
+        return new FileLookupResult(transforms, uid, transformsComplete, transformsSupported,
+                ioPath);
     }
 
     /**
@@ -1430,15 +1437,10 @@ public class MediaProvider extends ContentProvider {
      * @see {@link transformForFuse}
      */
     private int getTransformsForFuse(String path, int uid) {
-        int result = 0;
-        if (mTranscodeHelper.supportsTranscode(path)) {
-            result |= FLAG_TRANSFORM_SUPPORTED;
-
-            if (mTranscodeHelper.shouldTranscode(path, uid, null /* bundle */)) {
-                result |= FLAG_TRANSFORM_TRANSCODING;
-            }
+        if (mTranscodeHelper.shouldTranscode(path, uid, null /* bundle */)) {
+            return FLAG_TRANSFORM_TRANSCODING;
         }
-        return result;
+        return 0;
     }
 
     /**