From 016bd09332c83e83afd8e711600cd64b5ae63245 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Sun, 14 Jul 2024 05:04:13 +0000 Subject: Fix UAF in android_os_Process_readProcFile when kDebugProc==true ReleaseStringUTFChars is called before the string is printed later in the function. Bug: 351917521 Test: cuttlefish boot Change-Id: I3d8e5c7250a94fef2a3154b9596a1bf532cdea2d --- core/jni/android_util_Process.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/core/jni/android_util_Process.cpp b/core/jni/android_util_Process.cpp index d7a0b0408e99..579a1f83ac6c 100644 --- a/core/jni/android_util_Process.cpp +++ b/core/jni/android_util_Process.cpp @@ -1047,21 +1047,21 @@ jboolean android_os_Process_readProcFile(JNIEnv* env, jobject clazz, return JNI_FALSE; } - const char* file8 = env->GetStringUTFChars(file, NULL); - if (file8 == NULL) { + auto releaser = [&](const char* jniStr) { env->ReleaseStringUTFChars(file, jniStr); }; + std::unique_ptr file8(env->GetStringUTFChars(file, NULL), + releaser); + if (!file8) { jniThrowException(env, "java/lang/OutOfMemoryError", NULL); return JNI_FALSE; } - ::android::base::unique_fd fd(open(file8, O_RDONLY | O_CLOEXEC)); + ::android::base::unique_fd fd(open(file8.get(), O_RDONLY | O_CLOEXEC)); if (!fd.ok()) { if (kDebugProc) { - ALOGW("Unable to open process file: %s\n", file8); + ALOGW("Unable to open process file: %s\n", file8.get()); } - env->ReleaseStringUTFChars(file, file8); return JNI_FALSE; } - env->ReleaseStringUTFChars(file, file8); // Most proc files we read are small, so we go through the loop // with the stack buffer firstly. We allocate a buffer big @@ -1082,7 +1082,7 @@ jboolean android_os_Process_readProcFile(JNIEnv* env, jobject clazz, if (numberBytesRead < 0) { if (kDebugProc) { ALOGW("Unable to read process file err: %s file: %s fd=%d\n", - strerror_r(errno, &readBufferStack[0], sizeof(readBufferStack)), file8, + strerror_r(errno, &readBufferStack[0], sizeof(readBufferStack)), file8.get(), fd.get()); } return JNI_FALSE; @@ -1099,7 +1099,7 @@ jboolean android_os_Process_readProcFile(JNIEnv* env, jobject clazz, // Buffer is fully used, try to grow it. if (readBufferSize > std::numeric_limits::max() / 2) { if (kDebugProc) { - ALOGW("Proc file too big: %s fd=%d\n", file8, fd.get()); + ALOGW("Proc file too big: %s fd=%d\n", file8.get(), fd.get()); } return JNI_FALSE; } -- cgit v1.2.3-59-g8ed1b From 8fdaa6ea70b28a964b2471d904e94d0d2fb7233a Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Sun, 14 Jul 2024 05:12:32 +0000 Subject: Don't restart procfs reads from scratch android_os_Process_readProcFile is mainly used for small files. It tries to minimize memory use by first attempting to read into a stack buffer, and only after that buffer is found to be too small does it use a heap-allocated buffer. The heap buffer starts out small and grows as the file size is found to exceed the size of the buffer. However on both the transition from the stack buffer to the heap buffer, and when enlarging the heap buffer, the data that has already been read gets thrown away and the read of the file restarts from the beginning. The dominating runtime cost for the function is the kernel work to produce the output. Copying the already-read data into a larger buffer in userspace is more efficient than throwing it away and rereading from the beginning. So this commit changes the function to do that. This reduces cpu-cycles by about 35% for reads of the largest /proc/pid/maps file on Android. Tested on a Pixel 6 Pro with system_server's maps file across 20 runs: Simpleperf event count (cpu-cycles) Copy Reread Average 13,338,222 20,254,826 StdDev 4,791,640 9,644,942 Min 7,460,801 12,576,877 Max 25,403,534 45,759,654 Eliminating the 1024 byte stack buffer and just using an expandable heap buffer indicates further improvement with an average of 12,835,802 cycles, code simplification, and a .text savings of between about 50-200 bytes depending on compiler optimizations. I suspect the memory savings from attempting to read small files into the stack buffer is not that meaningful, however I didn't measure peak memory use for either small or large reads so I did not remove it in this commit. Bug: 351917521 Test: cuttlefish boot Change-Id: Ifa4a739ca43415b1d8dd85177a7c50eb86955160 --- core/jni/android_util_Process.cpp | 84 ++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 46 deletions(-) diff --git a/core/jni/android_util_Process.cpp b/core/jni/android_util_Process.cpp index 579a1f83ac6c..2efadbbcf85a 100644 --- a/core/jni/android_util_Process.cpp +++ b/core/jni/android_util_Process.cpp @@ -33,6 +33,7 @@ #include #include +#include #include #include #include @@ -50,7 +51,6 @@ #include #include #include -#include #include #include #include @@ -73,13 +73,13 @@ static constexpr bool kDebugProc = false; // readProcFile() are reading files under this threshold, e.g., // /proc/pid/stat. /proc/pid/time_in_state ends up being about 520 // bytes, so use 1024 for the stack to provide a bit of slack. -static constexpr ssize_t kProcReadStackBufferSize = 1024; +static constexpr size_t kProcReadStackBufferSize = 1024; // The other files we read from proc tend to be a bit larger (e.g., // /proc/stat is about 3kB), so once we exhaust the stack buffer, // retry with a relatively large heap-allocated buffer. We double // this size and retry until the whole file fits. -static constexpr ssize_t kProcReadMinHeapBufferSize = 4096; +static constexpr size_t kProcReadMinHeapBufferSize = 4096; #if GUARD_THREAD_PRIORITY Mutex gKeyCreateMutex; @@ -1064,61 +1064,53 @@ jboolean android_os_Process_readProcFile(JNIEnv* env, jobject clazz, } // Most proc files we read are small, so we go through the loop - // with the stack buffer firstly. We allocate a buffer big - // enough for the whole file. - - char readBufferStack[kProcReadStackBufferSize]; - std::unique_ptr readBufferHeap; - char* readBuffer = &readBufferStack[0]; - ssize_t readBufferSize = kProcReadStackBufferSize; - ssize_t numberBytesRead; + // with the stack buffer first. We allocate a buffer big enough + // for most files. + + char stackBuf[kProcReadStackBufferSize]; + std::vector heapBuf; + char* buf = stackBuf; + + size_t remaining = sizeof(stackBuf); off_t offset = 0; - for (;;) { - ssize_t requestedBufferSize = readBufferSize - offset; - // By using pread, we can avoid an lseek to rewind the FD - // before retry, saving a system call. - numberBytesRead = - TEMP_FAILURE_RETRY(pread(fd, readBuffer + offset, requestedBufferSize, offset)); - if (numberBytesRead < 0) { + ssize_t numBytesRead; + + do { + numBytesRead = TEMP_FAILURE_RETRY(pread(fd, buf + offset, remaining, offset)); + if (numBytesRead < 0) { if (kDebugProc) { ALOGW("Unable to read process file err: %s file: %s fd=%d\n", - strerror_r(errno, &readBufferStack[0], sizeof(readBufferStack)), file8.get(), - fd.get()); + strerror_r(errno, stackBuf, sizeof(stackBuf)), file8.get(), fd.get()); } return JNI_FALSE; } - if (numberBytesRead == 0) { - // End of file. - numberBytesRead = offset; - break; - } - if (numberBytesRead < requestedBufferSize) { - // Read less bytes than requested, it's not an error per pread(2). - offset += numberBytesRead; - } else { - // Buffer is fully used, try to grow it. - if (readBufferSize > std::numeric_limits::max() / 2) { - if (kDebugProc) { - ALOGW("Proc file too big: %s fd=%d\n", file8.get(), fd.get()); + + offset += numBytesRead; + remaining -= numBytesRead; + + if (numBytesRead && !remaining) { + if (buf == stackBuf) { + heapBuf.resize(kProcReadMinHeapBufferSize); + static_assert(kProcReadMinHeapBufferSize > sizeof(stackBuf)); + std::memcpy(heapBuf.data(), stackBuf, sizeof(stackBuf)); + } else { + if (heapBuf.size() >= std::numeric_limits::max() / 2) { + if (kDebugProc) { + ALOGW("Proc file too big: %s fd=%d size=%zu\n", + file8.get(), fd.get(), heapBuf.size()); + } + return JNI_FALSE; } - return JNI_FALSE; + heapBuf.resize(2 * heapBuf.size()); } - readBufferSize = std::max(readBufferSize * 2, kProcReadMinHeapBufferSize); - readBufferHeap.reset(); // Free address space before getting more. - readBufferHeap = std::make_unique(readBufferSize); - if (!readBufferHeap) { - jniThrowException(env, "java/lang/OutOfMemoryError", NULL); - return JNI_FALSE; - } - readBuffer = readBufferHeap.get(); - offset = 0; + buf = heapBuf.data(); + remaining = heapBuf.size() - offset; } - } + } while (numBytesRead != 0); // parseProcLineArray below modifies the buffer while parsing! return android_os_Process_parseProcLineArray( - env, clazz, readBuffer, 0, numberBytesRead, - format, outStrings, outLongs, outFloats); + env, clazz, buf, 0, offset, format, outStrings, outLongs, outFloats); } void android_os_Process_setApplicationObject(JNIEnv* env, jobject clazz, -- cgit v1.2.3-59-g8ed1b From ec5dcca57ec8a5f8b614414a4e1b284e0baf0d89 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Thu, 18 Jul 2024 20:59:06 +0000 Subject: Adjust procfs heap buffer growth limit to 64 MiB The current 1 GiB limit is excessive, and would result in extreme amounts of copying for very large files. Files read by android_os_Process_readProcFile should be well under this new limit, and most should only use the stack buffer anyways. Bug: 351917521 Test: cuttlefish boot Change-Id: I37da934b5e66d9b56da928dabd7e115e2ed6dca7 --- core/jni/android_util_Process.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/jni/android_util_Process.cpp b/core/jni/android_util_Process.cpp index 2efadbbcf85a..43bfa7c70e4e 100644 --- a/core/jni/android_util_Process.cpp +++ b/core/jni/android_util_Process.cpp @@ -1094,7 +1094,8 @@ jboolean android_os_Process_readProcFile(JNIEnv* env, jobject clazz, static_assert(kProcReadMinHeapBufferSize > sizeof(stackBuf)); std::memcpy(heapBuf.data(), stackBuf, sizeof(stackBuf)); } else { - if (heapBuf.size() >= std::numeric_limits::max() / 2) { + constexpr size_t MAX_READABLE_PROCFILE_SIZE = 64 << 20; + if (heapBuf.size() >= MAX_READABLE_PROCFILE_SIZE) { if (kDebugProc) { ALOGW("Proc file too big: %s fd=%d size=%zu\n", file8.get(), fd.get(), heapBuf.size()); -- cgit v1.2.3-59-g8ed1b From 44390105c7a097d1ab516f7a4e363745ac5b2099 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Fri, 19 Jul 2024 23:09:28 +0000 Subject: Fix memory leak of lastArray when newArray == NULL lastArray remains copied/pinned when android_os_Process_getPids returns early because of a failure to allocate newArray. Bug: 351917521 Test: cuttlefish boot Change-Id: I95ebeb36ffc185a21acca11b9393b6dd8f4e0f83 --- core/jni/android_util_Process.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/jni/android_util_Process.cpp b/core/jni/android_util_Process.cpp index 43bfa7c70e4e..3ac1892664d1 100644 --- a/core/jni/android_util_Process.cpp +++ b/core/jni/android_util_Process.cpp @@ -818,7 +818,6 @@ jintArray android_os_Process_getPids(JNIEnv* env, jobject clazz, } DIR* dirp = opendir(file8); - env->ReleaseStringUTFChars(file, file8); if(dirp == NULL) { @@ -851,6 +850,7 @@ jintArray android_os_Process_getPids(JNIEnv* env, jobject clazz, jintArray newArray = env->NewIntArray(newCount); if (newArray == NULL) { closedir(dirp); + if (curData) env->ReleaseIntArrayElements(lastArray, curData, 0); jniThrowException(env, "java/lang/OutOfMemoryError", NULL); return NULL; } -- cgit v1.2.3-59-g8ed1b