[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [RFC] subversion upload for squeeze - what to include



And here, attached, are the patch files.

> 1. 'server-memleaks': Fix a handful of server-side memory leaks, in
>    which untrusted remote clients can DoS a server by making it use way
>    too much memory.  There's a patch to shuffle around some memory pool
>    usage - create, use, destroy.  And a smaller patch for svnserve (for
>    the svn:// network protocol), to tweak the behavior of the pool
>    allocator so it frees memory more aggressively.

> 2. 'dav-skip-unreadable-children': Patch to Apache mod_dav_svn ordinary
>    directory listing, to filter out paths the user doesn't have
>    permission to read.
> 
>    Could be construed as a security fix, to match the expectation that
>    when you set a file (or subdir) as not readable, the existence of
>    the file will also be hidden.  Very un-Unixy; Unix admins should
>    know better than to assume that.
> 
>    I believe this is _not_ needed in squeeze, but if debian-release
>    thinks it would be useful, I will include it.

> 3. 'no-wc1.7-check': Rip out some expensive code to detect, and abort,
>    if you're in a Subversion 1.7 working copy.  This was intended as
>    future proofing, but we've since reconsidered and decided that it
>    isn't really very important, and the client slowdown is far out of
>    proportion to any value it brings.
> 
>    This is clearly not RC, but it's also easy to review: it merely
>    deletes a function, its caller, and a few lines in the testsuite.
Memory leak fixes for Subversion's svnserve and mod_dav_svn servers,
from upstream 1.6.15.

--- a/subversion/libsvn_repos/rev_hunt.c
+++ b/subversion/libsvn_repos/rev_hunt.c
@@ -1080,7 +1080,8 @@ find_interesting_revisions(apr_array_hea
                            apr_hash_t *duplicate_path_revs,
                            svn_repos_authz_func_t authz_read_func,
                            void *authz_read_baton,
-                           apr_pool_t *pool)
+                           apr_pool_t *pool,
+                           apr_pool_t *scratch_pool)
 {
   apr_pool_t *iter_pool, *last_pool;
   svn_fs_history_t *history;
@@ -1089,23 +1090,24 @@ find_interesting_revisions(apr_array_hea
 
   /* We switch between two pools while looping, since we need information from
      the last iteration to be available. */
-  iter_pool = svn_pool_create(pool);
-  last_pool = svn_pool_create(pool);
+  iter_pool = svn_pool_create(scratch_pool);
+  last_pool = svn_pool_create(scratch_pool);
 
   /* The path had better be a file in this revision. */
-  SVN_ERR(svn_fs_revision_root(&root, repos->fs, end, last_pool));
-  SVN_ERR(svn_fs_check_path(&kind, root, path, pool));
+  SVN_ERR(svn_fs_revision_root(&root, repos->fs, end, scratch_pool));
+  SVN_ERR(svn_fs_check_path(&kind, root, path, scratch_pool));
   if (kind != svn_node_file)
     return svn_error_createf
       (SVN_ERR_FS_NOT_FILE, NULL, _("'%s' is not a file in revision %ld"),
        path, end);
 
   /* Open a history object. */
-  SVN_ERR(svn_fs_node_history(&history, root, path, last_pool));
-
+  SVN_ERR(svn_fs_node_history(&history, root, path, scratch_pool));
   while (1)
     {
-      struct path_revision *path_rev = apr_palloc(pool, sizeof(*path_rev));
+      struct path_revision *path_rev;
+      svn_revnum_t tmp_revnum;
+      const char *tmp_path;
       apr_pool_t *tmp_pool;
 
       svn_pool_clear(iter_pool);
@@ -1114,13 +1116,13 @@ find_interesting_revisions(apr_array_hea
       SVN_ERR(svn_fs_history_prev(&history, history, TRUE, iter_pool));
       if (!history)
         break;
-      SVN_ERR(svn_fs_history_location(&path_rev->path, &path_rev->revnum,
+      SVN_ERR(svn_fs_history_location(&tmp_path, &tmp_revnum,
                                       history, iter_pool));
 
       /* Check to see if we already saw this path (and it's ancestors) */
       if (include_merged_revisions
-          && is_path_in_hash(duplicate_path_revs, path_rev->path,
-                             path_rev->revnum, iter_pool))
+          && is_path_in_hash(duplicate_path_revs, tmp_path,
+                             tmp_revnum, iter_pool))
          break;
 
       /* Check authorization. */
@@ -1129,15 +1131,18 @@ find_interesting_revisions(apr_array_hea
           svn_boolean_t readable;
           svn_fs_root_t *tmp_root;
 
-          SVN_ERR(svn_fs_revision_root(&tmp_root, repos->fs, path_rev->revnum,
+          SVN_ERR(svn_fs_revision_root(&tmp_root, repos->fs, tmp_revnum,
                                        iter_pool));
-          SVN_ERR(authz_read_func(&readable, tmp_root, path_rev->path,
+          SVN_ERR(authz_read_func(&readable, tmp_root, tmp_path,
                                   authz_read_baton, iter_pool));
           if (! readable)
             break;
         }
 
-      path_rev->path = apr_pstrdup(pool, path_rev->path);
+      /* We didn't break, so we must really want this path-rev. */
+      path_rev = apr_palloc(pool, sizeof(*path_rev));
+      path_rev->path = apr_pstrdup(pool, tmp_path);
+      path_rev->revnum = tmp_revnum;
       path_rev->merged = mark_as_merged;
       APR_ARRAY_PUSH(path_revisions, struct path_revision *) = path_rev;
 
@@ -1165,6 +1170,7 @@ find_interesting_revisions(apr_array_hea
     }
 
   svn_pool_destroy(iter_pool);
+  svn_pool_destroy(last_pool);
 
   return SVN_NO_ERROR;
 }
@@ -1192,7 +1198,7 @@ find_merged_revisions(apr_array_header_t
                       apr_pool_t *pool)
 {
   apr_array_header_t *old, *new;
-  apr_pool_t *iter_pool, *last_pool;
+  apr_pool_t *iterpool, *last_pool;
   apr_array_header_t *merged_path_revisions = apr_array_make(pool, 0,
                                                 sizeof(struct path_revision *));
 
@@ -1212,20 +1218,27 @@ find_merged_revisions(apr_array_header_t
          path_revisions for any merged revisions, and store those in NEW. */
       for (i = 0; i < old->nelts; i++)
         {
+          apr_pool_t *iterpool2;
           apr_hash_index_t *hi;
           struct path_revision *old_pr = APR_ARRAY_IDX(old, i,
                                                        struct path_revision *);
           if (!old_pr->merged_mergeinfo)
             continue;
 
+          iterpool2 = svn_pool_create(iter_pool);
+
           /* Determine and trace the merge sources. */
           for (hi = apr_hash_first(iter_pool, old_pr->merged_mergeinfo); hi;
                hi = apr_hash_next(hi))
             {
+              apr_pool_t *iterpool3;
               apr_array_header_t *rangelist;
               const char *path;
               int j;
 
+              svn_pool_clear(iterpool2);
+              iterpool3 = svn_pool_create(iterpool2);
+
               apr_hash_this(hi, (void *) &path, NULL, (void *) &rangelist);
 
               for (j = 0; j < rangelist->nelts; j++)
@@ -1235,9 +1248,10 @@ find_merged_revisions(apr_array_header_t
                   svn_node_kind_t kind;
                   svn_fs_root_t *root;
 
+                  svn_pool_clear(iterpool3);
                   SVN_ERR(svn_fs_revision_root(&root, repos->fs, range->end,
-                                               iter_pool));
-                  SVN_ERR(svn_fs_check_path(&kind, root, path, iter_pool));
+                                               iterpool3));
+                  SVN_ERR(svn_fs_check_path(&kind, root, path, iterpool3));
                   if (kind != svn_node_file)
                     continue;
 
@@ -1247,9 +1261,12 @@ find_merged_revisions(apr_array_header_t
                                                      TRUE, TRUE,
                                                      duplicate_path_revs,
                                                      authz_read_func,
-                                                     authz_read_baton, pool));
+                                                     authz_read_baton, pool,
+                                                     iterpool3));
                 }
+              svn_pool_destroy(iterpool3);
             }
+          svn_pool_destroy(iterpool2);
         }
 
       /* Append the newly found path revisions with the old ones. */
@@ -1407,7 +1424,8 @@ svn_repos_get_file_revs2(svn_repos_t *re
   SVN_ERR(find_interesting_revisions(mainline_path_revisions, repos, path,
                                      start, end, include_merged_revisions,
                                      FALSE, duplicate_path_revs,
-                                     authz_read_func, authz_read_baton, pool));
+                                     authz_read_func, authz_read_baton, pool,
+                                     pool));
 
   /* If we are including merged revisions, go get those, too. */
   if (include_merged_revisions)
--- a/subversion/svnserve/main.c
+++ b/subversion/svnserve/main.c
@@ -356,6 +356,7 @@ int main(int argc, const char *argv[])
   apr_sockaddr_t *sa;
   apr_pool_t *pool;
   apr_pool_t *connection_pool;
+  apr_allocator_t *allocator;
   svn_error_t *err;
   apr_getopt_t *os;
   int opt;
@@ -747,10 +748,22 @@ int main(int argc, const char *argv[])
         return ERROR_SUCCESS;
 #endif
 
+      /* If we are using fulltext caches etc., we will allocate many large
+         chunks of memory of various sizes outside the cachde for those
+         fulltexts. Make sure, we use the memory wisely: use an allocator
+         that causes memory fragments to be given back to the OS early. */
+
+      if (apr_allocator_create(&allocator))
+        return EXIT_FAILURE;
+
+      apr_allocator_max_free_set(allocator, SVN_ALLOCATOR_RECOMMENDED_MAX_FREE);
+
       /* Non-standard pool handling.  The main thread never blocks to join
          the connection threads so it cannot clean up after each one.  So
          separate pools, that can be cleared at thread exit, are used */
-      connection_pool = svn_pool_create(NULL);
+
+      connection_pool = svn_pool_create_ex(NULL, allocator);
+      apr_allocator_owner_set(allocator, connection_pool);
 
       status = apr_socket_accept(&usock, sock, connection_pool);
       if (handling_mode == connection_mode_fork)
Make mod_dav_svn hide unreadable directory children in HTTP GET output.
This is a security measure in the sense that some admins may expect
that setting something as unreadable will also hide its very existence.
Unix admins really should know better than to expect this, but....

>From the upstream 1.6.15 release.

--- a/subversion/mod_dav_svn/liveprops.c
+++ b/subversion/mod_dav_svn/liveprops.c
@@ -139,7 +139,7 @@ get_path_revprop(svn_string_t **propval,
 {
   *propval = NULL;
 
-  if (! dav_svn__allow_read(resource, committed_rev, pool))
+  if (! dav_svn__allow_read_resource(resource, committed_rev, pool))
     return SVN_NO_ERROR;
 
   /* Get the property of the created revision. The authz is already
--- a/subversion/mod_dav_svn/dav_svn.h
+++ b/subversion/mod_dav_svn/dav_svn.h
@@ -587,17 +587,32 @@ typedef struct
 } dav_svn__authz_read_baton;
 
 
-/* Convert incoming RESOURCE and revision REV into a version-resource URI and
-   perform a GET subrequest on it.  This will invoke any authz modules loaded
-   into apache. Return TRUE if the subrequest succeeds, FALSE otherwise.
+/* Return TRUE iff the current user (as determined by Apache's
+   authentication system) has permission to read PATH in REPOS at REV
+   (where an invalid REV means "HEAD").  This will invoke any authz
+   modules loaded into Apache unless this Subversion location has been
+   configured to bypass those in favor of a direct lookup in the
+   Subversion authz subsystem.  Use POOL for any temporary allocation.
+*/
+svn_boolean_t
+dav_svn__allow_read(request_rec *r,
+                    const dav_svn_repos *repos,
+                    const char *path,
+                    svn_revnum_t rev,
+                    apr_pool_t *pool);
 
-   If REV is SVN_INVALID_REVNUM, then we look at HEAD.
-   Use POOL for any temporary allocation.
+/* Return TRUE iff the current user (as determined by Apache's
+   authentication system) has permission to read RESOURCE in REV
+   (where an invalid REV means "HEAD").  This will invoke any authz
+   modules loaded into Apache unless this Subversion location has been
+   configured to bypass those in favor of a direct lookup in the
+   Subversion authz subsystem.  Use POOL for any temporary allocation.
 */
 svn_boolean_t
-dav_svn__allow_read(const dav_resource *resource,
-                   svn_revnum_t rev,
-                   apr_pool_t *pool);
+dav_svn__allow_read_resource(const dav_resource *resource,
+                             svn_revnum_t rev,
+                             apr_pool_t *pool);
+
 
 /* If authz is enabled in the specified BATON, return a read authorization
    function. Otherwise, return NULL. */
--- a/subversion/mod_dav_svn/lock.c
+++ b/subversion/mod_dav_svn/lock.c
@@ -453,7 +453,8 @@ get_locks(dav_lockdb *lockdb,
 
   /* If the resource's fs path is unreadable, we don't want to say
      anything about locks attached to it.*/
-  if (! dav_svn__allow_read(resource, SVN_INVALID_REVNUM, resource->pool))
+  if (! dav_svn__allow_read_resource(resource, SVN_INVALID_REVNUM,
+                                     resource->pool))
     return dav_new_error(resource->pool, HTTP_FORBIDDEN,
                          DAV_ERR_LOCK_SAVE_LOCK,
                          "Path is not accessible.");
@@ -513,7 +514,8 @@ find_lock(dav_lockdb *lockdb,
 
   /* If the resource's fs path is unreadable, we don't want to say
      anything about locks attached to it.*/
-  if (! dav_svn__allow_read(resource, SVN_INVALID_REVNUM, resource->pool))
+  if (! dav_svn__allow_read_resource(resource, SVN_INVALID_REVNUM,
+                                     resource->pool))
     return dav_new_error(resource->pool, HTTP_FORBIDDEN,
                          DAV_ERR_LOCK_SAVE_LOCK,
                          "Path is not accessible.");
@@ -591,7 +593,8 @@ has_locks(dav_lockdb *lockdb, const dav_
 
   /* If the resource's fs path is unreadable, we don't want to say
      anything about locks attached to it.*/
-  if (! dav_svn__allow_read(resource, SVN_INVALID_REVNUM, resource->pool))
+  if (! dav_svn__allow_read_resource(resource, SVN_INVALID_REVNUM,
+                                     resource->pool))
     return dav_new_error(resource->pool, HTTP_FORBIDDEN,
                          DAV_ERR_LOCK_SAVE_LOCK,
                          "Path is not accessible.");
@@ -634,7 +637,8 @@ append_locks(dav_lockdb *lockdb,
 
   /* If the resource's fs path is unreadable, we don't allow a lock to
      be created on it. */
-  if (! dav_svn__allow_read(resource, SVN_INVALID_REVNUM, resource->pool))
+  if (! dav_svn__allow_read_resource(resource, SVN_INVALID_REVNUM,
+                                     resource->pool))
     return dav_new_error(resource->pool, HTTP_FORBIDDEN,
                          DAV_ERR_LOCK_SAVE_LOCK,
                          "Path is not accessible.");
@@ -801,7 +805,8 @@ remove_lock(dav_lockdb *lockdb,
 
   /* If the resource's fs path is unreadable, we don't allow a lock to
      be removed from it. */
-  if (! dav_svn__allow_read(resource, SVN_INVALID_REVNUM, resource->pool))
+  if (! dav_svn__allow_read_resource(resource, SVN_INVALID_REVNUM,
+                                     resource->pool))
     return dav_new_error(resource->pool, HTTP_FORBIDDEN,
                          DAV_ERR_LOCK_SAVE_LOCK,
                          "Path is not accessible.");
@@ -886,7 +891,8 @@ refresh_locks(dav_lockdb *lockdb,
 
   /* If the resource's fs path is unreadable, we don't want to say
      anything about locks attached to it.*/
-  if (! dav_svn__allow_read(resource, SVN_INVALID_REVNUM, resource->pool))
+  if (! dav_svn__allow_read_resource(resource, SVN_INVALID_REVNUM,
+                                     resource->pool))
     return dav_new_error(resource->pool, HTTP_FORBIDDEN,
                          DAV_ERR_LOCK_SAVE_LOCK,
                          "Path is not accessible.");
--- a/subversion/mod_dav_svn/repos.c
+++ b/subversion/mod_dav_svn/repos.c
@@ -2840,6 +2840,7 @@ deliver(const dav_resource *resource, ap
       apr_hash_t *entries;
       apr_pool_t *entry_pool;
       apr_array_header_t *sorted;
+      svn_revnum_t dir_rev = SVN_INVALID_REVNUM;
       int i;
 
       /* XML schema for the directory index if xslt_uri is set:
@@ -2916,6 +2917,7 @@ deliver(const dav_resource *resource, ap
         }
       else
         {
+          dir_rev = svn_fs_revision_root_revision(resource->info->root.root);
           serr = svn_fs_dir_entries(&entries, resource->info->root.root,
                                     resource->info->repos_path, resource->pool);
           if (serr != NULL)
@@ -3022,9 +3024,31 @@ deliver(const dav_resource *resource, ap
           const char *name = item->key;
           const char *href = name;
           svn_boolean_t is_dir = (entry->kind == svn_node_dir);
+          const char *repos_relpath = NULL;
 
           svn_pool_clear(entry_pool);
 
+          /* DIR_REV is set to a valid revision if we're looking at
+             the entries of a versioned directory.  Otherwise, we're
+             looking at a parent-path listing. */
+          if (SVN_IS_VALID_REVNUM(dir_rev))
+            {
+              repos_relpath = svn_path_join(resource->info->repos_path,
+                                            name, entry_pool);
+              if (! dav_svn__allow_read(resource->info->r,
+                                        resource->info->repos,
+                                        repos_relpath,
+                                        dir_rev,
+                                        entry_pool))
+                continue;
+            }
+          else
+            {
+              /* ### TODO:  We could test for readability of the root
+                     directory of each repository and hide those that
+                     the user can't see. */
+            }
+
           /* append a trailing slash onto the name for directories. we NEED
              this for the href portion so that the relative reference will
              descend properly. for the visible portion, it is just nice. */
--- a/subversion/mod_dav_svn/authz.c
+++ b/subversion/mod_dav_svn/authz.c
@@ -26,17 +26,12 @@
 #include "dav_svn.h"
 
 
-/* Convert incoming REV and PATH from request R into a version-resource URI
-   for REPOS and perform a GET subrequest on it.  This will invoke any authz
-   modules loaded into apache.  Return TRUE if the subrequest succeeds, FALSE
-   otherwise. If REV is SVN_INVALID_REVNUM, then we look at HEAD.
-*/
-static svn_boolean_t
-allow_read(request_rec *r,
-           const dav_svn_repos *repos,
-           const char *path,
-           svn_revnum_t rev,
-           apr_pool_t *pool)
+svn_boolean_t
+dav_svn__allow_read(request_rec *r,
+                    const dav_svn_repos *repos,
+                    const char *path,
+                    svn_revnum_t rev,
+                    apr_pool_t *pool)
 {
   const char *uri;
   request_rec *subreq;
@@ -170,7 +165,7 @@ authz_read(svn_boolean_t *allowed,
     }
 
   /* We have a (rev, path) pair to check authorization on. */
-  *allowed = allow_read(arb->r, arb->repos, revpath, rev, pool);
+  *allowed = dav_svn__allow_read(arb->r, arb->repos, revpath, rev, pool);
 
   return SVN_NO_ERROR;
 }
@@ -189,10 +184,10 @@ dav_svn__authz_read_func(dav_svn__authz_
 
 
 svn_boolean_t
-dav_svn__allow_read(const dav_resource *resource,
-                   svn_revnum_t rev,
-                   apr_pool_t *pool)
+dav_svn__allow_read_resource(const dav_resource *resource,
+                             svn_revnum_t rev,
+                             apr_pool_t *pool)
 {
-  return allow_read(resource->info->r, resource->info->repos,
-                    resource->info->repos_path, rev, pool);
+  return dav_svn__allow_read(resource->info->r, resource->info->repos,
+                             resource->info->repos_path, rev, pool);
 }
Remove the check for being inside a svn 1.7 working copy.
This check was added in 1.6.6 and removed again in 1.6.15.

It was intended as future-proofing, but we've since concluded that it's
not actually that useful.  Especially given how expensive it is: it is
called potentially a great many times for a single svn operation, and
each time, calls stat() and recurses up the filesystem to "/".


--- a/subversion/tests/cmdline/svntest/actions.py
+++ b/subversion/tests/cmdline/svntest/actions.py
@@ -29,12 +29,6 @@ def no_sleep_for_timestamps():
 def do_sleep_for_timestamps():
   os.environ['SVN_I_LOVE_CORRUPTED_WORKING_COPIES_SO_DISABLE_SLEEP_FOR_TIMESTAMPS'] = 'no'
 
-def no_check_for_wc_ng():
-  os.environ['SVN_I_LOVE_CORRUPTED_WORKING_COPIES_SO_DISABLE_CHECK_FOR_WC_NG'] = 'yes'
-
-def do_check_for_wc_ng():
-  os.environ['SVN_I_LOVE_CORRUPTED_WORKING_COPIES_SO_DISABLE_CHECK_FOR_WC_NG'] = 'no'
-
 def setup_pristine_repository():
   """Create the pristine repository and 'svn import' the greek tree"""
 
--- a/subversion/tests/cmdline/svntest/main.py
+++ b/subversion/tests/cmdline/svntest/main.py
@@ -1218,7 +1218,6 @@ class TestRunner:
                                       str(self.index)
 
     actions.no_sleep_for_timestamps()
-    actions.no_check_for_wc_ng()
 
     saved_dir = os.getcwd()
     try:
--- a/subversion/libsvn_wc/questions.c
+++ b/subversion/libsvn_wc/questions.c
@@ -43,56 +43,6 @@
 #include "private/svn_wc_private.h"
 #include "private/svn_sqlite.h"
 
-#define SVN_WC_NG_CHECK_ENV_VAR "SVN_I_LOVE_CORRUPTED_WORKING_COPIES_SO_DISABLE_CHECK_FOR_WC_NG"
-
-static svn_error_t *
-is_inside_wc_ng(const char *abspath,
-                const char *target_path,
-                int *wc_format,
-                apr_pool_t *pool)
-{
-  svn_node_kind_t kind;
-  const char *wc_db_path;
-  char *wc_ng_check_env_var;
-  svn_error_t *err;
-
-  wc_ng_check_env_var = getenv(SVN_WC_NG_CHECK_ENV_VAR);
-  if (wc_ng_check_env_var &&
-      apr_strnatcasecmp(wc_ng_check_env_var, "yes") == 0)
-    return SVN_NO_ERROR; /* Allow skipping for testing */
-
-  wc_db_path = svn_path_join_many(pool, abspath, SVN_WC_ADM_DIR_NAME,
-                                  "wc.db", NULL);
-  err = svn_io_check_path(wc_db_path, &kind, pool);
-  if (err)
-    {
-      svn_error_clear(err);
-      return SVN_NO_ERROR;
-    }
-
-  if (kind == svn_node_file)
-    {
-      /* This value is completely bogus, but it is much higher than 1.6 will
-         have any prayer of reading. */
-      *wc_format = 9999;
-
-      return svn_error_createf(SVN_ERR_WC_UNSUPPORTED_FORMAT, NULL,
-         _("The path '%s' appears to be part of a Subversion 1.7 or greater\n"
-           "working copy rooted at '%s'.\n"
-           "Please upgrade your Subversion client to use this working copy."
-           ),
-         svn_path_local_style(target_path, pool),
-         svn_path_local_style(abspath, pool));
-    }
-
-  if (svn_dirent_is_root(abspath, strlen(abspath)))
-    return SVN_NO_ERROR;
-  else
-    return is_inside_wc_ng(svn_path_dirname(abspath, pool), target_path,
-                           wc_format, pool);
-}
-
-
 /* ### todo: make this compare repository too?  Or do so in parallel
    code.  */
 svn_error_t *
@@ -146,15 +96,6 @@ svn_wc_check_wc(const char *path,
   else if (err)
     return err;
 
-  /* Let's check for the future. */
-  if (*wc_format == 0)
-    {
-      const char *abspath;
-
-      SVN_ERR(svn_path_get_absolute(&abspath, path, pool));
-      SVN_ERR(is_inside_wc_ng(abspath, path, wc_format, pool));
-    }
-
   if (*wc_format > 0)
     {
       /* If we managed to read the format file we assume that we

Reply to: