Skip to content

Commit

Permalink
Optimize jsonRpc_handleReply.
Browse files Browse the repository at this point in the history 
1. Apply early return to make error handling dead simple.
2. Remove unnecessary argument list iterations.
3. Protect against user-provided nullptr.
4. Deal with invalid text output.
  • Loading branch information
@PengZheng
PengZheng committed Jan 26, 2024
1 parent 28db6c3 commit 56abd7d
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 65 deletions.
93 changes: 92 additions & 1 deletion libs/dfi/gtest/src/json_rpc_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ extern "C" {
rc = jsonRpc_handleReply(dynFunc, reply, args, &rsErrno);
ASSERT_EQ(0, rc);
ASSERT_EQ(0, rsErrno);
//ASSERT_EQ(2.2, result);
EXPECT_DOUBLE_EQ(2.2, result);

dynFunction_destroy(dynFunc);
}
Expand Down Expand Up @@ -720,6 +720,24 @@ TEST_F(JsonRpcTests, handleTestPre) {
handleTestPre();
}

TEST_F(JsonRpcTests, handleTestNullPre) {
dyn_function_type *dynFunc = nullptr;
int rc = dynFunction_parseWithStr("add(#am=handle;PDD#am=pre;*D)N", nullptr, &dynFunc);
ASSERT_EQ(0, rc);

const char *reply = "{\"r\":2.2}";
double *out = NULL;
void *args[4];
args[3] = &out;
int rsErrno = 0;
rc = jsonRpc_handleReply(dynFunc, reply, args, &rsErrno);
ASSERT_EQ(0, rc);
ASSERT_EQ(0, rsErrno);
//ASSERT_EQ(2.2, result);

dynFunction_destroy(dynFunc);
}

TEST_F(JsonRpcTests, handleTestInvalidReply) {
handleTestInvalidReply();
}
Expand All @@ -728,6 +746,42 @@ TEST_F(JsonRpcTests, handleTestOut) {
handleTestOut();
}

TEST_F(JsonRpcTests, handleTestNullOut) {
dyn_interface_type *intf = nullptr;
FILE *desc = fopen("descriptors/example1.descriptor", "r");
ASSERT_TRUE(desc != nullptr);
int rc = dynInterface_parse(desc, &intf);
ASSERT_EQ(0, rc);
fclose(desc);

const struct methods_head* head = dynInterface_methods(intf);
dyn_function_type *func = nullptr;
struct method_entry *entry = nullptr;
TAILQ_FOREACH(entry, head, entries) {
if (strcmp(dynFunction_getName(entry->dynFunc), "stats") == 0) {
func = entry->dynFunc;
break;
}
}
ASSERT_TRUE(func != nullptr);

const char *reply = R"({"r":{"input":[1.0,2.0],"max":2.0,"average":1.5,"min":1.0}})";

void *args[3];
args[0] = nullptr;
args[1] = nullptr;
args[2] = nullptr;

void *out = nullptr;
args[2] = &out;

int rsErrno = 0;
rc = jsonRpc_handleReply(func, reply, args, &rsErrno);
ASSERT_EQ(0, rc);
ASSERT_EQ(0, rsErrno);
dynInterface_destroy(intf);
}

TEST_F(JsonRpcTests, callPreReference) {
dyn_interface_type *intf = nullptr;
FILE *desc = fopen("descriptors/example7.descriptor", "r");
Expand Down Expand Up @@ -798,6 +852,43 @@ TEST_F(JsonRpcTests, handleOutChar) {
handleTestOutChar();
}

TEST_F(JsonRpcTests, handleInvalidOutChar) {

dyn_interface_type *intf = nullptr;
FILE *desc = fopen("descriptors/example4.descriptor", "r");
ASSERT_TRUE(desc != nullptr);
int rc = dynInterface_parse(desc, &intf);
ASSERT_EQ(0, rc);
fclose(desc);

const struct methods_head* head = dynInterface_methods(intf);
dyn_function_type *func = nullptr;
struct method_entry *entry = nullptr;
TAILQ_FOREACH(entry, head, entries) {
if (strcmp(dynFunction_getName(entry->dynFunc), "getName") == 0) {
func = entry->dynFunc;
break;
}
}

ASSERT_TRUE(func != nullptr);

const char *reply = R"({"r": 12345})";
char *result = nullptr;
void *out = &result;

void *args[2];
args[0] = nullptr;
args[1] = &out;

if (func != nullptr) { // Check needed just to satisfy Coverity
int rsErrno = 0;
int status = jsonRpc_handleReply(func, reply, args, &rsErrno);
EXPECT_NE(0, status);
}
dynInterface_destroy(intf);
}

TEST_F(JsonRpcTests, handleReplyError) {
handleTestReplyError();
}
Expand Down
115 changes: 51 additions & 64 deletions libs/dfi/src/json_rpc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,81 +277,68 @@ int jsonRpc_handleReply(const dyn_function_type* func, const char* reply, void*
int status = OK;

json_error_t error;
json_t* replyJson = json_loads(reply, JSON_DECODE_ANY, &error);
json_auto_t* replyJson = json_loads(reply, JSON_DECODE_ANY, &error);
if (replyJson == NULL) {
status = ERROR;
celix_err_pushf("Error parsing json '%s', got error '%s'", reply, error.text);
return ERROR;
}

json_t* result = NULL;
json_t* rsError = NULL;
bool replyHasError = false;
if (status == OK) {
*rsErrno = 0;
result = json_object_get(replyJson, "r");
if (result == NULL) {
rsError = json_object_get(replyJson, "e");
if(rsError != NULL) {
//get the invocation error of remote service function
*rsErrno = (int)json_integer_value(rsError);
replyHasError = true;
}
}
*rsErrno = 0;

const struct dyn_function_arguments_head* arguments = dynFunction_arguments(func);
dyn_function_argument_type* last = TAILQ_LAST(arguments, dyn_function_arguments_head);
const dyn_type* argType = last->type;
enum dyn_function_argument_meta meta = last->argumentMeta;
rsError = json_object_get(replyJson, "e");
if(rsError != NULL) {
//get the invocation error of remote service function
*rsErrno = (int) json_integer_value(rsError);
return OK;
}

int nrOfArgs = dynFunction_nrOfArguments(func);
for (int j = 0; j < nrOfArgs; ++j) {
enum dyn_function_argument_meta meta = dynFunction_argumentMetaForIndex(func, j);
if (meta == DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT || meta == DYN_FUNCTION_ARGUMENT_META__OUTPUT) {
if (result == NULL && !replyHasError) {
status = ERROR;
celix_err_pushf("Expected result in reply. got '%s'", reply);
break;
}
}
if (meta != DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT && meta != DYN_FUNCTION_ARGUMENT_META__OUTPUT) {
return OK;
}

if (status == OK && !replyHasError) {
int i;
for (i = 0; i < nrOfArgs; i += 1) {
const dyn_type* argType = dynFunction_argumentTypeForIndex(func, i);
enum dyn_function_argument_meta meta = dynFunction_argumentMetaForIndex(func, i);
if (meta == DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT) {
void* tmp = NULL;
void** out = (void **) args[i];
size_t size = 0;

argType = dynType_typedPointer_getTypedType(argType);
status = jsonSerializer_deserializeJson(argType, result, &tmp);
if (tmp != NULL) {
size = dynType_size(argType);
memcpy(*out, tmp, size);
}

dynType_free(argType, tmp);
} else if (meta == DYN_FUNCTION_ARGUMENT_META__OUTPUT) {
const dyn_type* subType = dynType_typedPointer_getTypedType(argType);

if (dynType_descriptorType(subType) == 't') {
char*** out = (char ***) args[i];
char** ptrToString = NULL;
status = jsonSerializer_deserializeJson(subType, result, (void**)&ptrToString);
char* s CELIX_UNUSED = *ptrToString; //note for debug
free(ptrToString);
**out = (void*)s;
} else {
const dyn_type* subSubType = dynType_typedPointer_getTypedType(subType);
void*** out = (void ***) args[i];
status = jsonSerializer_deserializeJson(subSubType, result, *out);
}
} else {
//skip
result = json_object_get(replyJson, "r");
if (result == NULL) {
celix_err_pushf("Expected result in reply. got '%s'", reply);
return ERROR;
}
void** lastArg = (void **) args[last->index];
if (*lastArg == NULL) {
// caller provides nullptr, no need to deserialize
return OK;
}
if (meta == DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT) {
void* tmp = NULL;
void** out = lastArg;
size_t size = 0;

argType = dynType_typedPointer_getTypedType(argType);
status = jsonSerializer_deserializeJson(argType, result, &tmp);
if (tmp != NULL) {
size = dynType_size(argType);
memcpy(*out, tmp, size);
dynType_free(argType, tmp);
}
} else {
const dyn_type* subType = dynType_typedPointer_getTypedType(argType);

if (dynType_descriptorType(subType) == 't') {
char*** out = (char ***) lastArg;
char** ptrToString = NULL;
status = jsonSerializer_deserializeJson(subType, result, (void**)&ptrToString);
if (ptrToString != NULL) {
**out = (void*)*ptrToString;
free(ptrToString);
}
} else {
const dyn_type* subSubType = dynType_typedPointer_getTypedType(subType);
void*** out = (void ***) lastArg;
status = jsonSerializer_deserializeJson(subSubType, result, *out);
}
}


json_decref(replyJson);

return status;
}

0 comments on commit 56abd7d

Please sign in to comment.