Skip to content

Commit ccd2b67

Browse files
jkstrickjoeferner
jkstrick
authored and
joeferner
committed
Fix clobbering of dynamicProxyData with concurrent proxy calls.
1 parent d0e64ce commit ccd2b67

2 files changed

Lines changed: 48 additions & 37 deletions

File tree

‎src/java.cpp‎

Lines changed: 43 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1226,18 +1226,22 @@ void EIO_AfterCallJs(uv_work_t* req, int status) {
12261226
#else
12271227
void EIO_AfterCallJs(uv_work_t* req) {
12281228
#endif
1229-
DynamicProxyData* dynamicProxyData = static_cast<DynamicProxyData*>(req->data);
1229+
DynamicProxyJsCallData* callData = static_cast<DynamicProxyJsCallData*>(req->data);
1230+
DynamicProxyData* dynamicProxyData = callData->dynamicProxyData;
1231+
1232+
assert(callData->done == 0);
1233+
12301234
if(!dynamicProxyDataVerify(dynamicProxyData)) {
12311235
return;
12321236
}
1233-
dynamicProxyData->result = NULL;
1237+
callData->result = NULL;
12341238

12351239
JNIEnv* env;
12361240
int ret = dynamicProxyData->java->getJvm()->GetEnv((void**)&env, JNI_BEST_VERSION);
12371241
if (ret != JNI_OK) {
1238-
dynamicProxyData->throwableClass = "java/lang/IllegalStateException";
1239-
dynamicProxyData->throwableMessage = "Could not retrieve JNIEnv: jvm->GetEnv returned " + to_string<int>(ret);
1240-
dynamicProxyData->done = DYNAMIC_PROXY_JS_ERROR;
1242+
callData->throwableClass = "java/lang/IllegalStateException";
1243+
callData->throwableMessage = "Could not retrieve JNIEnv: jvm->GetEnv returned " + to_string<int>(ret);
1244+
callData->done = DYNAMIC_PROXY_JS_ERROR;
12411245
return;
12421246
}
12431247

@@ -1251,24 +1255,24 @@ void EIO_AfterCallJs(uv_work_t* req) {
12511255
jobject javaResult;
12521256

12531257
v8::Local<v8::Object> dynamicProxyDataFunctions = Nan::New(dynamicProxyData->functions);
1254-
v8::Local<v8::Value> fnObj = dynamicProxyDataFunctions->Get(Nan::GetCurrentContext(), Nan::New<v8::String>(dynamicProxyData->methodName.c_str()).ToLocalChecked()).ToLocalChecked();
1258+
v8::Local<v8::Value> fnObj = dynamicProxyDataFunctions->Get(Nan::GetCurrentContext(), Nan::New<v8::String>(callData->methodName.c_str()).ToLocalChecked()).ToLocalChecked();
12551259
if(fnObj->IsUndefined() || fnObj->IsNull()) {
1256-
dynamicProxyData->throwableClass = "java/lang/NoSuchMethodError";
1257-
dynamicProxyData->throwableMessage = "Could not find js function " + dynamicProxyData->methodName;
1258-
dynamicProxyData->done = DYNAMIC_PROXY_JS_ERROR;
1260+
callData->throwableClass = "java/lang/NoSuchMethodError";
1261+
callData->throwableMessage = "Could not find js function " + callData->methodName;
1262+
callData->done = DYNAMIC_PROXY_JS_ERROR;
12591263
return;
12601264
}
12611265
if(!fnObj->IsFunction()) {
1262-
dynamicProxyData->throwableClass = "java/lang/IllegalStateException";
1263-
dynamicProxyData->throwableMessage = dynamicProxyData->methodName + " is not a function";
1264-
dynamicProxyData->done = DYNAMIC_PROXY_JS_ERROR;
1266+
callData->throwableClass = "java/lang/IllegalStateException";
1267+
callData->throwableMessage = callData->methodName + " is not a function";
1268+
callData->done = DYNAMIC_PROXY_JS_ERROR;
12651269
return;
12661270
}
12671271

12681272
fn = fnObj.As<v8::Function>();
12691273

1270-
if(dynamicProxyData->args) {
1271-
v8Args = v8::Array::Cast(*javaArrayToV8(dynamicProxyData->java, env, dynamicProxyData->args));
1274+
if(callData->args) {
1275+
v8Args = v8::Array::Cast(*javaArrayToV8(dynamicProxyData->java, env, callData->args));
12721276
argc = v8Args->Length();
12731277
} else {
12741278
argc = 0;
@@ -1283,11 +1287,11 @@ void EIO_AfterCallJs(uv_work_t* req) {
12831287
v8Result = Nan::Call(fn, dynamicProxyDataFunctions, argc, argv).FromMaybe(v8::Local<v8::Value>());
12841288
delete[] argv;
12851289
if (tryCatch.HasCaught()) {
1286-
dynamicProxyData->throwableClass = "node/NodeJsException";
1290+
callData->throwableClass = "node/NodeJsException";
12871291
Nan::Utf8String message(tryCatch.Message()->Get());
1288-
dynamicProxyData->throwableMessage = std::string(*message);
1292+
callData->throwableMessage = std::string(*message);
12891293
tryCatch.Reset();
1290-
dynamicProxyData->done = DYNAMIC_PROXY_JS_ERROR;
1294+
callData->done = DYNAMIC_PROXY_JS_ERROR;
12911295
return;
12921296
}
12931297

@@ -1297,12 +1301,12 @@ void EIO_AfterCallJs(uv_work_t* req) {
12971301

12981302
javaResult = v8ToJava(env, v8Result);
12991303
if(javaResult == NULL) {
1300-
dynamicProxyData->result = NULL;
1304+
callData->result = NULL;
13011305
} else {
1302-
dynamicProxyData->result = env->NewGlobalRef(javaResult);
1306+
callData->result = env->NewGlobalRef(javaResult);
13031307
}
13041308

1305-
dynamicProxyData->done = true;
1309+
callData->done = true;
13061310
}
13071311

13081312
void throwNewThrowable(JNIEnv* env, const char * excClassName, std::string msg) {
@@ -1319,21 +1323,24 @@ JNIEXPORT jobject JNICALL Java_node_NodeDynamicProxyClass_callJs(JNIEnv *env, jo
13191323

13201324
bool hasArgsGlobalRef = false;
13211325

1322-
// args needs to be global, you can't send env across thread boundaries
13231326
DynamicProxyData* dynamicProxyData = (DynamicProxyData*)ptr;
1324-
dynamicProxyData->args = args;
1325-
dynamicProxyData->done = false;
1326-
dynamicProxyData->result = NULL;
1327-
dynamicProxyData->throwableClass = "";
1328-
dynamicProxyData->throwableMessage = "";
1327+
1328+
// args needs to be global, you can't send env across thread boundaries
1329+
DynamicProxyJsCallData callData;
1330+
callData.dynamicProxyData = dynamicProxyData;
1331+
callData.args = args;
1332+
callData.done = false;
1333+
callData.result = NULL;
1334+
callData.throwableClass = "";
1335+
callData.throwableMessage = "";
13291336

13301337
jclass methodClazz = env->FindClass("java/lang/reflect/Method");
13311338
jmethodID method_getName = env->GetMethodID(methodClazz, "getName", "()Ljava/lang/String;");
1332-
dynamicProxyData->methodName = javaObjectToString(env, env->CallObjectMethod(method, method_getName));
1339+
callData.methodName = javaObjectToString(env, env->CallObjectMethod(method, method_getName));
13331340
assertNoException(env);
13341341

13351342
uv_work_t* req = new uv_work_t();
1336-
req->data = dynamicProxyData;
1343+
req->data = &callData; // we wait for work to finish, so ok to pass ref to local var
13371344
if(v8ThreadIdEquals(myThreadId, v8ThreadId)) {
13381345
#if NODE_MINOR_VERSION >= 10
13391346
EIO_AfterCallJs(req, 0);
@@ -1343,13 +1350,13 @@ JNIEXPORT jobject JNICALL Java_node_NodeDynamicProxyClass_callJs(JNIEnv *env, jo
13431350
} else {
13441351
if (args) {
13451352
// if args is not null and we have to kick this across the thread boundary, make it a global ref
1346-
dynamicProxyData->args = (jobjectArray) env->NewGlobalRef(args);
1353+
callData.args = (jobjectArray) env->NewGlobalRef(args);
13471354
hasArgsGlobalRef = true;
13481355
}
13491356

13501357
uv_queue_work(uv_default_loop(), req, EIO_CallJs, (uv_after_work_cb)EIO_AfterCallJs);
13511358

1352-
while(!dynamicProxyData->done) {
1359+
while(!callData.done) {
13531360
my_sleep(100);
13541361
}
13551362
}
@@ -1358,18 +1365,18 @@ JNIEXPORT jobject JNICALL Java_node_NodeDynamicProxyClass_callJs(JNIEnv *env, jo
13581365
throwNewThrowable(env, "java/lang/IllegalStateException", "dynamicProxyData was corrupted");
13591366
}
13601367
if(hasArgsGlobalRef) {
1361-
env->DeleteGlobalRef(dynamicProxyData->args);
1368+
env->DeleteGlobalRef(callData.args);
13621369
}
13631370

1364-
if (dynamicProxyData->done == DYNAMIC_PROXY_JS_ERROR) {
1365-
throwNewThrowable(env, dynamicProxyData->throwableClass.c_str(), dynamicProxyData->throwableMessage);
1371+
if (callData.done == DYNAMIC_PROXY_JS_ERROR) {
1372+
throwNewThrowable(env, callData.throwableClass.c_str(), callData.throwableMessage);
13661373
}
13671374

13681375
jobject result = NULL;
1369-
if(dynamicProxyData->result) {
1376+
if(callData.result) {
13701377
// need to retain a local ref so that we can return it, otherwise the returned object gets corrupted
1371-
result = env->NewLocalRef(dynamicProxyData->result);
1372-
env->DeleteGlobalRef(dynamicProxyData->result);
1378+
result = env->NewLocalRef(callData.result);
1379+
env->DeleteGlobalRef(callData.result);
13731380
}
13741381
return result;
13751382
}

‎src/utils.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,17 @@ struct DynamicProxyData {
3737
std::string interfaceName;
3838
Nan::Persistent<v8::Object> functions;
3939
Nan::Persistent<v8::Value> jsObject;
40+
unsigned int markerEnd;
41+
};
42+
43+
struct DynamicProxyJsCallData {
44+
DynamicProxyData *dynamicProxyData;
4045
std::string methodName;
4146
jobjectArray args;
4247
jobject result;
4348
std::string throwableClass;
4449
std::string throwableMessage;
4550
int done;
46-
unsigned int markerEnd;
4751
};
4852

4953
#define DYNAMIC_PROXY_DATA_MARKER_START 0x12345678

0 commit comments

Comments
 (0)