From 366493edd29b1ec2a7be0323dd547d242e6797fa Mon Sep 17 00:00:00 2001 From: Zim Date: Thu, 29 Jul 2021 20:51:10 +0100 Subject: [PATCH] Fix fd leak while bypassing transcoding in media APIs To call the media service while bypassing transcoding there are 4 file descriptors involved: A: The original file descriptor the app owns and is responsible for closing B: Dupe of A, to pass into the MediaStore API as an input to receive a non-transoding fd C: The output from the MediaStore API D: Final fd owned by the media service, after C gets duped over binder as part of setDataSource We were leaking (B) and (C). Now we close them appropriately. See I0124ec8fbd0471237e99bab321f903544f8fe1f8 for another 2 fd leak fix in MediaProvider Test: atest TranscodeTest Test: Manual with 'adb shell lsof | grep ' Bug: 194828489 Bug: 179804072 Change-Id: I978257bbc4a8f6813b6e6a5ce22124257204f432 Merged-In: I978257bbc4a8f6813b6e6a5ce22124257204f432 --- core/java/android/os/FileUtils.java | 18 +++++++++--------- media/java/android/media/ExifInterface.java | 9 ++++++++- .../android/media/MediaMetadataRetriever.java | 17 ++++++++++++----- media/java/android/media/MediaPlayer.java | 13 ++++++++----- 4 files changed, 37 insertions(+), 20 deletions(-) diff --git a/core/java/android/os/FileUtils.java b/core/java/android/os/FileUtils.java index f3e0ce9cd19e8..1bc64951ad960 100644 --- a/core/java/android/os/FileUtils.java +++ b/core/java/android/os/FileUtils.java @@ -1460,15 +1460,15 @@ public final class FileUtils { /** {@hide} */ @VisibleForTesting public static ParcelFileDescriptor convertToModernFd(FileDescriptor fd) { - try { - Context context = AppGlobals.getInitialApplication(); - if (UserHandle.getAppId(Process.myUid()) == getMediaProviderAppId(context)) { - // Never convert modern fd for MediaProvider, because this requires - // MediaStore#scanFile and can cause infinite loops when MediaProvider scans - return null; - } - return MediaStore.getOriginalMediaFormatFileDescriptor(context, - ParcelFileDescriptor.dup(fd)); + Context context = AppGlobals.getInitialApplication(); + if (UserHandle.getAppId(Process.myUid()) == getMediaProviderAppId(context)) { + // Never convert modern fd for MediaProvider, because this requires + // MediaStore#scanFile and can cause infinite loops when MediaProvider scans + return null; + } + + try (ParcelFileDescriptor dupFd = ParcelFileDescriptor.dup(fd)) { + return MediaStore.getOriginalMediaFormatFileDescriptor(context, dupFd); } catch (Exception e) { Log.d(TAG, "Failed to convert to modern format file descriptor", e); return null; diff --git a/media/java/android/media/ExifInterface.java b/media/java/android/media/ExifInterface.java index d746c850a0181..37054b8383b11 100644 --- a/media/java/android/media/ExifInterface.java +++ b/media/java/android/media/ExifInterface.java @@ -1573,6 +1573,9 @@ public class ExifInterface { if (isFdDuped) { closeFileDescriptor(fileDescriptor); } + if (modernFd != null) { + modernFd.close(); + } } } @@ -2554,12 +2557,13 @@ public class ExifInterface { private void initForFilename(String filename) throws IOException { FileInputStream in = null; + ParcelFileDescriptor modernFd = null; mAssetInputStream = null; mFilename = filename; mIsInputStream = false; try { in = new FileInputStream(filename); - ParcelFileDescriptor modernFd = FileUtils.convertToModernFd(in.getFD()); + modernFd = FileUtils.convertToModernFd(in.getFD()); if (modernFd != null) { closeQuietly(in); in = new FileInputStream(modernFd.getFileDescriptor()); @@ -2570,6 +2574,9 @@ public class ExifInterface { loadAttributes(in); } finally { closeQuietly(in); + if (modernFd != null) { + modernFd.close(); + } } } diff --git a/media/java/android/media/MediaMetadataRetriever.java b/media/java/android/media/MediaMetadataRetriever.java index 2943eee5b1da4..a15529e99d15c 100644 --- a/media/java/android/media/MediaMetadataRetriever.java +++ b/media/java/android/media/MediaMetadataRetriever.java @@ -36,6 +36,7 @@ import android.os.IBinder; import android.os.ParcelFileDescriptor; import android.os.SystemProperties; import android.text.TextUtils; +import android.util.Log; import java.io.FileDescriptor; import java.io.FileInputStream; @@ -52,6 +53,8 @@ import java.util.Map; * frame and meta data from an input media file. */ public class MediaMetadataRetriever implements AutoCloseable { + private static final String TAG = "MediaMetadataRetriever"; + // borrowed from ExoPlayer private static final String[] STANDARD_GENRES = new String[] { // These are the official ID3v1 genres. @@ -301,11 +304,15 @@ public class MediaMetadataRetriever implements AutoCloseable { */ public void setDataSource(FileDescriptor fd, long offset, long length) throws IllegalArgumentException { - ParcelFileDescriptor modernFd = FileUtils.convertToModernFd(fd); - if (modernFd == null) { - _setDataSource(fd, offset, length); - } else { - _setDataSource(modernFd.getFileDescriptor(), offset, length); + + try (ParcelFileDescriptor modernFd = FileUtils.convertToModernFd(fd)) { + if (modernFd == null) { + _setDataSource(fd, offset, length); + } else { + _setDataSource(modernFd.getFileDescriptor(), offset, length); + } + } catch (IOException e) { + Log.w(TAG, "Ignoring IO error while setting data source", e); } } diff --git a/media/java/android/media/MediaPlayer.java b/media/java/android/media/MediaPlayer.java index 4f761ba0bf6a6..26eb2a9dc109f 100644 --- a/media/java/android/media/MediaPlayer.java +++ b/media/java/android/media/MediaPlayer.java @@ -1271,11 +1271,14 @@ public class MediaPlayer extends PlayerBase */ public void setDataSource(FileDescriptor fd, long offset, long length) throws IOException, IllegalArgumentException, IllegalStateException { - ParcelFileDescriptor modernFd = FileUtils.convertToModernFd(fd); - if (modernFd == null) { - _setDataSource(fd, offset, length); - } else { - _setDataSource(modernFd.getFileDescriptor(), offset, length); + try (ParcelFileDescriptor modernFd = FileUtils.convertToModernFd(fd)) { + if (modernFd == null) { + _setDataSource(fd, offset, length); + } else { + _setDataSource(modernFd.getFileDescriptor(), offset, length); + } + } catch (IOException e) { + Log.w(TAG, "Ignoring IO error while setting data source", e); } }