Memory leak parsing/relaying from detail files?

[Nathan Ward]

> On 9/03/2016, at 03:10, Alan DeKok <aland at deployingradius.com> wrote:
> 
> On Mar 8, 2016, at 12:10 AM, Nathan Ward <lists+freeradius at daork.net> wrote:
>> I am working through debugging a memory leak I am hitting, and I think I have found the culprit.
>> 
>> The trigger appears to be a malformed NAS-IPv6-Address attribute coming from a Cisco ASR9000, and I will certainly contact the vendor, however it occurs to me that we should work to solve this in FreeRADIUS as well.
> 
>  I've pushed a fix to v3.0.x.  I'll push a fix to v3.1.x later today.
> 
>  The backtrace and gdb output was exactly what I needed to fix it.

Hi Alan,

Thanks for that, that’s awesome.

I have applied your patch, presuming that the only changes in this area are in commit 6b8457f0cfaaf23ee3660d658a8bfa3d7c2b6a2b. I note that there are no other changes to pair.c around that time. If there are changes I’ve missed, please let me know. I am curious whether commit 296eeeb103cac714a58ae64a8130664b527495eb is related, but, I’m not certain.

I noted immediately that the memory leak no longer shows in valgrind, which is good!

However, I have since noted that the memory usage on the process still increases over time, at the same rate as before.
I think what is happening is that now the that the talloc context is set, valgrind no longer recognises it as a leak as it doesn’t consider memory that talloc is managing to be a leak, however we are still allocating memory for something that is never used again (the Attr-95 attribute name) and expanding that talloc context (I don’t know much about talloc, can you tell? :-).


What did you think of my prior suggestion in question 2 of my original email:
2) I would like to figure out this memory leak myself and submit a patch, but I’m not sure how this part of the code works. I *think* that this should be freed after being proxied to the backend server - so, perhaps in process.c somewhere after a packet is sent, it should have any vp das that have is_unknown set to true freed? I’m not clear what impact that would have on other bits of code.


Of course, happy to write the code, but some guidance on whether this is appropriate or likely to harm other processes would be helpful.

Another option that I have considered is whether it would be good to just prevent the detail reader from reading unknown attributes. I imagine that might have some impact on people who rely on sending data which FreeRADIUS doesn’t have dictionaries for, however.


I have managed to filter out the bad attribute, temporarily, by re-defining NAS-IPv6-Address in the dictionary to be “octets” type so that it is visible to unlang and can strip it out before it’s written to the file. I do not have any NASes relying on this attribute, so things are working OK. I also had to remove it from the Acct-Unique-Session-Id generation in the acct_unique policy, so that it would not impact existing sessions while I implemented this change.
I would, of course, like to find a long term solution for this. I can relax somewhat though, which is helpful.


Naturally, I’m progressing this with the vendor for a fix on their end, too.

--
Nathan Ward