On Jan 20, 2008 7:00 PM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> 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?
I'm imagining:
anon-access=none
auth-access=write
user jrandom has explicit read access to / but no access at all to /foo/bar
/foo and /foo/bar both have svn:mergeinfo explicitly set on them.
jrandom calls svn_ra_get_mergeinfo ("/foo", include_descendents=true).
Does that not do the trick?
--dave
> -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
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
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-23 02:08:03 CET