On 12 Dec 2013, at 2:00 AM, Kean Johnston <kean.johns...@gmail.com> wrote:
> So I've been spending a fair bit of time inside Apache recently and I've seen
> a pattern. Consider the following code (from mod_proxy_fcgi.c):
>
> apr_uri_t *uri = apr_palloc(r->pool, sizeof(*uri));
>
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01076)
> "url: %s proxyname: %s proxyport: %d",
> url, proxyname, proxyport);
>
> if (strncasecmp(url, "fcgi:", 5) != 0) {
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01077) "declining
> URL %s", url);
> return DECLINED;
> }
>
> That allocation of uri can be moved down (further than the code shown above)
> until right before it is used. I've seen this in a number of places and it
> "feels" like it is considered OK because the pool is relatively short lived
> and in most cases I've seen so far the allocation is pretty small. But as a
> concept, this strikes me as bad. If that code was using a traditional
> malloc/free the above would be a memory leak. I am aware that pools provide
> protection against such leaks, but nonetheless, that is wasted memory
> allocated and although quick, also a waste of time.
The idea behind pools is that you allocate an arbitrary and unpredictable set
of memory, and then free the whole set at some future point in time. This means
you don't need to keep track of each and every escape path out of a system and
hope you've cleaned up everything, you allocate at will confident that it will
all be freed.
Obviously allocating too early and then throwing away the results of the
allocation is a waste as you've pointed out, and should ideally be smoked out
and fixed.
> Am I being too obsessive? If not, would you like patches to correct these as
> I find them, and if so, should I open a bug about this or just post patches
> here (they are all likely to be a simple move of 1 or 2 lines)?
I'd love to see these things fixed, because they add up. If you post them here
they are likely to be reviewed very quickly, as they'll no doubt be simple to
review.
Regards,
Graham
--
