From 5cb769d99952ef9fb4f576abba70423fe157342b Mon Sep 17 00:00:00 2001 From: Derek Sollenberger Date: Wed, 24 Sep 2014 09:20:09 -0400 Subject: [PATCH] Fix memory leak where we close the descriptor instead of the file. bug: 17541634 Change-Id: I9968f9df249e4cba24383239ce6130dd16fcd532 --- core/jni/android/graphics/BitmapFactory.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/core/jni/android/graphics/BitmapFactory.cpp b/core/jni/android/graphics/BitmapFactory.cpp index 8ea28eca476b2..e0abc24c42c50 100644 --- a/core/jni/android/graphics/BitmapFactory.cpp +++ b/core/jni/android/graphics/BitmapFactory.cpp @@ -478,7 +478,7 @@ static jobject nativeDecodeFileDescriptor(JNIEnv* env, jobject clazz, jobject fi NPE_CHECK_RETURN_ZERO(env, fileDescriptor); - jint descriptor = jniGetFDFromFileDescriptor(env, fileDescriptor); + int descriptor = jniGetFDFromFileDescriptor(env, fileDescriptor); struct stat fdStat; if (fstat(descriptor, &fdStat) == -1) { @@ -486,16 +486,27 @@ static jobject nativeDecodeFileDescriptor(JNIEnv* env, jobject clazz, jobject fi return nullObjectReturn("fstat return -1"); } - // Restore the descriptor's offset on exiting this function. + // Restore the descriptor's offset on exiting this function. Even though + // we dup the descriptor, both the original and dup refer to the same open + // file description and changes to the file offset in one impact the other. AutoFDSeek autoRestore(descriptor); - FILE* file = fdopen(descriptor, "r"); + // Duplicate the descriptor here to prevent leaking memory. A leak occurs + // if we only close the file descriptor and not the file object it is used to + // create. If we don't explicitly clean up the file (which in turn closes the + // descriptor) the buffers allocated internally by fseek will be leaked. + int dupDescriptor = dup(descriptor); + + FILE* file = fdopen(dupDescriptor, "r"); if (file == NULL) { + // cleanup the duplicated descriptor since it will not be closed when the + // file is cleaned up (fclose). + close(dupDescriptor); return nullObjectReturn("Could not open file"); } SkAutoTUnref fileStream(new SkFILEStream(file, - SkFILEStream::kCallerRetains_Ownership)); + SkFILEStream::kCallerPasses_Ownership)); // Use a buffered stream. Although an SkFILEStream can be rewound, this // ensures that SkImageDecoder::Factory never rewinds beyond the