Karl Fogel wrote on Wed, 30 Apr 2008 at 18:00 -0400:
> danielsh_at_tigris.org writes:
> > Log:
> > In svnserve, tolerate unreadable passwd files.
> >
> > Review by: ghudson
> > Found by: Kristian Kauper <kkauper_at_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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-01 08:04:04 CEST