[squid-dev] [PREVIEW] Free AccessLogEntry::url when needed

[Amos Jeffries]

On 11/03/2016 12:53 p.m., Nathan Hoad wrote:
> Hello,
> 
> The attached patch is a demonstration of a memory leak on
> AccessLogentry::url, created by ACLFilledChecklist::syncAle(). The
> patch itself is not good enough quality to warrant acceptance, and no
> doubt introduces bugs.
> 
> ::syncAle() is the only place in the codebase that assigns a URL that
> AccessLogEntry is expected to free(), which AccessLogEntry doesn't do.
> This results in a memory leak.
> 
> As mentioned, the patch is of questionable quality, as the chunk in
> src/client_side.cc shows. This sort of change would have to be made
> everywhere that url is assigned to, which isn't practical.
> 
> It would be good to modify ::syncAle() so that it doesn't allocate a
> new URL, but I'm not sure how that could be achieved. I think the only
> workable alternative is to change AccessLogEntry::url to a more
> intelligent string type. There is also the possibility of making the
> member private and using a setter to ensure it is always xstrdup'd
> from another URL.
> 
> If someone with better knowledge than me would like to make a
> recommendation on what to do here, I'd be happy to work towards a
> better solution.

You can probably guess by now that I'm going to say SBuf.

In this case there is also possibly the option of removing that url
variable entirely. A class URL is the proper way to store URLs. But that
could be significantly more work to make URL ref-countable with a Pointer.

FYI:
 SBuf is the intended solution to all our many string related problems.
We are just in the transitional period of converting things.

One issue we are working around during the transition time is that SBuf
data-copies when importing from any non-SBuf type, and when c_str() is
called to export. Effort to avoid those is worthwhile. Use in the wrong
places can reduce performance noticably.
 However, any object which is already being allocated or copied to the
string/char* can be SBuf converted without this being a new cost. And we
then gain improved memory management from it.

Amos