Skip to content

Commit

Permalink
Fix JNI local and global reference leaks in controller (#29236)
Browse files Browse the repository at this point in the history
  • Loading branch information
yunhanw-google authored Oct 5, 2023
1 parent 8296a75 commit ccdbfee
Show file tree
Hide file tree
Showing 8 changed files with 292 additions and 100 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ class PairOnNetworkLongImReadCommand(
eventPath: ChipEventPath?,
e: Exception
) {
if (attributePath != null && attributePath.clusterId.getId() == UNIT_TEST_CLUSTER) {
logger.log(
Level.INFO,
"TODO: skip the error check for unit test cluster that covers most error result"
)
return
}
logger.log(Level.INFO, "Read receive onError")
setFailure("read failure")
}
Expand All @@ -55,23 +62,31 @@ class PairOnNetworkLongImReadCommand(
fun checkStartUpEventJson(event: EventState): Boolean =
event.getJson().toString() == """{"0:STRUCT":{"0:UINT":1}}"""

fun checkAllAttributesJsonForBasicCluster(cluster: String): Boolean {
fun checkAllAttributesJsonForFixedLabel(cluster: String): Boolean {
val expected =
"""{"16:BOOL":false,""" +
""""65531:ARRAY-UINT":[""" +
"""0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,18,19,20,65528,65529,65531,65532,65533]}"""
"""{"65528:ARRAY-?":[],"0:ARRAY-STRUCT":[{"0:STRING":"room","1:STRING":"bedroom 2"},""" +
"""{"0:STRING":"orientation","1:STRING":"North"},{"0:STRING":"floor","1:STRING":"2"},""" +
"""{"0:STRING":"direction","1:STRING":"up"}],"65531:ARRAY-UINT":[0,65528,65529,65531,65532,65533],""" +
""""65533:UINT":1,"65529:ARRAY-?":[],"65532:UINT":0}"""
return cluster.equals(expected)
}

private fun validateResponse(nodeState: NodeState) {
val endpointZero =
requireNotNull(nodeState.getEndpointState(0)) { "Endpoint zero not found." }

val endpointOne = requireNotNull(nodeState.getEndpointState(0)) { "Endpoint one not found." }

val basicCluster =
requireNotNull(endpointZero.getClusterState(CLUSTER_ID_BASIC)) {
"Basic cluster not found."
}

val fixedLabelCluster =
requireNotNull(endpointOne.getClusterState(FIXED_LABEL_CLUSTER)) {
"fixed label cluster not found."
}

val localConfigDisabledAttribute =
requireNotNull(basicCluster.getAttributeState(ATTR_ID_LOCAL_CONFIG_DISABLED)) {
"No local config disabled attribute found."
Expand All @@ -81,7 +96,9 @@ class PairOnNetworkLongImReadCommand(
requireNotNull(basicCluster.getEventState(EVENT_ID_START_UP)) { "No start up event found." }

val clusterAttributes =
requireNotNull(basicCluster.getAttributesJson()) { "No basicCluster attribute found." }
requireNotNull(fixedLabelCluster.getAttributesJson()) {
"No fixed label cluster attribute found."
}

require(checkLocalConfigDisableAttributeTlv(localConfigDisabledAttribute)) {
"Invalid local config disabled attribute TLV ${localConfigDisabledAttribute.getTlv().contentToString()}"
Expand All @@ -101,8 +118,8 @@ class PairOnNetworkLongImReadCommand(
"Invalid start up event Json ${startUpEvents[0].getJson().toString()}"
}

require(checkAllAttributesJsonForBasicCluster(clusterAttributes)) {
"Invalid basic cluster attributes Json ${clusterAttributes}"
require(checkAllAttributesJsonForFixedLabel(clusterAttributes)) {
"Invalid fixed label cluster attributes Json ${clusterAttributes}"
}
}

Expand Down Expand Up @@ -132,9 +149,9 @@ class PairOnNetworkLongImReadCommand(
val attributePathList =
listOf(
ChipAttributePath.newInstance(
ChipPathId.forId(/* endpointId= */ 0),
ChipPathId.forId(CLUSTER_ID_BASIC),
ChipPathId.forId(ATTR_ID_LOCAL_CONFIG_DISABLED),
ChipPathId.forWildcard(),
ChipPathId.forWildcard(),
ChipPathId.forWildcard()
),
ChipAttributePath.newInstance(
ChipPathId.forId(/* endpointId= */ 0),
Expand Down Expand Up @@ -176,6 +193,8 @@ class PairOnNetworkLongImReadCommand(

private const val MATTER_PORT = 5540
private const val CLUSTER_ID_BASIC = 0x0028L
private const val FIXED_LABEL_CLUSTER = 0x0040L
private const val UNIT_TEST_CLUSTER = 0xfff1fc05
private const val ATTR_ID_LOCAL_CONFIG_DISABLED = 16L
private const val EVENT_ID_START_UP = 0L
private const val GLOBAL_ATTRIBUTE_LIST = 65531L
Expand Down
211 changes: 145 additions & 66 deletions src/controller/java/AndroidCallbacks.cpp

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions src/controller/java/AndroidCallbacks.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ struct ReportCallback : public app::ClusterStateCache::Callback
jobject mReportCallbackRef = nullptr;
// NodeState Java object that will be returned to the application.
jobject mNodeStateObj = nullptr;
jclass mNodeStateCls = nullptr;
};

struct WriteAttributesCallback : public app::WriteClient::Callback
Expand Down Expand Up @@ -127,7 +126,7 @@ struct InvokeCallback : public app::CommandSender::Callback

void OnDone(app::CommandSender * apCommandSender) override;

CHIP_ERROR CreateInvokeElement(const app::ConcreteCommandPath & aPath, TLV::TLVReader * apData, jobject & outObj);
CHIP_ERROR CreateInvokeElement(JNIEnv * env, const app::ConcreteCommandPath & aPath, TLV::TLVReader * apData, jobject & outObj);
void ReportError(CHIP_ERROR err);
void ReportError(Protocols::InteractionModel::Status status);
void ReportError(const char * message, ChipError::StorageType errorCode);
Expand Down
15 changes: 13 additions & 2 deletions src/controller/java/AndroidCommissioningWindowOpener.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ AndroidCommissioningWindowOpener::AndroidCommissioningWindowOpener(DeviceControl
{
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
mJavaCallback = env->NewGlobalRef(jCallbackObject);
if (mJavaCallback == nullptr)
{
ChipLogError(Controller, "Failed to create global reference for mJavaCallback");
return;
}

jclass callbackClass = env->GetObjectClass(jCallbackObject);

Expand All @@ -60,7 +65,10 @@ AndroidCommissioningWindowOpener::~AndroidCommissioningWindowOpener()
{
ChipLogError(Controller, "Delete AndroidCommissioningWindowOpener");
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
env->DeleteGlobalRef(mJavaCallback);
if (mJavaCallback != nullptr)
{
env->DeleteGlobalRef(mJavaCallback);
}
}

CHIP_ERROR AndroidCommissioningWindowOpener::OpenBasicCommissioningWindow(DeviceController * controller, NodeId deviceId,
Expand Down Expand Up @@ -112,6 +120,8 @@ void AndroidCommissioningWindowOpener::OnOpenCommissioningWindowResponse(void *
{
auto * self = static_cast<AndroidCommissioningWindowOpener *>(context);
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
VerifyOrReturn(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
JniLocalReferenceManager manager(env);

VerifyOrExit(self->mJavaCallback != nullptr, ChipLogError(Controller, "mJavaCallback is not allocated."));

Expand Down Expand Up @@ -146,10 +156,11 @@ void AndroidCommissioningWindowOpener::OnOpenCommissioningWindowResponse(void *
void AndroidCommissioningWindowOpener::OnOpenBasicCommissioningWindowResponse(void * context, NodeId deviceId, CHIP_ERROR status)
{
auto * self = static_cast<AndroidCommissioningWindowOpener *>(context);

if (self->mJavaCallback != nullptr)
{
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
VerifyOrReturn(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
JniLocalReferenceManager manager(env);
if (status == CHIP_NO_ERROR)
{
if (self->mOnSuccessMethod != nullptr)
Expand Down
11 changes: 7 additions & 4 deletions src/controller/java/AndroidControllerExceptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ CHIP_ERROR AndroidControllerExceptions::CreateAndroidControllerException(JNIEnv
CHIP_ERROR err = JniReferences::GetInstance().GetClassRef(env, "chip/devicecontroller/ChipDeviceControllerException",
controllerExceptionCls);
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_JNI_ERROR_TYPE_NOT_FOUND);
JniClass controllerExceptionJniCls(controllerExceptionCls);

JniObject controllerExceptionJniCls(env, static_cast<jobject>(controllerExceptionCls));

jmethodID exceptionConstructor = env->GetMethodID(controllerExceptionCls, "<init>", "(JLjava/lang/String;)V");
outEx = (jthrowable) env->NewObject(controllerExceptionCls, exceptionConstructor, static_cast<jlong>(errorCode),
env->NewStringUTF(message));
VerifyOrReturnError(outEx != nullptr, CHIP_JNI_ERROR_TYPE_NOT_FOUND);
jobject localRef =
env->NewObject(controllerExceptionCls, exceptionConstructor, static_cast<jlong>(errorCode), env->NewStringUTF(message));
VerifyOrReturnError(localRef != nullptr, CHIP_JNI_ERROR_NULL_OBJECT);
outEx = (jthrowable) (env->NewGlobalRef(localRef));
VerifyOrReturnError(outEx != nullptr, CHIP_JNI_ERROR_NULL_OBJECT);
return CHIP_NO_ERROR;
}

Expand Down
2 changes: 2 additions & 0 deletions src/controller/java/AndroidDeviceControllerWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,8 @@ void AndroidDeviceControllerWrapper::OnCommissioningStatusUpdate(PeerId peerId,
{
chip::DeviceLayer::StackUnlock unlock;
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
VerifyOrReturn(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
JniLocalReferenceManager manager(env);
jmethodID onCommissioningStatusUpdateMethod;
CHIP_ERROR err = JniReferences::GetInstance().FindMethod(env, mJavaObjectRef, "onCommissioningStatusUpdate",
"(JLjava/lang/String;I)V", &onCommissioningStatusUpdateMethod);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
public final class NodeState {
private Map<Integer, EndpointState> endpoints;

public NodeState(Map<Integer, EndpointState> endpoints) {
this.endpoints = endpoints;
public NodeState() {
this.endpoints = new HashMap<>();
}

public Map<Integer, EndpointState> getEndpointStates() {
Expand Down
107 changes: 93 additions & 14 deletions src/lib/support/JniTypeWrappers.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <lib/support/Span.h>
#include <string>

#define JNI_LOCAL_REF_COUNT 256
namespace chip {
/// Exposes the underlying UTF string from a jni string
class JniUtfString
Expand Down Expand Up @@ -87,19 +88,41 @@ class JniByteArray
class UtfString
{
public:
UtfString(JNIEnv * env, const char * data) : mEnv(env) { mData = data != nullptr ? mEnv->NewStringUTF(data) : nullptr; }
UtfString(JNIEnv * env, const char * data) : mEnv(env)
{
jstring localRef = data != nullptr ? mEnv->NewStringUTF(data) : nullptr;
if (localRef == nullptr)
{
return;
}
mData = static_cast<jstring>(env->NewGlobalRef(localRef));
}

UtfString(JNIEnv * env, chip::CharSpan data) : mEnv(env)
{
std::string str(data.data(), data.size());
mData = env->NewStringUTF(str.c_str());
jstring localRef = env->NewStringUTF(str.c_str());
if (localRef == nullptr)
{
return;
}
mData = static_cast<jstring>(env->NewGlobalRef(localRef));
}

~UtfString()
{
if (mEnv != nullptr && mData != nullptr)
{
mEnv->DeleteGlobalRef(mData);
mData = nullptr;
}
}
~UtfString() { mEnv->DeleteLocalRef(mData); }

jstring jniValue() { return mData; }

private:
JNIEnv * mEnv;
jstring mData;
JNIEnv * mEnv = nullptr;
jstring mData = nullptr;
};

/// wrap a byte array as a JNI byte array
Expand All @@ -108,27 +131,39 @@ class ByteArray
public:
ByteArray(JNIEnv * env, const jbyte * data, jsize dataLen) : mEnv(env)
{
mArray = data != nullptr ? mEnv->NewByteArray(dataLen) : nullptr;
if (mArray != nullptr)
jbyteArray localRef = data != nullptr ? mEnv->NewByteArray(dataLen) : nullptr;
if (localRef == nullptr)
{
env->SetByteArrayRegion(mArray, 0, dataLen, data);
return;
}
env->SetByteArrayRegion(localRef, 0, dataLen, data);
mArray = static_cast<jbyteArray>(env->NewGlobalRef(localRef));
}

ByteArray(JNIEnv * env, chip::ByteSpan data) : mEnv(env)
{
mArray = mEnv->NewByteArray(static_cast<jsize>(data.size()));
if (mArray != nullptr)
jbyteArray localRef = mEnv->NewByteArray(static_cast<jsize>(data.size()));
if (localRef == nullptr)
{
env->SetByteArrayRegion(mArray, 0, static_cast<jsize>(data.size()), reinterpret_cast<const jbyte *>(data.data()));
return;
}
env->SetByteArrayRegion(localRef, 0, static_cast<jsize>(data.size()), reinterpret_cast<const jbyte *>(data.data()));
mArray = static_cast<jbyteArray>(env->NewGlobalRef(localRef));
}

~ByteArray()
{
if (mEnv != nullptr && mArray != nullptr)
{
mEnv->DeleteGlobalRef(mArray);
}
}
~ByteArray() { mEnv->DeleteLocalRef(mArray); }

jbyteArray jniValue() { return mArray; }

private:
JNIEnv * mEnv;
jbyteArray mArray;
JNIEnv * mEnv = nullptr;
jbyteArray mArray = nullptr;
};

/// Manages an pre-existing global reference to a jclass.
Expand All @@ -143,4 +178,48 @@ class JniClass
private:
jclass mClassRef;
};

class JniLocalReferenceManager
{
public:
JniLocalReferenceManager(JNIEnv * env) : mEnv(env)
{
if (mEnv->PushLocalFrame(JNI_LOCAL_REF_COUNT) == 0)
{
mlocalFramePushed = true;
}
}
~JniLocalReferenceManager()
{
if (mlocalFramePushed)
{
mEnv->PopLocalFrame(nullptr);
mlocalFramePushed = false;
}
}

private:
JNIEnv * mEnv = nullptr;
bool mlocalFramePushed = false;
};

class JniObject
{
public:
JniObject(JNIEnv * aEnv, jobject aObjectRef) : mEnv(aEnv), mObjectRef(aObjectRef) {}
~JniObject()
{
if (mEnv != nullptr && mObjectRef != nullptr)
{
mEnv->DeleteGlobalRef(mObjectRef);
}
}

jobject objectRef() { return mObjectRef; }

private:
JNIEnv * mEnv = nullptr;
jobject mObjectRef = nullptr;
};

} // namespace chip

0 comments on commit ccdbfee

Please sign in to comment.