[PATCH] Fix bug in recursive authz logic (Was: Re: problem with AUTHZ_SVN_RECURSIVE in mod_authz_svn.c)

[David Anderson <da...@calixo.net>]

I'm crossposting this to dev@ for review of my patches. Any followup 
discussion can probably stay on dev@ (remove users@ if replying, but 
keep Cc-ing to Bernd).

Bernd Rinn wrote:
> I think that I have found a bug in mod_authz_svn.c of svn 1.2.1 with
> respect to operations that require AUTHZ_SVN_RECURSIVE access.

Thanks for your report and the effort you put into identifying the cause 
of the problem!  You are indeed right, this is a nasty bug in the authz 
algorithm.

I have recently being altering the authz code quite wildly in trunk.  As 
such, your patch no longer applies at all (the relevant code is now in 
libsvn_repos).  I have ported your fix to be applicable to trunk, and 
ask, if it is satisfactory to the commiters, that the fix be nominated 
for inclusion in the upcoming 1.2.2 release.

As for your concerns about separators, there is no problem: paths in 
authz files are pieces of URIs, and as such always use forward slashes, 
whatever the platform.

Commiters: I include two patches that should be commited separately:
  - bernd_rinn_authz_bug_1.2.patch corrects the bug in a way similar to 
Bernd Rinn's proposed fix, and should be nominated for inclusion in 1.2.2 .
  - bernd_rinn_authz_bug_1.3.patch alters the code of the previous patch 
to use svn_path_is_ancestor, an new API of libsvn_subr introduced for 
svn 1.3 . The previous patch basically reimplements this function 
internal to authz.c; this patch undoes that and uses the new API.

Here are the two commit messages:

[[[
Fix a bug in the authz recursive lookup logic, reported at
http://subversion.tigris.org/servlets/ReadMsg?list=users&msgNo=35734 .

Suggested by: Bernd Rinn <be...@sdf.lonestar.org>
Patch by: David Anderson <da...@calixo.net>

* subversion/libsvn_repos/authz.c
   (authz_path_is_ancestor): New internal function.
   (authz_parse_section): use authz_path_is_ancestor to establish
     relationships between paths instead of just strncmp.
* subversion/tests/libsvn_repos/repos-test.c
   (authz): New regression test.
]]]

[[[
Port rXXXXX to the 1.3 API.

Patch by: David Anderson <da...@calixo.net>

* subversion/libsvn_repos/authz.c
   (authz_path_is_ancestor): Delete internal function.
   (authz_parse_section): use svn_path_is_ancestor instead of
     authz_path_is_ancestor.
]]]

Thanks again for your help!
- Dave.

Re: [PATCH] Fix bug in recursive authz logic (Was: Re: problem with AUTHZ_SVN_RECURSIVE in mod_authz_svn.c)

[David Anderson <da...@calixo.net>]

kfogel@collab.net wrote:
> We generally fix things on trunk first, and then backport to release 
> branches.  This case is a little odd, because you took your cue from
> a patch by Bernd that was written against 1.2 code -- which you redid
>  for trunk code, and then the "backport" of this trunk change was 
> simply Bernd's original patch with a few tweaks.

Woops, my first patch eligible for backporting, and I get it all wrong :-).

So, if I understand you correctly, the two patches become:
  - bernd_rinn_authz_bug_trunk.patch corrects the bug in trunk
  - bernd_rinn_authz_bug_1.2.patch corrects the bug using only 1.2 APIs

Each patch is to be commited to its own line, no merging required.  Is
that right?

> If you had to fix the bug twice, in two different ways, then there's
> no point expressing one change as a transformation of the other.

In this case, the backport simply implements the missing public API as a 
private utility function in authz.c. Given the size of the fix relative 
to this reimplementation, two separate independant patches make more sense.

Commit logs follow.

- Dave

[[[
Fix a bug in the authz recursive lookup logic, reported at
http://subversion.tigris.org/servlets/ReadMsg?list=users&msgNo=35734 .

Suggested by: Bernd Rinn <be...@sdf.lonestar.org>
Patch by: David Anderson <da...@calixo.net>

* subversion/libsvn_repos/authz.c
   (authz_parse_section): use svn_path_is_ancestor to establish
     relationships between paths instead of just strncmp.
* subversion/tests/libsvn_repos/repos-test.c
   (authz): New regression test.
]]]

[[[
Fix the same bug that was fixed on trunk in rXXXXX.  The bug made authz 
incorrectly consider some authz sections which didn't apply during 
recursive lookups.

Patch by: David Anderson <da...@calixo.net>

* subversion/libsvn_repos/authz.c
   (authz_path_is_ancestor): New internal function.  Implements the
     missing public API svn_path_is_ancestor, introduced in trunk.
   (authz_parse_section): use authz_path_is_ancestor to establish
     relationships between paths instead of just strncmp.
* subversion/tests/libsvn_repos/repos-test.c
   (authz): New regression test.
]]]