Skip to content

Commit

Permalink
Fix memory leaks in IPC
Browse files Browse the repository at this point in the history 
- Replace `cJSON_free` with proper destroy method
  of CJSON object -> `CJSON_Delete`
- Avoid double free in case of structure already
  added to root object - there is no need to
  priv structure anymore
- Add missing `scope_free` for `cJSON_PrintUnformatted` method

Ref: criblio#1108
  • Loading branch information
@michalbiesek
michalbiesek committed Jan 31, 2023
1 parent efc7ab5 commit 44a2409
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 31 deletions.
18 changes: 15 additions & 3 deletions src/ipc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,15 +237,15 @@ ipcParseSingleFrame(const char *msgBuf, ssize_t msgLen, req_parse_status_t *pars
// There is no scope data
if (msgLen <= dataOffset) {
*parseStatus = REQ_PARSE_MISSING_SCOPE_DATA_ERROR;
goto cleanJson;
goto cleanMetadata;
}
// Calculate the scope data length
size_t dataLen = msgLen - dataOffset;
// Allocate place for scope data in the current frame
scopeMsg = scope_calloc(1, sizeof(char) * dataLen);
if (!scopeMsg) {
*parseStatus = REQ_PARSE_ALLOCATION_ERROR;
goto cleanJson;
goto cleanMetadata;
}
// Skip NUL char separator
scope_memcpy(scopeMsg, msgBuf + dataOffset, dataLen);
Expand All @@ -255,6 +255,9 @@ ipcParseSingleFrame(const char *msgBuf, ssize_t msgLen, req_parse_status_t *pars
*parseStatus = REQ_PARSE_PARTIAL;
}

cleanMetadata:
scope_free(metaData);

cleanJson:
cJSON_Delete(msgJson);

Expand Down Expand Up @@ -418,6 +421,7 @@ ipcSendFailedResponse(mqd_t mqDes, size_t msgBufSize, req_parse_status_t parseSt
res = ipcSendFrameWithRetry(mqDes, metadataBytes, metadataLen);

end:
scope_free(metadataBytes);
cJSON_Delete(meta);
return res;
}
Expand Down Expand Up @@ -502,13 +506,16 @@ ipcSendSuccessfulResponse(mqd_t mqDes, size_t msgBufSize, const char *scopeDataR
return res;
}
char *scopeRespBytes = ipcRespScopeRespStr(scopeRespWrap);
if (!scopeRespBytes) {
goto destroyWrap;
}
size_t scopeDataRemainLen = scope_strlen(scopeRespBytes);
size_t scopeDataOffset = 0;

// Allocate buffer to send out
void *frame = scope_malloc(msgBufSize * sizeof(char));
if (!frame) {
goto destroyWrap;
goto destroyScopeRespStr;
}

while (scopeDataRemainLen) {
Expand All @@ -524,6 +531,7 @@ ipcSendSuccessfulResponse(mqd_t mqDes, size_t msgBufSize, const char *scopeDataR
// There is not sufficient place to use msg buffer
if (metadataLen >= msgBufSize) {
res = RESP_UNSUFFICENT_MSGBUF_ERROR;
scope_free(metadataBytes);
cJSON_Delete(metadataJson);
goto destroyFrame;
}
Expand All @@ -549,6 +557,7 @@ ipcSendSuccessfulResponse(mqd_t mqDes, size_t msgBufSize, const char *scopeDataR
}

scope_memcpy(frame, metadataBytes, metadataLen);
scope_free(metadataBytes);
cJSON_Delete(metadataJson);

// Copy the scope frame data
Expand All @@ -567,6 +576,9 @@ ipcSendSuccessfulResponse(mqd_t mqDes, size_t msgBufSize, const char *scopeDataR
destroyFrame:
scope_free(frame);

destroyScopeRespStr:
scope_free(scopeRespBytes);

destroyWrap:
ipcRespWrapperDestroy(scopeRespWrap);

Expand Down
59 changes: 31 additions & 28 deletions src/ipc_resp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,10 @@ extern log_t *g_log;
extern mtc_t *g_mtc;
extern ctl_t *g_ctl;

#define WRAP_PRIV_SIZE (2)

// Wrapper for scope message response
// TODO: replace scopeRespWrapper with cJSON
struct scopeRespWrapper{
cJSON *resp; // Scope message response
void *priv[WRAP_PRIV_SIZE]; // Additional resources allocated to create response
};

/*
Expand All @@ -44,9 +42,6 @@ respWrapperCreate(void) {
return NULL;
}
wrap->resp = NULL;
for (int i = 0; i < WRAP_PRIV_SIZE; ++i) {
wrap->priv[i] = NULL;
}
return wrap;
}

Expand All @@ -56,12 +51,7 @@ respWrapperCreate(void) {
void
ipcRespWrapperDestroy(scopeRespWrapper *wrap) {
if (wrap->resp) {
cJSON_free(wrap->resp);
}
for (int i = 0; i < WRAP_PRIV_SIZE; ++i) {
if (wrap->priv[i]) {
cJSON_free(wrap->priv[i]);
}
cJSON_Delete(wrap->resp);
}

scope_free(wrap);
Expand Down Expand Up @@ -112,12 +102,12 @@ createCmdDesc(int id, const char *name) {
}

if (!cJSON_AddNumberToObject(cmdDesc, "id", id)) {
cJSON_free(cmdDesc);
cJSON_Delete(cmdDesc);
return NULL;
}

if (!cJSON_AddStringToObject(cmdDesc, "name", name)) {
cJSON_free(cmdDesc);
cJSON_Delete(cmdDesc);
return NULL;
}

Expand Down Expand Up @@ -150,33 +140,37 @@ ipcRespGetScopeCmds(const cJSON * unused) {
goto allocFail;
}

wrap->priv[0] = metaCmds;
for (int id = 0; id < ARRAY_SIZE(cmdMetaName); ++id){
cJSON *singleCmd = createCmdDesc(id, cmdMetaName[id]);
if (!singleCmd) {
goto allocFail;
goto metaCmdFail;
}
cJSON_AddItemToArray(metaCmds, singleCmd);
}
cJSON_AddItemToObjectCS(resp, "commands_meta", metaCmds);

cJSON *scopeCmds = cJSON_CreateArray();
if (!scopeCmds) {
goto allocFail;
goto metaCmdFail;
}

wrap->priv[1] = scopeCmds;
for (int id = 0; id < ARRAY_SIZE(cmdScopeName); ++id){
cJSON *singleCmd = createCmdDesc(id, cmdScopeName[id]);
if (!singleCmd) {
goto allocFail;
goto scopeCmdFail;
}
cJSON_AddItemToArray(scopeCmds, singleCmd);
}
cJSON_AddItemToObjectCS(resp, "commands_scope", scopeCmds);

return wrap;

metaCmdFail:
cJSON_Delete(metaCmds);

scopeCmdFail:
cJSON_Delete(scopeCmds);

allocFail:
ipcRespWrapperDestroy(wrap);
return NULL;
Expand Down Expand Up @@ -233,7 +227,6 @@ ipcRespGetScopeCfg(const cJSON *unused) {
}
return wrap;
}
wrap->priv[0] = cfg;

cJSON_AddItemToObjectCS(resp, "cfg", cfg);

Expand Down Expand Up @@ -271,6 +264,7 @@ ipcProcessSetCfg(const cJSON *scopeReq) {
char *cfgStr = cJSON_PrintUnformatted(cfgKey);
config_t *cfg = cfgFromString(cfgStr);
doAndReplaceConfig(cfg);
scope_free(cfgStr);
res = TRUE;
return res;
}
Expand Down Expand Up @@ -416,7 +410,7 @@ ipcRespGetTransportStatus(const cJSON *unused) {
if (!interfaces) {
goto allocFail;
}
wrap->priv[0] = interfaces;

for (int index = 0; index < ARRAY_SIZE(scope_interfaces); ++index) {
int interfaceIndex = index;
// Skip preparing the interface info if it is disabled
Expand All @@ -431,34 +425,40 @@ ipcRespGetTransportStatus(const cJSON *unused) {

cJSON *singleInterface = cJSON_CreateObject();
if (!singleInterface) {
goto allocFail;
goto interfaceFail;
}

transport_status_t status = scope_interfaces[interfaceIndex].status();

if (!cJSON_AddStringToObject(singleInterface, "name", scope_interfaces[interfaceIndex].name)) {
goto allocFail;
cJSON_Delete(singleInterface);
goto interfaceFail;
}

if (!cJSON_AddStringToObject(singleInterface, "config", status.configString)) {
goto allocFail;
cJSON_Delete(singleInterface);
goto interfaceFail;
}

if (status.isConnected == TRUE) {
if (!cJSON_AddTrueToObject(singleInterface, "connected")) {
goto allocFail;
cJSON_Delete(singleInterface);
goto interfaceFail;
}
} else {
if (!cJSON_AddFalseToObject(singleInterface, "connected")) {
goto allocFail;
cJSON_Delete(singleInterface);
goto interfaceFail;
}
if (!cJSON_AddNumberToObject(singleInterface, "attempts", status.connectAttemptCount)) {
goto allocFail;
cJSON_Delete(singleInterface);
goto interfaceFail;
}

if (status.failureString) {
if (!cJSON_AddStringToObject(singleInterface, "failure_details", status.failureString)) {
goto allocFail;
cJSON_Delete(singleInterface);
goto interfaceFail;
}
}
}
Expand All @@ -467,6 +467,9 @@ ipcRespGetTransportStatus(const cJSON *unused) {
cJSON_AddItemToObjectCS(resp, "interfaces", interfaces);
return wrap;

interfaceFail:
cJSON_Delete(interfaces);

allocFail:
ipcRespWrapperDestroy(wrap);
return NULL;
Expand Down

0 comments on commit 44a2409

Please sign in to comment.