Fix memory leak in character translation caching.

If apr_xlate_open returned an error, some memory was allocated from a global
pool on every call.  The problem is solved by caching a NULL handle in
this case.

Suggested by: jerenkrantz, kfogel

* subversion/libsvn_subr/utf.c
  (xlate_handle_node_t): Add valid field.
  (xlate_handle_node_cleanup): Set valid to FALSE instead of resetting the
  handle.
  (get_xlate_handle_node): Fix docstring.
  Initialize valid field of allocated node struct.
  Check the valid field instead of the handle filed to make sure
  the handle is valid.  Avoid allocating a node if we're going to return an
  error.  Rename some variables for clarity.
  (svn_utf_stringbuf_to_utf8, svn_utf_string_to_utf8,
  svn_utf_stringbuf_from_utf8, svn_utf_string_from_utf8, 
  svn_utf_cstring_from_utf8, svn_utf_cstring_from_utf8_ex,
  svn_utf_cstring_from_utf8_string): Make sure the xlate handle node
  is always put back into the hash, even on errors and if there
  is no handle.


Index: subversion/libsvn_subr/utf.c
===================================================================
--- subversion/libsvn_subr/utf.c	(revision 15373)
+++ subversion/libsvn_subr/utf.c	(arbetskopia)
@@ -53,6 +53,9 @@
 
 typedef struct xlate_handle_node_t {
   apr_xlate_t *handle;
+  /* FALSE if the handle is not valid, since its pool is being
+     destroyed. */
+  svn_boolean_t valid;
   /* The name of a char encoding or APR_LOCALE_CHARSET. */
   const char *frompage, *topage;
   struct xlate_handle_node_t *next;
@@ -89,7 +92,7 @@
 {
   xlate_handle_node_t *node = arg;
 
-  node->handle = NULL;
+  node->valid = FALSE;
   return APR_SUCCESS;
 }
 
@@ -122,20 +125,25 @@
     }
 }
 
-/* Return an apr_xlate handle for converting from FROMPAGE to
-   TOPAGE. Create one if it doesn't exist in USERDATA_KEY. If
-   unable to find a handle, or unable to create one because
-   apr_xlate_open returned APR_EINVAL, then set *RET to null and
-   return SVN_NO_ERROR; if fail for some other reason, return
-   error. */
+/* Set *RET to a handle node for converting from FROMPAGE to TOPAGE,
+   creating the handle node if it doesn't exist in USERDATA_KEY.
+   If a node is not cached and apr_xlate_open() returns APR_EINVAL or
+   APR_ENOTIMPL, set (*ret)->handle to NULL.  If fail for any other
+   reason, return the error.
+
+   Allocate *ret and the xlate handle in POOL if svn_utf_initialize()
+   hasn't been called or USERDATA_KEY is NULL.  Else, allocate them
+   in the pool of xlate_handle_hash. */
 static svn_error_t *
 get_xlate_handle_node (xlate_handle_node_t **ret,
                        const char *topage, const char *frompage,
                        const char *userdata_key, apr_pool_t *pool)
 {
-  xlate_handle_node_t **old_handle_p;
-  xlate_handle_node_t *old_handle = NULL;
+  xlate_handle_node_t **old_node_p;
+  xlate_handle_node_t *old_node = NULL;
   apr_status_t apr_err;
+  apr_xlate_t *handle;
+  svn_error_t *err = NULL;
 
   /* If we already have a handle, just return it. */
   if (userdata_key)
@@ -148,18 +156,18 @@
             return svn_error_create (apr_err, NULL,
                                      _("Can't lock charset translation mutex"));
 #endif
-          old_handle_p = apr_hash_get (xlate_handle_hash, userdata_key,
-                                       APR_HASH_KEY_STRING);
-          if (old_handle_p)
-            old_handle = *old_handle_p;
-          if (old_handle)
+          old_node_p = apr_hash_get (xlate_handle_hash, userdata_key,
+                                     APR_HASH_KEY_STRING);
+          if (old_node_p)
+            old_node = *old_node_p;
+          if (old_node)
             {
               /* Ensure that the handle is still valid. */
-              if (old_handle->handle)
+              if (old_node->valid)
                 {
                   /* Remove from the list. */
-                  *old_handle_p = old_handle->next;
-                  old_handle->next = NULL;
+                  *old_node_p = old_node->next;
+                  old_node->next = NULL;
 #if APR_HAS_THREADS
                   apr_err = apr_thread_mutex_unlock (xlate_handle_mutex);
                   if (apr_err != APR_SUCCESS)
@@ -167,7 +175,7 @@
                                              _("Can't unlock charset "
                                                "translation mutex"));
 #endif
-                  *ret = old_handle;
+                  *ret = old_node;
                   return SVN_NO_ERROR;
                 }
             }
@@ -177,11 +185,11 @@
           void *p;
           /* We fall back on a per-pool cache instead. */
           apr_pool_userdata_get (&p, userdata_key, pool);
-          old_handle = p;
+          old_node = p;
           /* Ensure that the handle is still valid. */
-          if (old_handle && old_handle->handle)
+          if (old_node && old_node->valid)
             {
-              *ret = old_handle;
+              *ret = old_node;
               return SVN_NO_ERROR;
             }
         }
@@ -195,13 +203,40 @@
   assert (frompage != APR_DEFAULT_CHARSET && topage != APR_DEFAULT_CHARSET
           && (frompage != APR_LOCALE_CHARSET || topage != APR_LOCALE_CHARSET));
 
+  /* Try to create a handle. */
+  apr_err = apr_xlate_open (&handle, topage, frompage, pool);
+
+  if (APR_STATUS_IS_EINVAL (apr_err) || APR_STATUS_IS_ENOTIMPL (apr_err))
+    handle = NULL;
+  else if (apr_err != APR_SUCCESS)
+    {
+      const char *errstr;
+      /* Can't use svn_error_wrap_apr here because it calls functions in
+         this file, leading to infinite recursion. */
+      if (frompage == APR_LOCALE_CHARSET)
+        errstr = apr_psprintf (pool,
+                               _("Can't create a character converter from "
+                                 "native encoding to '%s'"), topage);
+      else if (topage == APR_LOCALE_CHARSET)
+        errstr = apr_psprintf (pool,
+                               _("Can't create a character converter from "
+                                 "'%s' to native encoding"), frompage);
+      else
+        errstr = apr_psprintf (pool,
+                               _("Can't create a character converter from "
+                                 "'%s' to '%s'"), frompage, topage);
+      err = svn_error_create (apr_err, NULL, errstr);
+      goto cleanup;
+    }
+
   /* Use the correct pool for creating the handle. */
   if (userdata_key && xlate_handle_hash)
     pool = apr_hash_pool_get (xlate_handle_hash);
 
-  /* Try to create a handle. */
+  /* Allocate and initialize the node. */
   *ret = apr_palloc (pool, sizeof(xlate_handle_node_t));
-  apr_err = apr_xlate_open (&(*ret)->handle, topage, frompage, pool);
+  (*ret)->handle = handle;
+  (*ret)->valid = TRUE;
   (*ret)->frompage = ((frompage != APR_LOCALE_CHARSET)
                       ? apr_pstrdup (pool, frompage) : frompage);
   (*ret)->topage = ((topage != APR_LOCALE_CHARSET)
@@ -211,11 +246,13 @@
   /* If we are called from inside a pool cleanup handler, the just created
      xlate handle will be closed when that handler returns by a newly
      registered cleanup handler, however, the handle is still cached by us.
-     To prevent this, we register a cleanup handler that will reset our
-     handle, so we don't use an invalid one. */
-  apr_pool_cleanup_register (pool, *ret, xlate_handle_node_cleanup,
-                             apr_pool_cleanup_null);
+     To prevent this, we register a cleanup handler that will reset the valid
+     flag of our node, so we don't use an invalid handle. */
+  if (handle)
+    apr_pool_cleanup_register (pool, *ret, xlate_handle_node_cleanup,
+                               apr_pool_cleanup_null);
 
+ cleanup:
   /* Don't need the lock anymore. */
 #if APR_HAS_THREADS
   if (userdata_key && xlate_handle_hash)
@@ -227,31 +264,7 @@
     }
 #endif
 
-  if (APR_STATUS_IS_EINVAL (apr_err) || APR_STATUS_IS_ENOTIMPL (apr_err))
-    {
-      (*ret)->handle = NULL;
-      return SVN_NO_ERROR;
-    }
-  if (apr_err != APR_SUCCESS)
-    {
-      const char *errstr;
-      /* Can't use svn_error_wrap_apr here because it calls functions in
-         this file, leading to infinite recursion. */
-      if (frompage == APR_LOCALE_CHARSET)
-        errstr = apr_psprintf (pool,
-                               _("Can't create a character converter from "
-                                 "native encoding to '%s'"), topage);
-      else if (topage == APR_LOCALE_CHARSET)
-        errstr = apr_psprintf (pool,
-                               _("Can't create a character converter from "
-                                 "'%s' to native encoding"), frompage);
-      else
-        errstr = apr_psprintf (pool,
-                               _("Can't create a character converter from "
-                                 "'%s' to '%s'"), frompage, topage);
-      return svn_error_create (apr_err, NULL, errstr);
-    }
-  return SVN_NO_ERROR;
+  return err;
 }
 
 /* Put back NODE into the xlate handle cache for use by other calls.
@@ -578,16 +591,19 @@
   if (node->handle)
     {
       err = convert_to_stringbuf (node, src->data, src->len, dest, pool);
-      put_xlate_handle_node (node, SVN_UTF_NTOU_XLATE_HANDLE, pool);
-      SVN_ERR (err);
-      return check_utf8 ((*dest)->data, (*dest)->len, pool);
+      if (! err)
+        err = check_utf8 ((*dest)->data, (*dest)->len, pool);
     }
   else
     {
-      SVN_ERR (check_non_ascii (src->data, src->len, pool));
-      *dest = svn_stringbuf_dup (src, pool);
-      return SVN_NO_ERROR;
+      err = check_non_ascii (src->data, src->len, pool);
+      if (! err)
+        *dest = svn_stringbuf_dup (src, pool);
     }
+
+  put_xlate_handle_node (node, SVN_UTF_NTOU_XLATE_HANDLE, pool);
+
+  return err;
 }
 
 
@@ -605,18 +621,21 @@
   if (node->handle)
     {
       err = convert_to_stringbuf (node, src->data, src->len, &destbuf, pool);
-      put_xlate_handle_node (node, SVN_UTF_NTOU_XLATE_HANDLE, pool);
-      SVN_ERR (err);
-      SVN_ERR (check_utf8 (destbuf->data, destbuf->len, pool));
-      *dest = svn_string_create_from_buf (destbuf, pool);
+      if (! err)
+        err = check_utf8 (destbuf->data, destbuf->len, pool);
+      if (! err)
+        *dest = svn_string_create_from_buf (destbuf, pool);
     }
   else
     {
-      SVN_ERR (check_non_ascii (src->data, src->len, pool));
-      *dest = svn_string_dup (src, pool);
+      err = check_non_ascii (src->data, src->len, pool);
+      if (! err)
+        *dest = svn_string_dup (src, pool);
     }
 
-  return SVN_NO_ERROR;
+  put_xlate_handle_node (node, SVN_UTF_NTOU_XLATE_HANDLE, pool);
+
+  return err;
 }
 
 
@@ -697,17 +716,20 @@
 
   if (node->handle)
     {
-      SVN_ERR (check_utf8 (src->data, src->len, pool));
-      err = convert_to_stringbuf (node, src->data, src->len, dest, pool);
-      put_xlate_handle_node (node, SVN_UTF_UTON_XLATE_HANDLE, pool);
-      return err;
+      err = check_utf8 (src->data, src->len, pool);
+      if (! err)
+        err = convert_to_stringbuf (node, src->data, src->len, dest, pool);
     }
   else
     {
-      SVN_ERR (check_non_ascii (src->data, src->len, pool));
-      *dest = svn_stringbuf_dup (src, pool);
-      return SVN_NO_ERROR;
+      err = check_non_ascii (src->data, src->len, pool);
+      if (! err)
+        *dest = svn_stringbuf_dup (src, pool);
     }
+
+  put_xlate_handle_node (node, SVN_UTF_UTON_XLATE_HANDLE, pool);
+
+  return err;
 }
 
 
@@ -724,20 +746,23 @@
 
   if (node->handle)
     {
-      SVN_ERR (check_utf8 (src->data, src->len, pool));
-      err = convert_to_stringbuf (node, src->data, src->len,
-                                  &dbuf, pool);
-      put_xlate_handle_node (node, SVN_UTF_UTON_XLATE_HANDLE, pool);
-      SVN_ERR (err);
-      *dest = svn_string_create_from_buf (dbuf, pool);
+      err = check_utf8 (src->data, src->len, pool);
+      if (! err)
+        err = convert_to_stringbuf (node, src->data, src->len,
+                                    &dbuf, pool);
+      if (! err)
+        *dest = svn_string_create_from_buf (dbuf, pool);
     }
   else
     {
-      SVN_ERR (check_non_ascii (src->data, src->len, pool));
-      *dest = svn_string_dup (src, pool);
+      err = check_non_ascii (src->data, src->len, pool);
+      if (! err)
+        *dest = svn_string_dup (src, pool);
     }
 
-  return SVN_NO_ERROR;
+  put_xlate_handle_node (node, SVN_UTF_UTON_XLATE_HANDLE, pool);
+
+  return err;
 }
 
 
@@ -749,13 +774,13 @@
   xlate_handle_node_t *node;
   svn_error_t *err;
 
+  SVN_ERR (check_utf8 (src, strlen (src), pool));
+
   SVN_ERR (get_uton_xlate_handle_node (&node, pool));
-  SVN_ERR (check_utf8 (src, strlen (src), pool));
   err = convert_cstring (dest, src, node, pool);
   put_xlate_handle_node (node, SVN_UTF_UTON_XLATE_HANDLE, pool);
-  SVN_ERR (err);
 
-  return SVN_NO_ERROR;
+  return err;
 }
 
 
@@ -769,8 +794,9 @@
   xlate_handle_node_t *node;
   svn_error_t *err;
 
+  SVN_ERR (check_utf8 (src, strlen (src), pool));
+
   SVN_ERR (get_xlate_handle_node (&node, topage, "UTF-8", convset_key, pool));
-  SVN_ERR (check_utf8 (src, strlen (src), pool));
   err = convert_cstring (dest, src, node, pool);
   put_xlate_handle_node (node, convset_key, pool);
 
@@ -844,18 +870,21 @@
 
   if (node->handle)
     {
-      SVN_ERR (check_utf8 (src->data, src->len, pool));
-      err = convert_to_stringbuf (node, src->data, src->len,
-                                  &dbuf, pool);
-      put_xlate_handle_node (node, SVN_UTF_UTON_XLATE_HANDLE, pool);
-      SVN_ERR (err);
-      *dest = dbuf->data;
-      return SVN_NO_ERROR;
+      err = check_utf8 (src->data, src->len, pool);
+      if (! err)
+        err = convert_to_stringbuf (node, src->data, src->len,
+                                    &dbuf, pool);
+      if (! err)
+        *dest = dbuf->data;
     }
   else
     {
-      SVN_ERR (check_non_ascii (src->data, src->len, pool));
-      *dest = apr_pstrmemdup (pool, src->data, src->len);
-      return SVN_NO_ERROR;
+      err = check_non_ascii (src->data, src->len, pool);
+      if (! err)
+        *dest = apr_pstrmemdup (pool, src->data, src->len);
     }
+
+  put_xlate_handle_node (node, SVN_UTF_UTON_XLATE_HANDLE, pool);
+
+  return err;
 }