Re: svn commit: r30868 - trunk/subversion/svnserve

[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