Memory leak parsing/relaying from detail files?

[Alan DeKok]

On Apr 14, 2016, at 8:43 PM, Nathan Ward <lists+freeradius at daork.net> wrote:
> 
> 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.

  That's it.

> I am curious whether commit 296eeeb103cac714a58ae64a8130664b527495eb is related, but, I’m not certain.

  It's not related.

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

  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? :-).

  Are you sure it's the same issue?

  i.e. if you update the dictionary to have NAS-IPv6-Address be of type "octets", does the same file now *not* leak memory?

> 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.

  The memory should be freed automatically.

  I've pushed some changes which may help.  The issue is that we have a talloc context.  When the context gets freed, everything in it gets freed.  The context includes a VALUE_PAIR, usually 'vp', and the VALUE_PAIR includes a DICT_ATTR, vp->da.  The da (if unknown) SHOULD be parented from the vp.  So when the vp is freed, so is the da.

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

  The key is to ensure that the da has the correct parent.  It will then be automatically freed as necessary.

> 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.

  Yes.

> 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.

  OK.

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

  That would be good.

  I'll see if there are fixes to the detail file reader which may help.  But the patches I pushed today might fix it...

  Alan DeKok.