svn-3709

Version:

1.6.x

Bug Link:

http://subversion.tigris.org/issues/show_bug.cgi?id=3709

http://svn.haxx.se/users/archive-2010-09/0257.shtml

Patch Link:

server side patch:

http://svn.apache.org/viewvc?view=revision&revision=997026

http://svn.apache.org/viewvc?view=revision&revision=997474

client side patch:

http://svn.apache.org/viewvc?view=revision&revision=997457

http://svn.apache.org/viewvc?view=revision&revision=997466

http://svn.apache.org/viewvc?view=revision&revision=997471

Symptom:

Inconsistent output between "svn list" and "svn checkout".

When there’s an authorization restriction in the repository, the result of “svn list” and “svn checkout” look different. “svn checkout” skips restricted path without stop, but “svn list” stops printing filenames when the path is restricted.

They should proceed when encountered the error, but because their design pattern -- stop by default using SVN_ERR, they actually failed.

How it is diagnosed:

reproduced

 

How to reproduce:

Demonstration scenario 1:

 

1. No access authorization in the directory, ‘t3’. Other directories can be accessed by everyone.

2. Runs ‘svn checkout’

-Command: svn co svn://127.0.0.1/beta

-Output:

A        beta\t1

A        beta\t1\t1_text.txt

A        beta\t1\t1_text2.txt

A        beta\t2

A        beta\t2\t2_text.txt

A        beta\t4

A        beta\t4\t4_content.txt

Checked out revision 10.

                -Comment: skips the directory ‘t3’, but other files are printed.

3. Runs ‘svn list’

-Command: svn ls --verbose --recursive svn://127.0.0.1/beta

-Output:

         10  ?                        Jan 02 23:50 ./

          8  ?                        Dec 31 07:02 t1/

          8  ?                     30 Dec 31 07:02 t1/t1_text.txt

          3  ?                     22 Dec 30 01:44 t1/t1_text2.txt

          2  ?                        Dec 30 01:38 t2/

          2  ?                     15 Dec 30 01:38 t2/t2_text.txt

          9  ?                        Jan 02 11:53 t3/

svn: Authorization failed

-Comment: stops printing with error message ‘svn: Authorization failed’ when t3 is not accessible.

 

 

                -Server side log message:

3568 2011-01-03T08:05:40.844440Z 127.0.0.1 - beta open 2 cap=(edit-pipeline svndiff1 absent-entries depth mergeinfo log-revprops) / SVN/1.6.13%20(r1002816) -

3568 2011-01-03T08:05:40.844440Z 127.0.0.1 - beta get-latest-rev

3568 2011-01-03T08:05:40.845440Z 127.0.0.1 - beta reparent /

3568 2011-01-03T08:05:40.845440Z 127.0.0.1 - beta stat /@10

3568 2011-01-03T08:05:40.845440Z 127.0.0.1 - beta get-locks /

3568 2011-01-03T08:05:40.850441Z 127.0.0.1 - beta get-dir / r10 text

3568 2011-01-03T08:05:40.853441Z 127.0.0.1 - beta get-dir /t1 r10 text

3568 2011-01-03T08:05:40.860441Z 127.0.0.1 - beta get-dir /t2 r10 text

3568 2011-01-03T08:05:40.866442Z 127.0.0.1 mihn beta ERR E:\1.6.9\subversion\tmp\subversion\subversion\svnserve\serve.c 167 170001 Authorization failed

 

*Result: ‘svn checkout’ and ‘svn list’ have inconsistency when printing out the contents, and the fault exists near ‘serve.c@167’.

 

Demonstration scenario 2:

 

Run ‘svn ls -R http://svncorp.org/svncorp/svn/trunk’ in subversion 1.6.6 with debug mode

 

Result:

[DEBUG] subversion/libsvn_ra_neon/util.c:563: (apr_err=175002)

[ERROR] svn: Server sent unexpected return value (403 Forbidden) in response to PROPFIND request for '/svncorp/svn/trunk/private'

 

Source code analysis:

 

subversion/libsvn_ra_neon/util.c:563

        case NE_OK:

          switch (req->code)

            {

            case 404:

              return svn_error_create(SVN_ERR_FS_NOT_FOUND, NULL,

                                  apr_psprintf(pool, _("'%s' path not found"),

                                               req->url));

 

            case 301:

            case 302:

              return svn_error_create

            (SVN_ERR_RA_DAV_RELOCATED, NULL,

                 apr_psprintf(pool,

                          (req->code == 301)

                              ? _("Repository moved permanently to '%s';"

                              " please relocate")

                              : _("Repository moved temporarily to '%s';"

                              " please relocate"),

                          svn_ra_neon__request_get_location(req, pool)));

 

            default:

              return svn_error_create

                (errcode, NULL,

                 apr_psprintf(pool,

                          _("Server sent unexpected return value (%d %s) "

                                "in response to %s request for '%s'"), req->code,

                          req->code_desc, req->method, req->url));

            }

 

Call stack:

#0  generate_error () at subversion/libsvn_ra_neon/util.c:563

#1  svn_ra_neon__request_dispatch () at subversion/libsvn_ra_neon/util.c:1475

#2  parsed_request (method="PROPFIND", url="/svncorp/svn/trunk/private") at subversion/libsvn_ra_neon/util.c:1229

#3  svn_ra_neon__parsed_request () at subversion/libsvn_ra_neon/util.c:1292

#4  svn_ra_neon__get_props () at subversion/libsvn_ra_neon/props.c:542

...

#9  svn_ra_neon__get_baseline_info (url="http://svncorp.org/svncorp/svn/trunk/private", revision=288) at subversion/libsvn_ra_neon/props.c:961

#10 svn_ra_neon__get_dir (path="private", revision=288) at subversion/libsvn_ra_neon/fetch.c:811

#11 svn_ra_get_dir2 (path= "private", revision=288) at subversion/libsvn_ra/ra_loader.c:623

#12 get_dir_contents (dirent_fields=1, dir="private", rev=288, fs_path= "/trunk", depth=svn_depth_infinity) at subversion/libsvn_client/list.c:66

#13 get_dir_contents (dirent_fields=1, dir= "", rev=288, fs_path= "/trunk", depth=svn_depth_infinity) at subversion/libsvn_client/list.c:99

#14 svn_client_list2 (path_or_url="http://svncorp.org/svncorp/svn/trunk",depth=svn_depth_infinity) at subversion/libsvn_client/list.c:264

#15 svn_cl__list () at subversion/svn/list-cmd.c:277

#16 main () at subversion/svn/main.c:2123

 

 

Root Cause:

Brief:

Skipping unauthorized access errors is required. Because previously they used SVN_ERR, they would simply quit. But with the patch they handled the error return status without using SVN_ERR.

Detail:

--- subversion/trunk/subversion/libsvn_client/list.c  2010/09/15 19:26:35            997470

+++ subversion/trunk/subversion/libsvn_client/list.c             2010/09/15 19:27:29            997471

@@ -63,14 +63,23 @@ get_dir_contents(apr_uint32_t dirent_fie

   apr_hash_t *tmpdirents;

   apr_pool_t *iterpool = svn_pool_create(pool);

   apr_array_header_t *array;

+  svn_error_t *err;

   int i;

 

/* In the error execution, svn_ra_get_dir2 returned

    err->apr_err == SVN_ERR_RA_NOT_AUTHORIZED, so in the buggy

    execution they will quit. But in the correct execution, they handle it

    specifically to allow it continue. */

-  /* Get the directory's entries, but not its props. */

-  SVN_ERR(svn_ra_get_dir2(ra_session, &tmpdirents, NULL, NULL,

-                          dir, rev, dirent_fields, pool));

+  /* Get the directory's entries, but not its props.  Ignore any

+         not-authorized errors.  */

+  err = svn_ra_get_dir2(ra_session, &tmpdirents, NULL, NULL,

+                        dir, rev, dirent_fields, pool);

+  if (err && ((err->apr_err == SVN_ERR_RA_NOT_AUTHORIZED) ||

+                  (err->apr_err == SVN_ERR_RA_DAV_FORBIDDEN)))

+        {

+          svn_error_clear(err);

+          return SVN_NO_ERROR;

+        }

+  SVN_ERR(err);

 

// Even if files are not authorized, just return NO_ERROR to skip

 

   if (ctx->cancel_func)

         SVN_ERR(ctx->cancel_func(ctx->cancel_baton));

·        Server side patch 1:

 

Update do_walk function in mod_dav_svn to filter out the unauthorized files, and it helps to avoid the blocking of clients.

 

--- subversion/trunk/subversion/mod_dav_svn/repos.c           2010/09/14 18:05:48            997025

+++ subversion/trunk/subversion/mod_dav_svn/repos.c 2010/09/14 18:08:59            997026

@@ -3957,6 +3957,7 @@ do_walk(walker_ctx_t *ctx, int depth)

   apr_size_t uri_len;

   apr_size_t repos_len;

   apr_hash_t *children;

+  apr_pool_t *iterpool;

 

   /* Clear the temporary pool. */

   svn_pool_clear(ctx->info.pool);

@@ -4032,6 +4033,7 @@ do_walk(walker_ctx_t *ctx, int depth)

                                 params->pool);

 

   /* iterate over the children in this collection */

+  iterpool = svn_pool_create(params->pool);

   for (hi = apr_hash_first(params->pool, children); hi; hi = apr_hash_next(hi))

         {

           const void *key;

@@ -4039,6 +4041,8 @@ do_walk(walker_ctx_t *ctx, int depth)

           void *val;

           svn_fs_dirent_t *dirent;

 

+          svn_pool_clear(iterpool);

+

           /* fetch one of the children */

           apr_hash_this(hi, &key, &klen, &val);

           dirent = val;

@@ -4046,7 +4050,16 @@ do_walk(walker_ctx_t *ctx, int depth)

           /* authorize access to this resource, if applicable */

           if (params->walk_type & DAV_WALKTYPE_AUTH)

             {

-              /* ### how/what to do? */

+              const char *repos_relpath =

+            apr_pstrcat(iterpool,

+                        apr_pstrmemdup(iterpool,

+                                           ctx->repos_path->data,

+                                       ctx->repos_path->len),

+                        key, NULL);

+              if (! dav_svn__allow_read(ctx->info.r, ctx->info.repos,

+                                    repos_relpath, ctx->info.root.rev,

+                                        iterpool))

+                continue;

             }

 // if there’s no authority to read files, then filter them out.

 

           /* append this child to our buffers */

@@ -4087,6 +4100,9 @@ do_walk(walker_ctx_t *ctx, int depth)

           ctx->uri->len = uri_len;

           ctx->repos_path->len = repos_len;

         }

+

+  svn_pool_destroy(iterpool);

+

   return NULL;

 }

 

·        Server side patch 2:

 

Make SVNSERVE not transmit ‘unreadable/no authority’ paths in its ‘get-dir’ command response.

 

--- subversion/trunk/subversion/svnserve/serve.c         2010/09/15 19:32:40            997473

+++ subversion/trunk/subversion/svnserve/serve.c   2010/09/15 19:39:04            997474

@@ -1452,6 +1452,14 @@ static svn_error_t *get_dir(svn_ra_svn_c

           svn_pool_clear(subpool);

 

               file_path = svn_uri_join(full_path, name, subpool);

+

+              if (! lookup_access(subpool, b, conn, svn_authz_read,

+                                  file_path, FALSE))

+                {

+                  apr_hash_set(entries, name, APR_HASH_KEY_STRING, NULL);

+                  continue;

+                }

+

// If files are not readable due to lack of authority, just do not transmit to the client

 

               entry = apr_pcalloc(pool, sizeof(*entry));

 

               if (dirent_fields & SVN_DIRENT_KIND)

 

 

·        Client side patch 1:

 

Skip SVN_ERR_RA_SVN_CMD_ERR. This error follows unauthorized file access.

 

Call stack:

#0  svn_ra_svn__handle_failure_status (...)

at subversion/libsvn_ra_svn/marshal.c:858

#1  svn_ra_svn_read_cmd_response (...)

at subversion/libsvn_ra_svn/marshal.c:885

#2  ra_svn_get_dir (fetched_rev=0x0, rev=10)

at subversion/libsvn_ra_svn/client.c:1039

#3  get_dir_contents (dirent_fields, dir="t3", rev=10, fs_path="/",

depth=svn_depth_infinity) at subversion/libsvn_client/list.c:66

#4  get_dir_contents (dirent_fields, dir="", rev=10, fs_path="/",

depth=svn_depth_infinity) at subversion/libsvn_client/list.c:99

#5  svn_client_list2

#6  svn_cl__list

#7  main

 

--- subversion/trunk/subversion/libsvn_ra_svn/marshal.c   2010/09/15 19:07:40            997456

+++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c              2010/09/15 19:10:14            997457

@@ -860,12 +860,27 @@ svn_error_t *svn_ra_svn__handle_failure_status

              easily change that, so "" means a nonexistent message. */

           if (!*message)

             message = NULL;

+      

+          /* Skip over links in the error chain that were intended only to

+             exist on the server (to wrap real errors intended for the

+             client) but accidentally got included in the server's actual

+             response. */

+          if ((apr_status_t)apr_err != SVN_ERR_RA_SVN_CMD_ERR)

+            {

           err = svn_error_create((apr_status_t)apr_err, err, message);

           err->file = apr_pstrdup(err->pool, file);

           err->line = (long)line;

         }

+        }

 // If error status is SVN_ERR_RA_SVN_CMD_ERR then, skip them

 

   svn_pool_destroy(subpool);

+

+  /* If we get here, then we failed to find a real error in the error

+         chain that the server proported to be sending us.  That's bad. */

+  if (! err)

+        err = svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,

+                           _("Malformed error list"));

+        

   return err;

 }

 

·        Client side patch 2:

 

Ignores authz errors raised when recursing when running ‘svn list’

This patch is solving the issue higher level than ‘client side patch 1’.

 

Either ‘client side patch1’ or ‘client side patch2’ makes the program work correctly, but developers have applied both patches somehow.

 

Failure type

wrong result

Is there any log message?

Yes. Printed via default-switch pattern