Re: svn commit: r1554807 - in /subversion/trunk/subversion: libsvn_fs/editor.c libsvn_repos/delta.c libsvn_repos/rev_hunt.c mod_dav_svn/util.c mod_dav_svn/version.c

On 2 January 2014 at 17:01, <stefan2_at_apache.org> wrote:
> Author: stefan2
> Date: Thu Jan 2 14:01:43 2014
> New Revision: 1554807
>
> URL: http://svn.apache.org/r1554807
> Log:
> Replace the use of svn_fs_node_id + svn_fs_compare_ids with
> simple calls to svn_fs_node_relation.
>
Stefan,

I was looking to one of compiler warnings and noticed unbounded memory
usage problem in this commit. See my review inline.

[...]

> Modified: subversion/trunk/subversion/libsvn_repos/rev_hunt.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/rev_hunt.c?rev=1554807&r1=1554806&r2=1554807&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/rev_hunt.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/rev_hunt.c Thu Jan 2 14:01:43 2014
> @@ -309,11 +309,11 @@ svn_repos_deleted_rev(svn_fs_t *fs,
> apr_pool_t *pool)
> {
> apr_pool_t *subpool;
> - svn_fs_root_t *root, *copy_root;
> + svn_fs_root_t *start_root, *root, *copy_root;
> const char *copy_path;
> svn_revnum_t mid_rev;
> - const svn_fs_id_t *start_node_id, *curr_node_id;
> - svn_error_t *err;
> + svn_node_kind_t kind;
> + svn_fs_node_relation_t node_relation;
>
> /* Validate the revision range. */
> if (! SVN_IS_VALID_REVNUM(start))
> @@ -334,32 +334,19 @@ svn_repos_deleted_rev(svn_fs_t *fs,
> }
>
> /* Ensure path exists in fs at start revision. */
> - SVN_ERR(svn_fs_revision_root(&root, fs, start, pool));
> - err = svn_fs_node_id(&start_node_id, root, path, pool);
> - if (err)
> + SVN_ERR(svn_fs_revision_root(&start_root, fs, start, pool));
> + SVN_ERR(svn_fs_check_path(&kind, start_root, path, pool));
> + if (kind == svn_node_none)
> {
> - if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
> - {
> - /* Path must exist in fs at start rev. */
> - *deleted = SVN_INVALID_REVNUM;
> - svn_error_clear(err);
> - return SVN_NO_ERROR;
> - }
> - return svn_error_trace(err);
> + /* Path must exist in fs at start rev. */
> + *deleted = SVN_INVALID_REVNUM;
> + return SVN_NO_ERROR;
> }
>
> /* Ensure path was deleted at or before end revision. */
> SVN_ERR(svn_fs_revision_root(&root, fs, end, pool));
> - err = svn_fs_node_id(&curr_node_id, root, path, pool);
> - if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND)
> - {
> - svn_error_clear(err);
> - }
> - else if (err)
> - {
> - return svn_error_trace(err);
> - }
> - else
> + SVN_ERR(svn_fs_check_path(&kind, root, path, pool));
> + if (kind != svn_node_none)
> {
> /* path exists in the end node and the end node is equivalent
> or otherwise equivalent to the start node. This can mean
> @@ -386,8 +373,9 @@ svn_repos_deleted_rev(svn_fs_t *fs,
> 5) The start node was deleted and replaced by a node which
> it does not share any history with.
> */
> - SVN_ERR(svn_fs_node_id(&curr_node_id, root, path, pool));
> - if (svn_fs_compare_ids(start_node_id, curr_node_id) != -1)
> + SVN_ERR(svn_fs_node_relation(&node_relation, start_root, path,
> + root, path, pool));
> + if (node_relation != svn_fs_node_unrelated)
> {
> SVN_ERR(svn_fs_closest_copy(&copy_root, &copy_path, root,
> path, pool));
> @@ -450,28 +438,23 @@ svn_repos_deleted_rev(svn_fs_t *fs,
>
> /* Get revision root and node id for mid_rev at that revision. */
> SVN_ERR(svn_fs_revision_root(&root, fs, mid_rev, subpool));
> - err = svn_fs_node_id(&curr_node_id, root, path, subpool);
> -
> - if (err)
> + SVN_ERR(svn_fs_check_path(&kind, root, path, pool));
You're using wrong pool here: POOL instead of SUBPOOL (which is
actually iterpool). As result this function consume unbounded amount
of memory.

> + if (kind == svn_node_none)
> {
> - if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
> - {
> - /* Case D: Look lower in the range. */
> - svn_error_clear(err);
> - end = mid_rev;
> - mid_rev = (start + mid_rev) / 2;
> - }
> - else
> - return svn_error_trace(err);
> + /* Case D: Look lower in the range. */
> + end = mid_rev;
> + mid_rev = (start + mid_rev) / 2;
> }
> else
> {
> /* Determine the relationship between the start node
> and the current node. */
> - int cmp = svn_fs_compare_ids(start_node_id, curr_node_id);
> + SVN_ERR(svn_fs_node_relation(&node_relation, start_root, path,
> + root, path, pool));
Same here.

> + if (node_relation != svn_fs_node_unrelated)
> SVN_ERR(svn_fs_closest_copy(&copy_root, &copy_path, root,
> path, subpool));
> - if (cmp == -1 ||
> + if (node_relation == svn_fs_node_unrelated ||
> (copy_root &&
> (svn_fs_revision_root_revision(copy_root) > start)))
> {

I've fixed these problems in r1664084 and nominated to 1.9.x branch.

---
Ivan Zhakov