Julian Foad wrote on Tue, Sep 20, 2011 at 15:37:08 +0100:
> On Tue, 2011-09-20 at 17:19 +0300, Daniel Shahaf wrote:
> > I found it while reading the code; I don't know of a real-world bug that
> > this fixes. I suppose it fixes the locks check if someone calls, say,
> > svn_fs_change_node_prop() with a PATH argument that doesn't have
> > a leading slash [1], but I admit I haven't tested that.
> >
> > (The more-obvious code path, svn_fs_get_locks(), does canonicalize PATH,
> > causing it to get a leading slash, before passing it to lower-level code.)
>
> It looks like this code is adding a *trailing* slash, not a leading one.
>
Ahem, yes. Good catch. Although it doesn't undermine the original
theory ("KEY should match LOOKUP_PATH"), it does invalidate the
analysis above. Here's a new one:
That part of the function deals with locks on *children* of PATH.
LOOKUP_PATH grows a trailing slash in order to perform prefix filtering
on the table's keys --- see the strncmp() call. I guess the bug would
appear when /A2 and /A/mu are both locked, and the while() sees /A2
before /A/mu.
Thanks,
Daniel
(reminded of #desc4 of issue #1853.)
> - Julian
>
>
> > Julian Foad wrote on Tue, Sep 20, 2011 at 15:03:31 +0100:
> > > What does this fix?
> > >
> > > - Julian
> > >
> > >
> > > On Tue, 2011-09-20 at 16:25 +0300, Daniel Shahaf wrote:
> > > > Does this look correct? locks-test and lock_tests.py both pass.
> > > >
> > > > [[[
> > > > * subversion/libsvn_fs_base/bdb/locks-table.c
> > > > (svn_fs_bdb__locks_get):
> > > > Set LOOKUP_PATH before constructing BDB's KEY from it.
> > > > ]]]
> > > >
> > > > [[[
> > > > Index: subversion/libsvn_fs_base/bdb/locks-table.c
> > > > ===================================================================
> > > > --- subversion/libsvn_fs_base/bdb/locks-table.c (revision 1173129)
> > > > +++ subversion/libsvn_fs_base/bdb/locks-table.c (working copy)
> > > > @@ -206,7 +206,7 @@ svn_fs_bdb__locks_get(svn_fs_t *fs,
> > > > const char *lock_token;
> > > > svn_lock_t *lock;
> > > > svn_error_t *err;
> > > > - const char *lookup_path = path;
> > > > + const char *lookup_path;
> > > >
> > > > /* First, try to lookup PATH itself. */
> > > > err = svn_fs_bdb__lock_token_get(&lock_token, fs, path, trail, pool);
> > > > @@ -238,6 +238,11 @@ svn_fs_bdb__locks_get(svn_fs_t *fs,
> > > > &cursor, 0);
> > > > SVN_ERR(BDB_WRAP(fs, "creating cursor for reading lock tokens",
> > > > db_err));
> > > >
> > > > + if (!svn_fspath__is_root(path, strlen(path)))
> > > > + lookup_path = apr_pstrcat(pool, path, "/", (char *)NULL);
> > > > + else
> > > > + lookup_path = path;
> > > > +
> > > > /* Since the key is going to be returned as well as the value make
> > > > sure BDB malloc's the returned key. */
> > > > svn_fs_base__str_to_dbt(&key, lookup_path);
> > > > @@ -247,9 +252,6 @@ svn_fs_bdb__locks_get(svn_fs_t *fs,
> > > > the one passed in, by passing in the DB_RANGE_SET flag. */
> > > > db_err = svn_bdb_dbc_get(cursor, &key,
> > > > svn_fs_base__result_dbt(&value),
> > > > DB_SET_RANGE);
> > > > -
> > > > - if (!svn_fspath__is_root(path, strlen(path)))
> > > > - lookup_path = apr_pstrcat(pool, path, "/", (char *)NULL);
> > > >
> > > > /* As long as the prefix of the returned KEY matches LOOKUP_PATH we
> > > > know it is either LOOKUP_PATH or a decendant thereof. */
> > > > ]]]
> > >
> > >
>
>
