Re: [PATCH] reintegrate branch mergeinfo authz check (for glasser)

On Jan 10, 2008 2:41 AM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> David, this is for your review if you get a chance. I'm still writing
> a test for it, but wanted to parallelize that process with review...
>
> -Karl
>
> [[[
> On the reintegrate branch:
>
> * subversion/libsvn_repos/fs-wrap.c
> (svn_repos_fs_get_mergeinfo): Do authz checking on the paths
> returned in mergeinfo that haven't already been checked.
>
> * reintegrate-branch-TODO: Remove the reminder about this.
> ]]]
>
> Index: subversion/libsvn_repos/fs-wrap.c
> ===================================================================
> --- subversion/libsvn_repos/fs-wrap.c (revision 28840)
> +++ subversion/libsvn_repos/fs-wrap.c (working copy)
> @@ -595,10 +595,65 @@
> 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. */
> +
> + /* Make lookups easy by converting to hash. */
> + apr_hash_t *original_paths_hash, *readable_paths_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 (i = 0; i < mergeinfo->nelts; i++)

Uh, isn't mergeinfo an apr_hash_t **? This is not how you iterate
over a hash...

> + {
> + const char *path = APR_ARRAY_IDX(paths, i, char *);

... and this should be pulling from mergeinfo, not paths.

> + svn_pool_clear(iterpool);
> +
> + if (! apr_hash_get(readable_paths_hash, path,
> + APR_HASH_KEY_STRING))
> + {
> + /* 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))
> + {
> + apr_hash_set(mergeinfo, path, 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 (! readable)
> + apr_hash_set(mergeinfo, path, NULL);
> + }
> + }
> + }
> + }
> + }
> else
> *mergeinfo = NULL;
>
> Index: reintegrate-branch-TODO
> ===================================================================
> --- reintegrate-branch-TODO (revision 28840)
> +++ reintegrate-branch-TODO (working copy)
> @@ -20,8 +20,6 @@
>
> * "Is this the same line?" direct ancestor check (glasser)
>
> - * authz check (glasser)
> -
> * remember there may be some TODO(reint) comments in .py tests; they
> won't show up in a tags-search.
>
>
> ---------------------------------------------------------------------
> 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