aaa_diameter: fix SHM cJSON leaks and double-free found after 1e8001fa0#3882
Open
dondetir wants to merge 2 commits intoOpenSIPS:masterfrom
Open
aaa_diameter: fix SHM cJSON leaks and double-free found after 1e8001fa0#3882dondetir wants to merge 2 commits intoOpenSIPS:masterfrom
dondetir wants to merge 2 commits intoOpenSIPS:masterfrom
Conversation
_dm_release_message_response() is the only function that frees the SHM-backed cJSON reply tree (cond->rpl.json). In dm_send_request(), the release call was gated inside "if (rpl_avps_pv)", so every call without an output PV leaked the entire SHM cJSON tree. The same issue existed in dm_send_request_async_reply(): the release was guarded by "if (rpl_avps)", which was NULL both when no output PV was configured and on the read() failure path — even though the reply callback had already populated cond->rpl.json. _dm_release_message_response() safely handles a NULL rpl_avps argument (cJSON_PurgeString is a no-op on NULL), so removing both guards is safe. Found during a systematic audit of dm_send_request() paths following commit 1e8001f.
After cJSON_AddItemToArray(avps, item) transfers ownership of item to
avps, item is not cleared. Any subsequent FD_CHECK_GT failure in the
same or next loop iteration jumps to the out: label, which calls
cJSON_Delete(item) — freeing memory already owned by avps.
This double-free corrupts the SHM heap, affecting all worker processes.
The affected paths include:
- FD_CHECK_GT(fd_msg_browse()) at the skip: label (next-iteration
advance)
- FD_CHECK_GT(fd_msg_avp_hdr()) and FD_CHECK_GT(fd_dict_getval())
at the start of the next iteration
Fix: clear item immediately after the ownership transfer so that any
later cJSON_Delete(item) at out: is a safe no-op.
Found during a systematic audit following commit 1e8001f.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix SHM cJSON memory leaks and a heap double-free in the
aaa_diametermodule, found during a systematic audit ofdm_send_request()paths following commit1e8001fa0.Details
1e8001fa0established that_dm_release_message_response()is the only correct path for freeing the SHM-backedcond->rpl.jsoncJSON tree. A systematic audit of all callers revealed two paths where the release was silently skipped, plus a related double-free in the AVP parser:dm_send_request()— SHM leak when no output PV (aaa_diameter.c:336–341):_dm_release_message_response()was called only insideif (rpl_avps_pv). When the SIP script calls the function without an output PV argument, the SHM cJSON reply tree leaked on every Diameter response.dm_send_request_async_reply()— SHM leak when no output PV or on read() failure (aaa_diameter.c:524–525): The release was guarded byif (rpl_avps), which was NULL both when no output PV was configured (the async counterpart of bug 1) and on theread()failure path — even though the Diameter reply callback had already populatedcond->rpl.json.dm_avps2json()— SHM heap double-free (dm_impl.c:525–536):cJSON_AddItemToArray(avps, item)transfers ownership ofitemtoavps, butitemwas not cleared afterward. Any subsequentFD_CHECK_GTfailure in the loop (fd_msg_browse,fd_msg_avp_hdr, orfd_dict_getval) jumps toout:, which callscJSON_Delete(item)— double-freeing SHM memory and corrupting the shared heap across all worker processes.dm_send_request()call (sync or async) without an output PV, leaking shared memory proportional to request volume. Bug 3 triggers on any freeDiameter library error during AVP iteration after at least one AVP has been successfully parsed.Solution
aaa_diameter.c(Bugs 1 & 2): Moved_dm_release_message_response()outside theif (rpl_avps_pv)/if (rpl_avps)guards in bothdm_send_request()anddm_send_request_async_reply()._dm_release_message_response()safely handles a NULLrpl_avpsargument (cJSON_PurgeStringis a no-op on NULL, andcond->rpl.jsonis checked before deletion), so the unconditional call is safe in all cases.dm_impl.c(Bug 3): Addeditem = NULLimmediately aftercJSON_AddItemToArray(avps, item). This clears the pointer after ownership transfer, making any subsequentcJSON_Delete(item)atout:a safe no-op (cJSON_Delete(NULL)is defined to do nothing).Remaining concerns: None.
Compatibility
Closing Issues
N/A — found during a systematic audit of
dm_send_request()paths following commit1e8001fa0.