[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

[PATCH + RFC] authz scenario on 'reintegrate' branch?

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Sun, 20 Jan 2008 22:00:27 -0500

For way too long now, I've been trying to construct a scenario by
which I could test a patch I've written to address this "TODO(reint)"
comment in fs-wrap.c on the 'reintegrate' branch:

  /* We consciously do not perform authz checks on the paths returned
     in *MERGEINFO, avoiding massive authz overhead which would allow
     us to protect the name of where a change was merged from, but not
     the change itself. */
  /* ### TODO(reint): ... but how about descendant merged-to paths? */
  if (readable_paths->nelts > 0)
    SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
                                 include_descendants, pool));

Well, er, yes, how about descendant merged-to paths?

I could describe the zillion combinations of authz files and branch
maneuvers I've tried, hoping to stimulate the new code I've written
(see patch below). But that would take a lot of time, for probably
little benefit. Instead, can anyone *think* of a scenario by which
either of the two apr_hash_set() calls below might be reached? (Note
the debugging prints, in case you want to trace the process yourself.)

I ask knowing that I may be embarrassed by the obviousness of the
answer. However, spinning my wheels by myself has so far not proven
effective :-). When I try to construct scenarios, I can't ever make
one in which a pre-existing authz check (e.g. must_have_access() in
svnserve) doesn't fail first -- yet if some pre-existing check always
fails first, then we clearly don't need this patch.

Am I missing something?

-Karl

Index: subversion/libsvn_repos/fs-wrap.c
===================================================================
--- subversion/libsvn_repos/fs-wrap.c (revision 28964)
+++ subversion/libsvn_repos/fs-wrap.c (working copy)
@@ -23,9 +23,11 @@
 #include "svn_error.h"
 #include "svn_fs.h"
 #include "svn_path.h"
+#include "svn_hash.h"
 #include "svn_props.h"
 #include "svn_repos.h"
 #include "repos.h"
+#include "svn_mergeinfo.h"
 #include "svn_private_config.h"
 
 
@@ -595,10 +597,143 @@
      in *MERGEINFO, avoiding massive authz overhead which would allow
      us to protect the name of where a change was merged from, but not
      the change itself. */
- /* ### TODO(reint): ... but how about descendant merged-to paths? */
- if (readable_paths->nelts > 0)
- SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
- include_descendants, pool));
+ if (readable_paths->nelts > 0)
+ {
+ SVN_ERR(svn_fs_get_mergeinfo(mergeinfo, root, readable_paths, inherit,
+ include_descendants, pool));
+
+ if (include_descendants)
+ {
+ /* The mergeinfo might have paths that never got authz-checked;
+ if so, check them now. But only check the ones where we
+ can't know by examining 'paths' and 'readable_paths',
+ since an authz check is (relatively) expensive. */
+
+ apr_hash_index_t *hi;
+ apr_hash_t *original_paths_hash, *readable_paths_hash;
+
+#define KFF_DEBUG 1
+
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: include_descendants is true\n");
+ fprintf(fp, "KFF: paths: '%s'\n",
+ svn_cstring_join((apr_array_header_t *) paths,
+ ", ", pool));
+ fprintf(fp, "KFF: readable_paths: '%s'\n",
+ svn_cstring_join((apr_array_header_t *) readable_paths,
+ ", ", pool));
+ fprintf(fp, "KFF: *mergeinfo count: %d\n",
+ apr_hash_count(*mergeinfo));
+ fclose(fp);
+ }
+
+ /* Make lookups easy by converting to hash. */
+ SVN_ERR(svn_hash_from_cstring_keys(&original_paths_hash,
+ paths, pool));
+ if (readable_paths == paths)
+ {
+ readable_paths_hash = original_paths_hash;
+ }
+ else
+ {
+ SVN_ERR(svn_hash_from_cstring_keys(&readable_paths_hash,
+ readable_paths, pool));
+ }
+
+ for (hi = apr_hash_first(pool, *mergeinfo); hi;
+ hi = apr_hash_next(hi))
+ {
+ const void *key;
+ void *val;
+ const char *path;
+ const char *raw_mergeinfo_string;
+ apr_array_header_t *mergeinfo_value_paths;
+ int j;
+
+ svn_pool_clear(iterpool);
+ apr_hash_this(hi, &key, NULL, &val);
+ path = key;
+ raw_mergeinfo_string = val;
+
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: loop hit '%s'\n", path);
+ fprintf(fp, "KFF: mergeinfo for '%s':\n", path);
+ fprintf(fp, "'%s'\n",
+ (char *) apr_hash_get(*mergeinfo, path,
+ APR_HASH_KEY_STRING));
+ fprintf(fp, "KFF: (rp_h == op_h? --> %d)\n",
+ readable_paths_hash == original_paths_hash);
+ fclose(fp);
+ }
+
+ if (! apr_hash_get(readable_paths_hash, path,
+ APR_HASH_KEY_STRING))
+ {
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: '%s' is not in readable_paths\n",
+ path);
+ fclose(fp);
+ }
+
+ /* It's not in readable_paths, so if it *is* in
+ the original paths, that means it was already
+ authz-checked and found unreadable -- in which
+ case remove it from the mergeinfo. */
+ if (readable_paths_hash != original_paths_hash
+ && apr_hash_get(original_paths_hash, path,
+ APR_HASH_KEY_STRING))
+ {
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: '%s' is easily unreadable\n",
+ path);
+ fclose(fp);
+ }
+
+ apr_hash_set(*mergeinfo, path, APR_HASH_KEY_STRING,
+ NULL);
+ }
+ else
+ {
+ /* We have to actually do the authz check,
+ because we have no other information about
+ the path's readability. */
+ svn_boolean_t readable;
+ SVN_ERR(authz_read_func(&readable, root, path,
+ authz_read_baton, iterpool));
+ if (KFF_DEBUG)
+ {
+ FILE *fp = fopen("/tmp/kff.out", "a");
+ if (fp == NULL)
+ exit(1);
+ fprintf(fp, "KFF: '%s' authz-checked --> %d\n",
+ path, readable);
+ fclose(fp);
+ }
+
+ if (! readable)
+ apr_hash_set(*mergeinfo, path, APR_HASH_KEY_STRING,
+ NULL);
+ }
+ }
+ }
+ }
+ }
   else
     *mergeinfo = NULL;
 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-01-21 04:00:41 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.