siptrace: siptrace: fix memory leak in fake replies tracing #2295
Conversation
src/modules/siptrace/siptrace.c
Outdated
| /* check if from header has been already parsed. | ||
| * If not we have to parse it in pkg memory and free iit at the end. | ||
| */ | ||
| if (msg->from && msg->from->parsed != NULL) |
There was a problem hiding this comment.
Should the above condition have:
msg->from->parsed == NULL
Otherwise it should be already parsed.
There is also a small typo in the comment: iit.
There was a problem hiding this comment.
you're right, I wrongly copied from local. I'm going to fix and force push.
|
|
||
| end: | ||
| if (faked && parsed_f) | ||
| free_from(msg->from->parsed); |
There was a problem hiding this comment.
At least for safety, probably msg->from->parsed should be set back to NULL, not to be freed again in other places.
grumvalski
force-pushed
the
grumvalski/siptrace_memleak_fix
branch
from
c366b54
to
ee7496f
Compare
|
Looking at the function, I think two other |
|
Thank you Daniel and, again, you are right, I was working on 5.3 branch and didn't realize the new sip_trace_msg_attrs function. I'm pushing a fix. This also means that the patch cannot be backported to 5.3 as it is. |
| @@ -1383,7 +1383,7 @@ static void trace_onreq_out(struct cell *t, int type, struct tmcb_params *ps) | |||
| } | |||
|
|
|||
| if(sip_trace_msg_attrs(msg, &sto) < 0) { | |||
| return; | |||
| goto end; | |||
| } | |||
There was a problem hiding this comment.
This one is in another function -- mistake?
There was a problem hiding this comment.
sorry, definitely not my day today...
grumvalski
force-pushed
the
grumvalski/siptrace_memleak_fix
branch
from
f2e6758
to
0f461ee
Compare
|
No worries, I had worse days. It is ok to merge. The patch doesn't look that big to be converted and applied for what 5.3 needs. Thanks. |
Pre-Submission Checklist
in
doc/subfolder, the README file is autogenerated)Type Of Change
Checklist:
Description
When tracing a faked reply (like 200 for CANCEL), the sip msg used to retrieve the tracing parameters is the transaction's uas one (uas.request) which might have the from header parsed or not, depending on the routing script actions (by default, if I'm not wrong, it is not parsed). If the header is not parsed, calling get_from in trace_onreply_out function will lead to a memory leak, beacause the header is parsed in pkg memory not freed when the transaction is deleted.