Fix "using JNI after critical" exceptions
Refactored ScopedJavaNioBuffer implementation to copy
directly to std::vector<uint8_t> instances and release
the jni critical array queries immediately after copying
into a vector. This is to avoid potential interleavings
of JNI method calls before destructor of ScopedJavaNioBuffer
is invoked. As per jni requirements no other JNI calls are
allowed after GetPrimitiveArrayCritical until a matching call
to ReleasePrimitiveArrayCritical is made.
Fixes: 271468824
Test: Added tests to MeshTest
Change-Id: Ia4afd8b8584ae43f021c79b6a341b05e80e0e205
diff --git a/libs/hwui/Android.bp b/libs/hwui/Android.bp
index 536bb49..9c967a2 100644
--- a/libs/hwui/Android.bp
+++ b/libs/hwui/Android.bp
@@ -338,6 +338,7 @@
"jni/android_util_PathParser.cpp",
"jni/Bitmap.cpp",
+ "jni/BufferUtils.cpp",
"jni/HardwareBufferHelpers.cpp",
"jni/BitmapFactory.cpp",
"jni/ByteBufferStreamAdaptor.cpp",
diff --git a/libs/hwui/Mesh.h b/libs/hwui/Mesh.h
index 9836817..13e3c8e 100644
--- a/libs/hwui/Mesh.h
+++ b/libs/hwui/Mesh.h
@@ -104,33 +104,31 @@
class Mesh {
public:
- Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode, const void* vertexBuffer,
- size_t vertexBufferSize, jint vertexCount, jint vertexOffset,
+ Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode,
+ std::vector<uint8_t>&& vertexBufferData, jint vertexCount, jint vertexOffset,
std::unique_ptr<MeshUniformBuilder> builder, const SkRect& bounds)
: mMeshSpec(meshSpec)
, mMode(mode)
+ , mVertexBufferData(std::move(vertexBufferData))
, mVertexCount(vertexCount)
, mVertexOffset(vertexOffset)
, mBuilder(std::move(builder))
- , mBounds(bounds) {
- copyToVector(mVertexBufferData, vertexBuffer, vertexBufferSize);
- }
+ , mBounds(bounds) {}
- Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode, const void* vertexBuffer,
- size_t vertexBufferSize, jint vertexCount, jint vertexOffset, const void* indexBuffer,
- size_t indexBufferSize, jint indexCount, jint indexOffset,
+ Mesh(const sk_sp<SkMeshSpecification>& meshSpec, int mode,
+ std::vector<uint8_t>&& vertexBufferData, jint vertexCount, jint vertexOffset,
+ std::vector<uint8_t>&& indexBuffer, jint indexCount, jint indexOffset,
std::unique_ptr<MeshUniformBuilder> builder, const SkRect& bounds)
: mMeshSpec(meshSpec)
, mMode(mode)
+ , mVertexBufferData(std::move(vertexBufferData))
, mVertexCount(vertexCount)
, mVertexOffset(vertexOffset)
+ , mIndexBufferData(std::move(indexBuffer))
, mIndexCount(indexCount)
, mIndexOffset(indexOffset)
, mBuilder(std::move(builder))
- , mBounds(bounds) {
- copyToVector(mVertexBufferData, vertexBuffer, vertexBufferSize);
- copyToVector(mIndexBufferData, indexBuffer, indexBufferSize);
- }
+ , mBounds(bounds) {}
Mesh(Mesh&&) = default;
@@ -180,13 +178,6 @@
MeshUniformBuilder* uniformBuilder() { return mBuilder.get(); }
private:
- void copyToVector(std::vector<uint8_t>& dst, const void* src, size_t srcSize) {
- if (src) {
- dst.resize(srcSize);
- memcpy(dst.data(), src, srcSize);
- }
- }
-
sk_sp<SkMeshSpecification> mMeshSpec;
int mMode = 0;
diff --git a/libs/hwui/jni/BufferUtils.cpp b/libs/hwui/jni/BufferUtils.cpp
new file mode 100644
index 0000000..3eb08d7
--- /dev/null
+++ b/libs/hwui/jni/BufferUtils.cpp
@@ -0,0 +1,130 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "BufferUtils.h"
+
+#include "graphics_jni_helpers.h"
+
+static void copyToVector(std::vector<uint8_t>& dst, const void* src, size_t srcSize) {
+ if (src) {
+ dst.resize(srcSize);
+ memcpy(dst.data(), src, srcSize);
+ }
+}
+
+/**
+ * This code is taken and modified from com_google_android_gles_jni_GLImpl.cpp to extract data
+ * from a java.nio.Buffer.
+ */
+static void* getDirectBufferPointer(JNIEnv* env, jobject buffer) {
+ if (buffer == nullptr) {
+ return nullptr;
+ }
+
+ jint position;
+ jint limit;
+ jint elementSizeShift;
+ jlong pointer;
+ pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
+ if (pointer == 0) {
+ jniThrowException(env, "java/lang/IllegalArgumentException",
+ "Must use a native order direct Buffer");
+ return nullptr;
+ }
+ pointer += position << elementSizeShift;
+ return reinterpret_cast<void*>(pointer);
+}
+
+static void releasePointer(JNIEnv* env, jarray array, void* data, jboolean commit) {
+ env->ReleasePrimitiveArrayCritical(array, data, commit ? 0 : JNI_ABORT);
+}
+
+static void* getPointer(JNIEnv* env, jobject buffer, jarray* array, jint* remaining, jint* offset) {
+ jint position;
+ jint limit;
+ jint elementSizeShift;
+
+ jlong pointer;
+ pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
+ *remaining = (limit - position) << elementSizeShift;
+ if (pointer != 0L) {
+ *array = nullptr;
+ pointer += position << elementSizeShift;
+ return reinterpret_cast<void*>(pointer);
+ }
+
+ *array = jniGetNioBufferBaseArray(env, buffer);
+ *offset = jniGetNioBufferBaseArrayOffset(env, buffer);
+ return nullptr;
+}
+
+/**
+ * This is a copy of
+ * static void android_glBufferData__IILjava_nio_Buffer_2I
+ * from com_google_android_gles_jni_GLImpl.cpp
+ */
+static void setIndirectData(JNIEnv* env, size_t size, jobject data_buf,
+ std::vector<uint8_t>& result) {
+ jint exception = 0;
+ const char* exceptionType = nullptr;
+ const char* exceptionMessage = nullptr;
+ jarray array = nullptr;
+ jint bufferOffset = 0;
+ jint remaining;
+ void* data = 0;
+ char* dataBase = nullptr;
+
+ if (data_buf) {
+ data = getPointer(env, data_buf, (jarray*)&array, &remaining, &bufferOffset);
+ if (remaining < size) {
+ exception = 1;
+ exceptionType = "java/lang/IllegalArgumentException";
+ exceptionMessage = "remaining() < size < needed";
+ goto exit;
+ }
+ }
+ if (data_buf && data == nullptr) {
+ dataBase = (char*)env->GetPrimitiveArrayCritical(array, (jboolean*)0);
+ data = (void*)(dataBase + bufferOffset);
+ }
+
+ copyToVector(result, data, size);
+
+exit:
+ if (array) {
+ releasePointer(env, array, (void*)dataBase, JNI_FALSE);
+ }
+ if (exception) {
+ jniThrowException(env, exceptionType, exceptionMessage);
+ }
+}
+
+std::vector<uint8_t> copyJavaNioBufferToVector(JNIEnv* env, jobject buffer, size_t size,
+ jboolean isDirect) {
+ std::vector<uint8_t> data;
+ if (buffer == nullptr) {
+ jniThrowNullPointerException(env);
+ } else {
+ if (isDirect) {
+ void* directBufferPtr = getDirectBufferPointer(env, buffer);
+ if (directBufferPtr) {
+ copyToVector(data, directBufferPtr, size);
+ }
+ } else {
+ setIndirectData(env, size, buffer, data);
+ }
+ }
+ return data;
+}
diff --git a/libs/hwui/jni/BufferUtils.h b/libs/hwui/jni/BufferUtils.h
new file mode 100644
index 0000000..b43c320
--- /dev/null
+++ b/libs/hwui/jni/BufferUtils.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef BUFFERUTILS_H_
+#define BUFFERUTILS_H_
+
+#include <jni.h>
+
+#include <vector>
+
+/**
+ * Helper method to load a java.nio.Buffer instance into a vector. This handles
+ * both direct and indirect buffers and promptly releases any critical arrays that
+ * have been retrieved in order to avoid potential jni exceptions due to interleaved
+ * jni calls between get/release primitive method invocations.
+ */
+std::vector<uint8_t> copyJavaNioBufferToVector(JNIEnv* env, jobject buffer, size_t size,
+ jboolean isDirect);
+
+#endif // BUFFERUTILS_H_
diff --git a/libs/hwui/jni/android_graphics_Mesh.cpp b/libs/hwui/jni/android_graphics_Mesh.cpp
index 04339dc..5cb43e5 100644
--- a/libs/hwui/jni/android_graphics_Mesh.cpp
+++ b/libs/hwui/jni/android_graphics_Mesh.cpp
@@ -14,172 +14,18 @@
* limitations under the License.
*/
-#include <GrDirectContext.h>
#include <Mesh.h>
#include <SkMesh.h>
#include <jni.h>
-#include <log/log.h>
#include <utility>
+#include "BufferUtils.h"
#include "GraphicsJNI.h"
#include "graphics_jni_helpers.h"
#define gIndexByteSize 2
-// A smart pointer that provides read only access to Java.nio.Buffer. This handles both
-// direct and indrect buffers, allowing access to the underlying data in both
-// situations. If passed a null buffer, we will throw NullPointerException,
-// and c_data will return nullptr.
-//
-// This class draws from com_google_android_gles_jni_GLImpl.cpp for Buffer to void *
-// conversion.
-class ScopedJavaNioBuffer {
-public:
- ScopedJavaNioBuffer(JNIEnv* env, jobject buffer, size_t size, jboolean isDirect)
- : mEnv(env), mBuffer(buffer) {
- if (buffer == nullptr) {
- mDataBase = nullptr;
- mData = nullptr;
- jniThrowNullPointerException(env);
- } else {
- mArray = (jarray) nullptr;
- if (isDirect) {
- mData = getDirectBufferPointer(mEnv, mBuffer);
- } else {
- mData = setIndirectData(size);
- }
- }
- }
-
- ScopedJavaNioBuffer(ScopedJavaNioBuffer&& rhs) noexcept { *this = std::move(rhs); }
-
- ~ScopedJavaNioBuffer() { reset(); }
-
- void reset() {
- if (mDataBase) {
- releasePointer(mEnv, mArray, mDataBase, JNI_FALSE);
- mDataBase = nullptr;
- }
- }
-
- ScopedJavaNioBuffer& operator=(ScopedJavaNioBuffer&& rhs) noexcept {
- if (this != &rhs) {
- reset();
-
- mEnv = rhs.mEnv;
- mBuffer = rhs.mBuffer;
- mDataBase = rhs.mDataBase;
- mData = rhs.mData;
- mArray = rhs.mArray;
- rhs.mEnv = nullptr;
- rhs.mData = nullptr;
- rhs.mBuffer = nullptr;
- rhs.mArray = nullptr;
- rhs.mDataBase = nullptr;
- }
- return *this;
- }
-
- const void* data() const { return mData; }
-
-private:
- /**
- * This code is taken and modified from com_google_android_gles_jni_GLImpl.cpp to extract data
- * from a java.nio.Buffer.
- */
- void* getDirectBufferPointer(JNIEnv* env, jobject buffer) {
- if (buffer == nullptr) {
- return nullptr;
- }
-
- jint position;
- jint limit;
- jint elementSizeShift;
- jlong pointer;
- pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
- if (pointer == 0) {
- jniThrowException(mEnv, "java/lang/IllegalArgumentException",
- "Must use a native order direct Buffer");
- return nullptr;
- }
- pointer += position << elementSizeShift;
- return reinterpret_cast<void*>(pointer);
- }
-
- static void releasePointer(JNIEnv* env, jarray array, void* data, jboolean commit) {
- env->ReleasePrimitiveArrayCritical(array, data, commit ? 0 : JNI_ABORT);
- }
-
- static void* getPointer(JNIEnv* env, jobject buffer, jarray* array, jint* remaining,
- jint* offset) {
- jint position;
- jint limit;
- jint elementSizeShift;
-
- jlong pointer;
- pointer = jniGetNioBufferFields(env, buffer, &position, &limit, &elementSizeShift);
- *remaining = (limit - position) << elementSizeShift;
- if (pointer != 0L) {
- *array = nullptr;
- pointer += position << elementSizeShift;
- return reinterpret_cast<void*>(pointer);
- }
-
- *array = jniGetNioBufferBaseArray(env, buffer);
- *offset = jniGetNioBufferBaseArrayOffset(env, buffer);
- return nullptr;
- }
-
- /**
- * This is a copy of
- * static void android_glBufferData__IILjava_nio_Buffer_2I
- * from com_google_android_gles_jni_GLImpl.cpp
- */
- void* setIndirectData(size_t size) {
- jint exception;
- const char* exceptionType;
- const char* exceptionMessage;
- jint bufferOffset = (jint)0;
- jint remaining;
- void* tempData;
-
- if (mBuffer) {
- tempData =
- (void*)getPointer(mEnv, mBuffer, (jarray*)&mArray, &remaining, &bufferOffset);
- if (remaining < size) {
- exception = 1;
- exceptionType = "java/lang/IllegalArgumentException";
- exceptionMessage = "remaining() < size < needed";
- goto exit;
- }
- }
- if (mBuffer && tempData == nullptr) {
- mDataBase = (char*)mEnv->GetPrimitiveArrayCritical(mArray, (jboolean*)0);
- tempData = (void*)(mDataBase + bufferOffset);
- }
- return tempData;
- exit:
- if (mArray) {
- releasePointer(mEnv, mArray, (void*)(mDataBase), JNI_FALSE);
- }
- if (exception) {
- jniThrowException(mEnv, exceptionType, exceptionMessage);
- }
- return nullptr;
- }
-
- JNIEnv* mEnv;
-
- // Java Buffer data
- void* mData;
- jobject mBuffer;
-
- // Indirect Buffer Data
- jarray mArray;
- char* mDataBase;
-};
-
namespace android {
static jlong make(JNIEnv* env, jobject, jlong meshSpec, jint mode, jobject vertexBuffer,
@@ -187,9 +33,12 @@
jfloat right, jfloat bottom) {
auto skMeshSpec = sk_ref_sp(reinterpret_cast<SkMeshSpecification*>(meshSpec));
size_t bufferSize = vertexCount * skMeshSpec->stride();
- auto buff = ScopedJavaNioBuffer(env, vertexBuffer, bufferSize, isDirect);
+ auto buffer = copyJavaNioBufferToVector(env, vertexBuffer, bufferSize, isDirect);
+ if (env->ExceptionCheck()) {
+ return 0;
+ }
auto skRect = SkRect::MakeLTRB(left, top, right, bottom);
- auto meshPtr = new Mesh(skMeshSpec, mode, buff.data(), bufferSize, vertexCount, vertexOffset,
+ auto meshPtr = new Mesh(skMeshSpec, mode, std::move(buffer), vertexCount, vertexOffset,
std::make_unique<MeshUniformBuilder>(skMeshSpec), skRect);
auto [valid, msg] = meshPtr->validate();
if (!valid) {
@@ -205,11 +54,17 @@
auto skMeshSpec = sk_ref_sp(reinterpret_cast<SkMeshSpecification*>(meshSpec));
auto vertexBufferSize = vertexCount * skMeshSpec->stride();
auto indexBufferSize = indexCount * gIndexByteSize;
- auto vBuf = ScopedJavaNioBuffer(env, vertexBuffer, vertexBufferSize, isVertexDirect);
- auto iBuf = ScopedJavaNioBuffer(env, indexBuffer, indexBufferSize, isIndexDirect);
+ auto vBuf = copyJavaNioBufferToVector(env, vertexBuffer, vertexBufferSize, isVertexDirect);
+ if (env->ExceptionCheck()) {
+ return 0;
+ }
+ auto iBuf = copyJavaNioBufferToVector(env, indexBuffer, indexBufferSize, isIndexDirect);
+ if (env->ExceptionCheck()) {
+ return 0;
+ }
auto skRect = SkRect::MakeLTRB(left, top, right, bottom);
- auto meshPtr = new Mesh(skMeshSpec, mode, vBuf.data(), vertexBufferSize, vertexCount,
- vertexOffset, iBuf.data(), indexBufferSize, indexCount, indexOffset,
+ auto meshPtr = new Mesh(skMeshSpec, mode, std::move(vBuf), vertexCount, vertexOffset,
+ std::move(iBuf), indexCount, indexOffset,
std::make_unique<MeshUniformBuilder>(skMeshSpec), skRect);
auto [valid, msg] = meshPtr->validate();
if (!valid) {