Reland scanning files on close
We disabled scanning in If8e4f396f06c948267605ef83eda7dc3d331fefe,
however, issues seem fixed now so we should try to enable again.
Scan the file on close() for newly created file. ScanFile will insert
this file to MediaProvider database. File is considered new file if the
file is opened with O_CREAT flag or close() is called on file descriptor
obtained from create().
Changed FuseDaemonHostTest to verify file is inserted into database for
apps that bypass MediaProvider check.
Test: atest FuseDaemonHostTest
Bug: 145737191
Change-Id: I54f8e273751e7e0ff9c7d489e4629fd5fe273784
diff --git a/jni/FuseDaemon.cpp b/jni/FuseDaemon.cpp
index 1c10335..1fa5ebe 100644
--- a/jni/FuseDaemon.cpp
+++ b/jni/FuseDaemon.cpp
@@ -897,7 +897,7 @@
TRACE_FUSE(fuse) << "Using cache for " << path;
}
- handle* h = new handle(path, fd, ri.release(), !fi->direct_io);
+ handle* h = new handle(path, fd, ri.release(), !fi->direct_io, fi->flags & O_CREAT);
node->AddHandle(h);
fi->fh = ptr_to_id(h);
@@ -1104,8 +1104,10 @@
<< "0" << std::oct << fi->flags << " " << h << "(" << h->fd << ")";
fuse->fadviser.Close(h->fd);
- // TODO(b/145737191): Figure out if we need to scan files on close, and how to do it properly
if (node) {
+ if (h->is_new_file) {
+ fuse->mp->ScanFile(h->path);
+ }
node->DestroyHandle(h);
}
@@ -1375,7 +1377,8 @@
// This prevents crashing during reads but can be a security hole if a malicious app opens an fd
// to the file before all the EXIF content is written. We could special case reads before the
// first close after a file has just been created.
- handle* h = new handle(child_path, fd, new RedactionInfo(), true /* cached */);
+ handle* h = new handle(child_path, fd, new RedactionInfo(), /*cached*/ true,
+ /*is_new_file*/ true);
fi->fh = ptr_to_id(h);
fi->keep_cache = 1;
diff --git a/jni/node-inl.h b/jni/node-inl.h
index 4b688f1..76d1ba9 100644
--- a/jni/node-inl.h
+++ b/jni/node-inl.h
@@ -34,8 +34,9 @@
namespace fuse {
struct handle {
- explicit handle(const std::string& path, int fd, const RedactionInfo* ri, bool cached)
- : path(path), fd(fd), ri(ri), cached(cached) {
+ explicit handle(const std::string& path, int fd, const RedactionInfo* ri, bool cached,
+ bool is_new_file)
+ : path(path), fd(fd), ri(ri), cached(cached), is_new_file(is_new_file) {
CHECK(ri != nullptr);
}
@@ -43,6 +44,7 @@
const int fd;
const std::unique_ptr<const RedactionInfo> ri;
const bool cached;
+ const bool is_new_file;
~handle() { close(fd); }
};
diff --git a/jni/node_test.cpp b/jni/node_test.cpp
index ba876a0..3ead90b 100644
--- a/jni/node_test.cpp
+++ b/jni/node_test.cpp
@@ -209,7 +209,8 @@
TEST_F(NodeTest, AddDestroyHandle) {
unique_node_ptr node = CreateNode(nullptr, "/path");
- handle* h = new handle("/path", -1, new mediaprovider::fuse::RedactionInfo, true /* cached */);
+ handle* h = new handle("/path", -1, new mediaprovider::fuse::RedactionInfo, true /* cached */,
+ true /* is_new_file */);
node->AddHandle(h);
ASSERT_TRUE(node->HasCachedHandle());
@@ -220,7 +221,7 @@
// the node in question.
EXPECT_DEATH(node->DestroyHandle(h), "");
EXPECT_DEATH(node->DestroyHandle(nullptr), "");
- std::unique_ptr<handle> h2(
- new handle("/path2", -1, new mediaprovider::fuse::RedactionInfo, true /* cached */));
+ std::unique_ptr<handle> h2(new handle("/path2", -1, new mediaprovider::fuse::RedactionInfo,
+ true /* cached */, true /* is_new_file */));
EXPECT_DEATH(node->DestroyHandle(h2.get()), "");
}
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 00e698e..3bbdcdd 100644
--- a/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
+++ b/tests/jni/FuseDaemonTest/src/com/android/tests/fused/FilePathAccessTest.java
@@ -129,7 +129,6 @@
"com.android.tests.fused.testapp.A", 1, false, "TestAppA.apk");
private static final TestApp TEST_APP_B = new TestApp("TestAppB",
"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 };
@@ -276,6 +275,20 @@
final byte[] expected = (STR_DATA1 + STR_DATA2).getBytes();
assertFileContent(imageFile, expected);
+
+ // Closing the file after writing will not trigger a MediaScan. Call scanFile to update
+ // file's entry in MediaProvider's database.
+ assertThat(MediaStore.scanFile(getContentResolver(), imageFile)).isNotNull();
+
+ // Ensure that the scan was completed and the file's size was updated.
+ try (final Cursor c = cr.query(MediaStore.Images.Media.EXTERNAL_CONTENT_URI,
+ /* projection */new String[] {MediaColumns.SIZE},
+ selection, selectionArgs, null)) {
+ assertThat(c.getCount()).isEqualTo(1);
+ c.moveToFirst();
+ assertThat(c.getInt(c.getColumnIndex(MediaColumns.SIZE)))
+ .isEqualTo(BYTES_DATA1.length + BYTES_DATA2.length);
+ }
} finally {
imageFile.delete();
}
@@ -609,7 +622,6 @@
*/
@Test
public void testListFilesFromExternalMediaDirectory() throws Exception {
- final String packageName = THIS_PACKAGE_NAME;
final File videoFile = new File(EXTERNAL_MEDIA_DIR, VIDEO_FILE_NAME);
final String videoFileName = videoFile.getName();
@@ -621,29 +633,17 @@
// App should see its directory and other app's external media directories with media
// files.
- // TODO(b/145757667): Uncomment this when we start indexing Android/media files.
- // assertThat(ReaddirTestHelper.readDirectory(ANDROID_MEDIA_DIR)).contains(packageName);
assertThat(ReaddirTestHelper.readDirectory(EXTERNAL_MEDIA_DIR))
.containsExactly(videoFileName);
// Install TEST_APP_A with READ_EXTERNAL_STORAGE permission.
// TEST_APP_A with storage permission should see other app's external media directory.
-
- // TODO(b/145757667): Uncomment this when we start indexing Android/media files.
- // For context, this used to work when we used to scan files after closing them, but
- // now, since we don't, videoFileName is not indexed by MediaProvider, which means
- // that Android/media/<pkg-name> is empty and so MediaProvider can't see it.
- // We also can't use ContentResolver#insert since MediaProvider doesn't allow videos
- // under primary directory Android.
-// installApp(TEST_APP_A, true);
-// assertThat(listDirectoryEntriesFromTestApp(TEST_APP_A, ANDROID_MEDIA_DIR.getPath()))
-// .contains(packageName);
-// assertThat(listDirectoryEntriesFromTestApp(TEST_APP_A, EXTERNAL_MEDIA_DIR.getPath()))
-// .containsExactly(videoFileName);
+ installApp(TEST_APP_A, true);
+ // Apps can't list files in other app's external media directory.
+ assertThat(listAs(TEST_APP_A, ANDROID_MEDIA_DIR.getPath())).isEmpty();
+ assertThat(listAs(TEST_APP_A, EXTERNAL_MEDIA_DIR.getPath())).isEmpty();
} finally {
videoFile.delete();
- // TODO(b/145757667): Uncomment this when we start indexing Android/media files.
-// uninstallAppNoThrow(TEST_APP_A);
}
}
@@ -663,9 +663,12 @@
installApp(TEST_APP_A, true);
assertThat(listAs(TEST_APP_A, DCIM_DIR.getPath())).doesNotContain(NONMEDIA_FILE_NAME);
- // TEST_APP_A with storage permission should see video file in Music directory.
+
executeShellCommand("touch " + videoFile.getAbsolutePath());
- assertThat(MediaStore.scanFile(getContentResolver(), videoFile)).isNotNull();
+ // ScanFile doesn't insert an empty media file to database. Write some data to ensure
+ // file is inserted into database.
+ executeShellCommand("echo " + STR_DATA1 + " > " + videoFile.getAbsolutePath());
+ // TEST_APP_A with storage permission should see video file in Music directory.
assertThat(listAs(TEST_APP_A, MUSIC_DIR.getPath())).contains(VIDEO_FILE_NAME);
} finally {
executeShellCommand("rm " + pdfFile.getAbsolutePath());
@@ -895,14 +898,11 @@
assertThat(otherAppImageFile.delete()).isTrue();
assertThat(otherAppImageFile.exists()).isFalse();
- // Put the file back in its place and let TEST_APP_A delete it
- assertThat(otherAppImageFile.createNewFile()).isTrue();
-
// Can create an image anywhere
assertCanCreateFile(topLevelImageFile);
assertCanCreateFile(imageInAnObviouslyWrongPlace);
} finally {
- deleteFileAs(TEST_APP_A, otherAppImageFile.getPath());
+ otherAppImageFile.delete();
uninstallApp(TEST_APP_A);
denyAppOpsToUid(Process.myUid(), SYSTEM_GALERY_APPOPS);
}
@@ -1312,10 +1312,10 @@
assertThat(otherAppImage.createNewFile()).isTrue();
assertThat(otherAppMusic.createNewFile()).isTrue();
} finally {
+ otherAppPdf.delete();
+ otherAppImage.delete();
+ otherAppMusic.delete();
dropShellPermissionIdentity();
- deleteFileAs(TEST_APP_A, otherAppPdf.getPath());
- deleteFileAs(TEST_APP_A, otherAppImage.getPath());
- deleteFileAs(TEST_APP_A, otherAppMusic.getPath());
uninstallApp(TEST_APP_A);
}
}
@@ -1365,13 +1365,14 @@
// Rename file back to it's original name so that the test app can clean it up
assertThat(musicFile.renameTo(otherAppPdf)).isTrue();
+ assertThat(deleteFileAs(TEST_APP_A, otherAppPdf.getPath())).isTrue();
} finally {
pdf.delete();
pdfInObviouslyWrongPlace.delete();
topLevelPdf.delete();
musicFile.delete();
dropShellPermissionIdentity();
- deleteFileAs(TEST_APP_A, otherAppPdf.getPath());
+ otherAppPdf.delete();
uninstallApp(TEST_APP_A);
}
}