Re: [PATCH] Fix for memory leak in xlate code.

kfogel@collab.net writes:

> @@ -199,6 +211,14 @@
> if (userdata_key && xlate_handle_hash)
> pool = apr_hash_pool_get (xlate_handle_hash);
>
> + /* If we failed due to ENOTIMPL before, then we'd certainly fail
> + now, so just return without trying. */
> + if (enotimpl_handle_node)
> + {
> + *ret = enotimpl_handle_node;
> + return SVN_NO_ERROR;
> + }
> +
> /* Try to create a handle. */
> *ret = apr_palloc (pool, sizeof(xlate_handle_node_t));
> apr_err = apr_xlate_open (&(*ret)->handle, topage, frompage, pool);
> @@ -229,6 +249,19 @@
>
> if (APR_STATUS_IS_EINVAL (apr_err) || APR_STATUS_IS_ENOTIMPL (apr_err))
> {
> + if (APR_STATUS_IS_ENOTIMPL (apr_err))
> + {
> + /* Remember for next time that we needn't bother trying.
> + Use the top-level pool because we have no lifetime
> + guarantees about our local pool parameter. */
> + enotimpl_handle_node = apr_palloc (svn_pool_create (NULL),
> + sizeof (*enotimpl_handle_node));

Yuck! Use static memory and avoid the pool?

Of course all the usual "it's not thread-safe" caveats apply when
abusing a static variable like this :) There is one glaring problem
however: you are setting the enotimpl_handle_node pointer before
filling the struct so another thread could read the pointer and get
rubbish. Use static memory and that particular problem goes away, but
it's still implementation dependent whether it's really thread-safe.

> + enotimpl_handle_node->handle = NULL;
> + enotimpl_handle_node->frompage = NULL;
> + enotimpl_handle_node->topage = NULL;
> + enotimpl_handle_node->next = NULL;

It's a minor inconsistency that *ret is not enotimpl_handle_node the
first time we get ENOTIMPL, but is ever afterwards.

> + }
> +
> (*ret)->handle = NULL;
> return SVN_NO_ERROR;
> }

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org