You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gerald Macinenti <ge...@letitwave.fr> on 2008/05/28 15:05:51 UTC
[PATCH] issue #2662, yet another authz wildcards support patch...
Hi,
Here is a simple patch which seams to work for us in order to use
wildcards in authz access file paths.
I've seen the patch by Rodrigo Gallardo but frankly didn't understand
all the details (I'm not a Subversion nor a C expert sorry ;)), but I
think mine takes the problem at a lower level, or maybe is it just to naive?
I basically defined a find_section function to replace find_option in
svn_config_enumerate2 and used apr_fnmatch to get the wildcard matching
of configuration sections.
I didn't write any specific test yet but existing tests passed well.
I submit it to get some feedback/advises about it and then come back
with a better one if it can be useful.
Cheers.
--
Gerald MACINENTI, System/Network Administrator
LET IT WAVE, http://www.letitwave.fr
8-16, rue Paul-Vaillant-Couturier, 92240 Malakoff, FRANCE
Ph. +33-1-4092-5454 - Fax. +33-1-4092-5442
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@davidglasser.net> writes:
> On Wed, May 28, 2008 at 9:11 AM, Karl Fogel <kf...@red-bean.com> wrote:
>> Oh, we traditionally call it "iter_pool". No big deal, and obviously we
>> could just tweak it at commit time; I just noticed it and thought I'd
>> mention it.
>
> Shirley you mean iterpool?
Yes, sorry about that: "iterpool" is the standard (again, not that this
would ever prevent a patch from being applied).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by David Glasser <gl...@davidglasser.net>.
On Wed, May 28, 2008 at 9:11 AM, Karl Fogel <kf...@red-bean.com> wrote:
> Oh, we traditionally call it "iter_pool". No big deal, and obviously we
> could just tweak it at commit time; I just noticed it and thought I'd
> mention it.
Shirley you mean iterpool?
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Karl Fogel <kf...@red-bean.com>.
Gerald Macinenti <ge...@letitwave.fr> writes:
> --- subversion/libsvn_subr/config.c.orig
> +++ subversion/libsvn_subr/config.c
> @@ -356,6 +357,38 @@
> }
>
>
> +/* Find a matching section in the configuration with wildcard support
> + Return a pointer to a section in CFG, or NULL if it doesn't exist. */
> +static cfg_section_t *
> +find_section(svn_config_t *cfg, const char *section, apr_pool_t *pool)
> +{
> + apr_hash_index_t *sec_ndx;
> + apr_pool_t *iteration_pool;
> + cfg_section_t * sectionp = NULL;
> + int count = 0;
Oh, we traditionally call it "iter_pool". No big deal, and obviously we
could just tweak it at commit time; I just noticed it and thought I'd
mention it.
> + iteration_pool = svn_pool_create(pool);
> + for (sec_ndx = apr_hash_first(iteration_pool, cfg->sections);
> + sec_ndx != NULL;
> + sec_ndx = apr_hash_next(sec_ndx))
> + {
> + void *sec_ptr;
> + const void *sec_name;
> +
> + apr_hash_this(sec_ndx, &sec_name, NULL, &sec_ptr);
> + if (apr_fnmatch(sec_name, section, APR_FNM_PATHNAME | APR_FNM_CASE_BLIND) == APR_SUCCESS) {
> + sectionp = sec_ptr;
> + break;
> + }
> + ++count;
> + svn_pool_clear(iteration_pool);
> + }
Clear the iteration_pool at the top of the loop, not the bottom, so that
eventual loop-condition failure doesn't leave dangling storage.
Also, are you sure you want to pass iteration_pool to apr_hash_first(),
not pool? Think: we'll clear iteration_pool at the bottom of the first
iteration, then we'll refer into sec_ndx in order create a new sec_ndx.
That seems bad :-).
> + svn_pool_destroy(iteration_pool);
> +
> + return sectionp;
> +}
Finally, what is the 'count' variable for?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-05-30 14:56:47 Gerald Macinenti napisał(a):
> Daniel Shahaf wrote:
> > Gerald Macinenti wrote on Fri, 30 May 2008 at 10:33 +0200:
> >> this new version of the patch addresses the following issues some of you
> >> pointed me at:
> >> * superfluous use of "count" and "iteration_pool"
> >> * try find_option before find_section so that exactly matching sections
> >> have precedence over wildcard sections
> >> * added documentation and log message
> >> * GNU coding style
> >>
> >>
> >
> > Gerald, did you write and test the patch against trunk or against 1.4.6?
> > This time it applies cleanly, but in general patches should be written
> > (and tested) against trunk. Did you test the new functionality with trunk?
> against trunk, and yes I tested the new functionality in a real
> installation from trunk sources (apache + mod_authz_svn only, not svnserve).
>
> But the "make check" that was all good when I was working on the CentOS5
> 1.4.2 version skips all the authz tests in trunk (with or without the
> patch):
> SKIP: authz_tests.py 1: authz issue #2486 - open root
> SKIP: authz_tests.py 2: authz issue #2486 - open directory
> SKIP: authz_tests.py 3: broken authz files cause errors
> SKIP: authz_tests.py 4: test authz for read operations
> SKIP: authz_tests.py 5: test authz for write operations
> SKIP: authz_tests.py 6: test authz for checkout
> SKIP: authz_tests.py 7: test authz for log and tracing path changes
> SKIP: authz_tests.py 8: test authz for checkout and update
> SKIP: authz_tests.py 9: test authz for export with unreadable subfolder
> SKIP: authz_tests.py 10: test authz for aliases
> SKIP: authz_tests.py 11: test the authz validation rules
> SKIP: authz_tests.py 12: test authz for locking
> SKIP: authz_tests.py 13: authz issue #2712
> SKIP: authz_tests.py 14: switched to directory, no read access on parents
>
> why? I give the tests log in attachment.
NEVER SEND SUCH HUGE FILES IN UNCOMPRESSED FORM!!!
This attachment has ~3041 KiB (~2,97 MiB), but after compressing it with bzip2
it would have only ~95 KiB.
--
Arfrever Frehtes Taifersar Arahesis
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Gerald Macinenti <ge...@letitwave.fr>.
David Glasser wrote:
> 2008/5/30 Gerald Macinenti <ge...@letitwave.fr>:
>> But the "make check" that was all good when I was working on the CentOS5
>> 1.4.2 version skips all the authz tests in trunk (with or without the
>> patch):
>
> ra_local (file:///) doesn't support authz, so the tests aren't run.
> You should test against svnserve; you might find
> subversion/tests/cmdline/svnserveautocheck.sh helpful.
excellent, thank you for this, so I can now say that all tests are passing.
I suppose the next step is to add test cases to check that wildcards are
effectively supported...
--
Gerald MACINENTI, System/Network Administrator
LET IT WAVE, http://www.letitwave.fr
8-16, rue Paul-Vaillant-Couturier, 92240 Malakoff, FRANCE
Ph. +33-1-4092-5454 - Fax. +33-1-4092-5442
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by David Glasser <gl...@davidglasser.net>.
2008/5/30 Gerald Macinenti <ge...@letitwave.fr>:
> But the "make check" that was all good when I was working on the CentOS5
> 1.4.2 version skips all the authz tests in trunk (with or without the
> patch):
ra_local (file:///) doesn't support authz, so the tests aren't run.
You should test against svnserve; you might find
subversion/tests/cmdline/svnserveautocheck.sh helpful.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support
patch...
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Gerald Macinenti wrote on Fri, 30 May 2008 at 14:56 +0200:
> But the "make check" that was all good when I was working on the CentOS5 1.4.2
> version skips all the authz tests in trunk (with or without the patch):
> SKIP: authz_tests.py 1: authz issue #2486 - open root
> SKIP: authz_tests.py 2: authz issue #2486 - open directory
> SKIP: authz_tests.py 3: broken authz files cause errors
> SKIP: authz_tests.py 4: test authz for read operations
> SKIP: authz_tests.py 5: test authz for write operations
> SKIP: authz_tests.py 6: test authz for checkout
> SKIP: authz_tests.py 7: test authz for log and tracing path changes
> SKIP: authz_tests.py 8: test authz for checkout and update
> SKIP: authz_tests.py 9: test authz for export with unreadable subfolder
> SKIP: authz_tests.py 10: test authz for aliases
> SKIP: authz_tests.py 11: test the authz validation rules
> SKIP: authz_tests.py 12: test authz for locking
> SKIP: authz_tests.py 13: authz issue #2712
> SKIP: authz_tests.py 14: switched to directory, no read access on parents
>
> why?
Please refer to the definition of 'test_list' in authz_tests.py:
test_list = [ None,
Skip(authz_open_root, svntest.main.is_ra_type_file),
Skip(authz_open_directory, svntest.main.is_ra_type_file),
Skip(broken_authz_file, svntest.main.is_ra_type_file),
Skip(authz_read_access, svntest.main.is_ra_type_file),
Skip(authz_write_access, svntest.main.is_ra_type_file),
Skip(authz_checkout_test, svntest.main.is_ra_type_file),
Skip(authz_log_and_tracing_test, svntest.main.is_ra_type_file),
Skip(authz_checkout_and_update_test,
svntest.main.is_ra_type_file),
Skip(authz_partial_export_test, svntest.main.is_ra_type_file),
SkipUnless(Skip(authz_aliases, svntest.main.is_ra_type_file),
server_authz_has_aliases),
Skip(authz_validate, svntest.main.is_ra_type_file),
Skip(authz_locking, svntest.main.is_ra_type_file),
XFail(SkipUnless(authz_svnserve_anon_access_read,
svntest.main.is_ra_type_svn)),
XFail(Skip(authz_switch_to_directory,
svntest.main.is_ra_type_file)),
]
is_ra_type_file checks if you're running over ra_local.
> I give the tests log in attachment.
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support
patch...
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Gerald Macinenti wrote on Fri, 30 May 2008 at 15:26 +0200:
> Daniel Shahaf wrote:
>
> > Okay. I saw it wasn't `svn diff` format and had 1.4.6 in the name, so I
> > assumed it was against 1.4.6. Thanks for clarifying.
>
> You're right, sorry, here is the patch generated with svn diff.
>
For the record, I meant that as an observation, not as criticism. It's
fine to send patches as unidiffs; the reason most people don't do it is
just they find 'svn diff' easier. (Admittedly, sending `svn diff` output
helps slightly, because we're used to reading it and because it avoids
'patch -p' games.)
Thanks for resending, though!
Daniel
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Gerald Macinenti <ge...@letitwave.fr>.
Daniel Shahaf wrote:
> Okay. I saw it wasn't `svn diff` format and had 1.4.6 in the name, so I
> assumed it was against 1.4.6. Thanks for clarifying.
You're right, sorry, here is the patch generated with svn diff.
--
Gerald MACINENTI, System/Network Administrator
LET IT WAVE, http://www.letitwave.fr
8-16, rue Paul-Vaillant-Couturier, 92240 Malakoff, FRANCE
Ph. +33-1-4092-5454 - Fax. +33-1-4092-5442
Re: [PATCH] issue #2662, yet another authz wildcards support
patch...
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Gerald Macinenti wrote on Fri, 30 May 2008 at 14:56 +0200:
> Daniel Shahaf wrote:
> > Gerald Macinenti wrote on Fri, 30 May 2008 at 10:33 +0200:
> > > this new version of the patch addresses the following issues some of you
> > > pointed me at:
> > > * superfluous use of "count" and "iteration_pool"
> > > * try find_option before find_section so that exactly matching sections
> > > have precedence over wildcard sections
> > > * added documentation and log message
> > > * GNU coding style
> > >
> > >
> >
> > Gerald, did you write and test the patch against trunk or against 1.4.6?
> > This time it applies cleanly, but in general patches should be written
> > (and tested) against trunk. Did you test the new functionality with
> > trunk?
> against trunk, and yes I tested the new functionality in a real installation
> from trunk sources (apache + mod_authz_svn only, not svnserve).
>
Okay. I saw it wasn't `svn diff` format and had 1.4.6 in the name, so I
assumed it was against 1.4.6. Thanks for clarifying.
> But the "make check" that was all good when I was working on the CentOS5
> 1.4.2 version skips all the authz tests in trunk (with or without the
> patch):
> SKIP: authz_tests.py 1: authz issue #2486 - open root
> SKIP: authz_tests.py 2: authz issue #2486 - open directory
> SKIP: authz_tests.py 3: broken authz files cause errors
> SKIP: authz_tests.py 4: test authz for read operations
> SKIP: authz_tests.py 5: test authz for write operations
> SKIP: authz_tests.py 6: test authz for checkout
> SKIP: authz_tests.py 7: test authz for log and tracing path changes
> SKIP: authz_tests.py 8: test authz for checkout and update
> SKIP: authz_tests.py 9: test authz for export with unreadable subfolder
> SKIP: authz_tests.py 10: test authz for aliases
> SKIP: authz_tests.py 11: test the authz validation rules
> SKIP: authz_tests.py 12: test authz for locking
> SKIP: authz_tests.py 13: authz issue #2712
> SKIP: authz_tests.py 14: switched to directory, no read access on parents
>
> why? I give the tests log in attachment.
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Gerald Macinenti <ge...@letitwave.fr>.
Daniel Shahaf wrote:
> Gerald Macinenti wrote on Fri, 30 May 2008 at 10:33 +0200:
>> this new version of the patch addresses the following issues some of you
>> pointed me at:
>> * superfluous use of "count" and "iteration_pool"
>> * try find_option before find_section so that exactly matching sections
>> have precedence over wildcard sections
>> * added documentation and log message
>> * GNU coding style
>>
>>
>
> Gerald, did you write and test the patch against trunk or against 1.4.6?
> This time it applies cleanly, but in general patches should be written
> (and tested) against trunk. Did you test the new functionality with trunk?
against trunk, and yes I tested the new functionality in a real
installation from trunk sources (apache + mod_authz_svn only, not svnserve).
But the "make check" that was all good when I was working on the CentOS5
1.4.2 version skips all the authz tests in trunk (with or without the
patch):
SKIP: authz_tests.py 1: authz issue #2486 - open root
SKIP: authz_tests.py 2: authz issue #2486 - open directory
SKIP: authz_tests.py 3: broken authz files cause errors
SKIP: authz_tests.py 4: test authz for read operations
SKIP: authz_tests.py 5: test authz for write operations
SKIP: authz_tests.py 6: test authz for checkout
SKIP: authz_tests.py 7: test authz for log and tracing path changes
SKIP: authz_tests.py 8: test authz for checkout and update
SKIP: authz_tests.py 9: test authz for export with unreadable subfolder
SKIP: authz_tests.py 10: test authz for aliases
SKIP: authz_tests.py 11: test the authz validation rules
SKIP: authz_tests.py 12: test authz for locking
SKIP: authz_tests.py 13: authz issue #2712
SKIP: authz_tests.py 14: switched to directory, no read access on parents
why? I give the tests log in attachment.
--
Gerald MACINENTI, System/Network Administrator
LET IT WAVE, http://www.letitwave.fr
8-16, rue Paul-Vaillant-Couturier, 92240 Malakoff, FRANCE
Ph. +33-1-4092-5454 - Fax. +33-1-4092-5442
Re: [PATCH] issue #2662, yet another authz wildcards support
patch...
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Gerald Macinenti wrote on Fri, 30 May 2008 at 10:33 +0200:
> this new version of the patch addresses the following issues some of you
> pointed me at:
> * superfluous use of "count" and "iteration_pool"
> * try find_option before find_section so that exactly matching sections
> have precedence over wildcard sections
> * added documentation and log message
> * GNU coding style
>
>
Gerald, did you write and test the patch against trunk or against 1.4.6?
This time it applies cleanly, but in general patches should be written
(and tested) against trunk. Did you test the new functionality with trunk?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Gerald Macinenti <ge...@letitwave.fr>.
this new version of the patch addresses the following issues some of you
pointed me at:
* superfluous use of "count" and "iteration_pool"
* try find_option before find_section so that exactly matching sections
have precedence over wildcard sections
* added documentation and log message
* GNU coding style
--
Gerald MACINENTI, System/Network Administrator
LET IT WAVE, http://www.letitwave.fr
8-16, rue Paul-Vaillant-Couturier, 92240 Malakoff, FRANCE
Ph. +33-1-4092-5454 - Fax. +33-1-4092-5442
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Karl Fogel <kf...@red-bean.com>.
Gerald Macinenti <ge...@letitwave.fr> writes:
> Here is a simple patch which seams to work for us in order to use
> wildcards in authz access file paths.
>
> I've seen the patch by Rodrigo Gallardo but frankly didn't understand
> all the details (I'm not a Subversion nor a C expert sorry ;)), but I
> think mine takes the problem at a lower level, or maybe is it just to
> naive?
>
> I basically defined a find_section function to replace find_option in
> svn_config_enumerate2 and used apr_fnmatch to get the wildcard
> matching of configuration sections.
>
> I didn't write any specific test yet but existing tests passed well.
>
> I submit it to get some feedback/advises about it and then come back
> with a better one if it can be useful.
Thanks. One thing that would help would be a log message; see
http://subversion.tigris.org/hacking.html#patches for more on submitting
patches.
It's great that you documented the new function; people often overlook
that. The documentation doesn't state what happens in the case of
multiple matches, though. Since the sections are in a hash table, my
guess is that a wildcard with multiple matches will behave unpredictably
-- there's no way to know which one it will match. I'm not saying this
is necessarily a bug, but it should be documented.
I haven't had time to review your patch beyond the above, but I've
linked to your message from issue #2662.
-Karl
> --- subversion/libsvn_subr/config.c.orig
> +++ subversion/libsvn_subr/config.c
> @@ -24,6 +24,7 @@
>
> #include <apr_general.h>
> #include <apr_lib.h>
> +#include <apr_fnmatch.h>
> #include "svn_error.h"
> #include "svn_pools.h"
> #include "config_impl.h"
> @@ -356,6 +357,38 @@
> }
>
>
> +/* Find a matching section in the configuration with wildcard support
> + Return a pointer to a section in CFG, or NULL if it doesn't exist. */
> +static cfg_section_t *
> +find_section(svn_config_t *cfg, const char *section, apr_pool_t *pool)
> +{
> + apr_hash_index_t *sec_ndx;
> + apr_pool_t *iteration_pool;
> + cfg_section_t * sectionp = NULL;
> + int count = 0;
> +
> + iteration_pool = svn_pool_create(pool);
> + for (sec_ndx = apr_hash_first(iteration_pool, cfg->sections);
> + sec_ndx != NULL;
> + sec_ndx = apr_hash_next(sec_ndx))
> + {
> + void *sec_ptr;
> + const void *sec_name;
> +
> + apr_hash_this(sec_ndx, &sec_name, NULL, &sec_ptr);
> + if (apr_fnmatch(sec_name, section, APR_FNM_PATHNAME | APR_FNM_CASE_BLIND) == APR_SUCCESS) {
> + sectionp = sec_ptr;
> + break;
> + }
> + ++count;
> + svn_pool_clear(iteration_pool);
> + }
> + svn_pool_destroy(iteration_pool);
> +
> + return sectionp;
> +}
> +
> +
> /* Return a pointer to an option in CFG, or NULL if it doesn't exist.
> if SECTIONP is non-null, return a pointer to the option's section.
> OPTION may be NULL. */
> @@ -794,7 +827,7 @@
> apr_pool_t *iteration_pool;
> int count;
>
> - find_option(cfg, section, NULL, &sec);
> + sec = find_section(cfg, section, pool);
> if (sec == NULL)
> return 0;
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support
patch...
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Arfrever Frehtes Taifersar Arahesis wrote on Thu, 29 May 2008 at 14:28 +0200:
> Please read http://subversion.tigris.org/hacking.html#coding-style
>
You could be more specific. Gerald, the "if" is not correctly indented,
and is over 80 columns.
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
Please read http://subversion.tigris.org/hacking.html#coding-style
--
Arfrever Frehtes Taifersar Arahesis
Re: [PATCH] issue #2662, yet another authz wildcards support patch...
Posted by Martin Furter <mf...@rola.ch>.
On Wed, 28 May 2008, Gerald Macinenti wrote:
> @@ -794,7 +827,7 @@
> apr_pool_t *iteration_pool;
> int count;
>
> - find_option(cfg, section, NULL, &sec);
> + sec = find_section(cfg, section, pool);
> if (sec == NULL)
> return 0;
Would it be better (and faster) if first find_option is called and if it
didn't find anything then find_section is called?
Just a random idea...
Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org