You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/04/30 22:00:45 UTC
Re: svn commit: r30868 - trunk/subversion/svnserve
danielsh@tigris.org writes:
> Log:
> In svnserve, tolerate unreadable passwd files.
>
> Review by: ghudson
> Found by: Kristian Kauper <kk...@au.yahoo-inc.com>
>
> * subversion/svnserve/serve.c
> (load_configs): Skip reading the passwd file if it is unreadable (EACCES),
> logging the error but allowing other authentications (such as --tunnel)
> to continue.
+1, but a note about an "XXX" comment you left:
> --- trunk/subversion/svnserve/serve.c (r30867)
> +++ trunk/subversion/svnserve/serve.c (r30868)
> @@ -236,7 +236,14 @@ svn_error_t *load_configs(svn_config_t *
> if (server)
> /* Called by listening server; log error no matter what it is. */
> log_server_error(err, server, conn, pool);
> - if (err->apr_err != SVN_ERR_BAD_FILENAME)
> +
> + /* XXX: why do ignore SVN_ERR_BAD_FILENAME here? (see r16840)
> + *
> + * If the passwd file is unreadable, skip reading it and continue;
> + * some authentications (e.g. --tunnel) can proceed without it.
> + */
> + if (err->apr_err != SVN_ERR_BAD_FILENAME
> + && ! APR_STATUS_IS_EACCES(err->apr_err))
> {
> if (server)
> {
If we look at the larger block around that, it starts off with a comment
that reiterates what r16840's log message says:
/* Because it may be possible to read the pwdb file with some
* access methods and not others, ignore errors reading the pwdb
* file and just don't present password authentication as an
* option. */
err = svn_config_read(pwdb, pwdb_path, TRUE, pool);
if (err)
{
if (server)
/* Called by listening server; log error no matter what it is. */
log_server_error(err, server, conn, pool);
/* XXX: why do ignore SVN_ERR_BAD_FILENAME here? (see r16840)
*
* If the passwd file is unreadable, skip reading it and continue;
* some authentications (e.g. --tunnel) can proceed without it.
*/
if (err->apr_err != SVN_ERR_BAD_FILENAME
&& ! APR_STATUS_IS_EACCES(err->apr_err))
{
if (server)
{
/* Called by listening server: Now that we've logged
* the error, clear it and return a nice, generic
* error to the user
* (http://subversion.tigris.org/issues/show_bug.cgi?id=2271). */
svn_error_clear(err);
return svn_error_create(SVN_ERR_AUTHN_FAILED, NULL, NULL);
}
/* Called during startup; return the error, whereupon it
* will go to standard error for the admin to see. */
return err;
}
else
/* Ignore SVN_ERR_BAD_FILENAME and APR_EACCES and proceed. */
svn_error_clear(err);
}
Is the question you're really asking: "Why did r16840 allow for
SVN_ERR_BAD_FILENAME but not APR_STATUS_IS_EACCES?" ?
If so, I don't know why. But at this point, it might make sense to
rearrange the comments to explain the current state of the code in the
clearest way possible, which I've attempted to do in r30871. Please
review to make sure I didn't let anything drop.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30868 - trunk/subversion/svnserve
Posted by Daniel Shahaf <da...@tigris.org>.
Karl Fogel wrote on Wed, 30 Apr 2008 at 18:00 -0400:
> danielsh@tigris.org writes:
> > Log:
> > In svnserve, tolerate unreadable passwd files.
> >
> > Review by: ghudson
> > Found by: Kristian Kauper <kk...@au.yahoo-inc.com>
> >
> > * subversion/svnserve/serve.c
> > (load_configs): Skip reading the passwd file if it is unreadable (EACCES),
> > logging the error but allowing other authentications (such as --tunnel)
> > to continue.
>
> +1, but a note about an "XXX" comment you left:
>
> > --- trunk/subversion/svnserve/serve.c (r30867)
> > +++ trunk/subversion/svnserve/serve.c (r30868)
> > @@ -236,7 +236,14 @@ svn_error_t *load_configs(svn_config_t *
> > if (server)
> > /* Called by listening server; log error no matter what it is. */
> > log_server_error(err, server, conn, pool);
> > - if (err->apr_err != SVN_ERR_BAD_FILENAME)
> > +
> > + /* XXX: why do ignore SVN_ERR_BAD_FILENAME here? (see r16840)
> > + *
> > + * If the passwd file is unreadable, skip reading it and continue;
> > + * some authentications (e.g. --tunnel) can proceed without it.
> > + */
> > + if (err->apr_err != SVN_ERR_BAD_FILENAME
> > + && ! APR_STATUS_IS_EACCES(err->apr_err))
> > {
> > if (server)
> > {
>
> If we look at the larger block around that, it starts off with a comment
> that reiterates what r16840's log message says:
>
> /* Because it may be possible to read the pwdb file with some
> * access methods and not others, ignore errors reading the pwdb
> * file and just don't present password authentication as an
> * option. */
> err = svn_config_read(pwdb, pwdb_path, TRUE, pool);
> if (err)
> {
> if (server)
> /* Called by listening server; log error no matter what it is. */
> log_server_error(err, server, conn, pool);
>
> /* XXX: why do ignore SVN_ERR_BAD_FILENAME here? (see r16840)
> *
> * If the passwd file is unreadable, skip reading it and continue;
> * some authentications (e.g. --tunnel) can proceed without it.
> */
> if (err->apr_err != SVN_ERR_BAD_FILENAME
> && ! APR_STATUS_IS_EACCES(err->apr_err))
> {
> if (server)
> {
> /* Called by listening server: Now that we've logged
> * the error, clear it and return a nice, generic
> * error to the user
> * (http://subversion.tigris.org/issues/show_bug.cgi?id=2271). */
> svn_error_clear(err);
> return svn_error_create(SVN_ERR_AUTHN_FAILED, NULL, NULL);
> }
> /* Called during startup; return the error, whereupon it
> * will go to standard error for the admin to see. */
> return err;
> }
> else
> /* Ignore SVN_ERR_BAD_FILENAME and APR_EACCES and proceed. */
> svn_error_clear(err);
> }
>
> Is the question you're really asking: "Why did r16840 allow for
> SVN_ERR_BAD_FILENAME but not APR_STATUS_IS_EACCES?" ?
The latter half of this question is, if I understood him correctly,
already explained by Greg; he said he had probably made an oversight.
The former half, however, I do not know the answer to. By comparison,
svn/main.c:main() only checks for APR_EACCES (but not for BAD_FILENAME)
when it calls svn_config_get_config() (in line 1787).
The question I am really asking is:
"When does svn_config_read() return SVN_ERR_BAD_FILENAME ?".
Hope I'm clearer now.
>
I've looked yesterday where BAD_FILENAME is used in libsvn_subr. It
appears 4 times in Windows-specific code, and once in each of
svn_io_detect_mimetype2, svn_path_get_absolute, svn_path_split_if_file.
> If so, I don't know why. But at this point, it might make sense to
> rearrange the comments to explain the current state of the code in the
> clearest way possible, which I've attempted to do in r30871. Please
> review to make sure I didn't let anything drop.
>
Done, +1.
> -Karl
>
Thanks Karl.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org