Hello,
indeed, the pv names cache doesn't invalidate items at runtime, relying on the old model of static name string that evaluates its variables to get the dynamic name value.
I would be fine with solution to overcome this:
1) have some threshold of items/memory after which to start removing items. Ideally, this should be done only with the variables that have dynamic names (e,g for $sht(...), but not for $ru) -- iirc, there are some flags or type field in declaration of the pv that can be extended to cover this case.
2) have a core parameter (might be used only by the app_lua and the other embedded languages modules) to control whether to cache or not pv name specs for vars with dynamic name
One thig to double check is whether we need or not a free function for pv name spec, might be that some particular vars allocate memory some just point to other values...
Somehow it is pretty much obvious there would be security implications when taking values from incoming traffic without sanity/safety checks.
Cheers, Daniel
On 12/07/16 14:19, Timo Teras wrote:
Hi,
I was recently debugging a memory leak in Kamailio running lua code. And it turned out to be rather nasty interaction with the PV cache and the Lua code. Technically, this is bug in Kamailio. However, we ended up fixing the lua code because the same bug also introduces security issues.
The problem is that PV cache never releases cached items. Normally this is not a problem since in .cfg files you can only access PVs with fixed name. In cases where dynamic part is needed, like $sht() access based on variable, the expansion is done in the PV expression evaluation and the actual PV name is still static.
However, app_lua exposes sr.pv.* where the PV name is a string. The lua application is free to construct it as it wishes. The case I had was using lua code to expand a variable, and then do sr.pv.get/sets on the expanded "$sht(table=>"..key..")" type expression.
This resulted PV core leaking cached entry for each unique key. So I wonder if should limit the PV cache size, and e.g. make the bucket chain size limited (start releasing oldest cache entries when the bucket becomes full)? Or if this is not possible, e.g. due to memory allocation semantics, at least put big fat warning to the app_lua's sr.pv.* documentation on this.
I ended up just fixing the application to set it first to "$var(tmp)" and then use that in the PV query. The security implication is noticeable if 'key' comes from untrusted source (e.g. SIP message) as then an attacker can inject string for PV expansion...
Thanks, Timo
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev