subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
Subject RE: svn commit: r1443556 - in /subversion/trunk/subversion: include/svn_auth.h libsvn_subr/simple_providers.c
Date Thu, 07 Feb 2013 16:16:37 GMT


> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: donderdag 7 februari 2013 16:44
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1443556 - in /subversion/trunk/subversion:
> include/svn_auth.h libsvn_subr/simple_providers.c
> 
> On Thu, Feb 07, 2013 at 03:31:47PM -0000, rhuijben@apache.org wrote:
> > Author: rhuijben
> > Date: Thu Feb  7 15:31:46 2013
> > New Revision: 1443556
> >
> > URL: http://svn.apache.org/viewvc?rev=1443556&view=rev
> > Log:
> > Fix a simple to trigger, but never reported segfault in the auth helper
code.
> >
> > * subversion/include/svn_auth.h
> >   (SVN_AUTH_PARAM_STORE_PLAINTEXT_PASSWORDS,
> >    SVN_AUTH_PARAM_DONT_STORE_SSL_CLIENT_CERT_PP,
> >    SVN_AUTH_PARAM_STORE_SSL_CLIENT_CERT_PP_PLAINTEXT): Mark as
> new in 1.6.
> >
> > * subversion/libsvn_subr/simple_providers.c
> >   (svn_auth__simple_creds_cache_set): If nobody has set
> >      SVN_AUTH_PARAM_STORE_PLAINTEXT_PASSWORDS (introduced in
> 1.6),
> >      we shouldn't segfault.
> 
> I agree that we shouldn't segfault. But it seems you've changed
> the default behaviour with this commit, from 'ask' to 'yes',
> in case the option isn't specified. See below.
> 
> > @@ -372,8 +372,9 @@ svn_auth__simple_creds_cache_set(svn_boo
> >            simple_provider_baton_t *b =
> >              (simple_provider_baton_t *)provider_baton;
> >
> > -          if (svn_cstring_casecmp(store_plaintext_passwords,
> > -                                  SVN_CONFIG_ASK) == 0)
> > +          if (store_plaintext_passwords
> > +              && svn_cstring_casecmp(store_plaintext_passwords,
> > +                                     SVN_CONFIG_ASK) == 0)
> 
> I think this should be:
> 
>           if (!store_plaintext_passwords
>               || svn_cstring_casecmp(store_plaintext_passwords,
>                                      SVN_CONFIG_ASK) == 0)
> 
> to make 'ask' the default catch-all case.

Which makes things much harder to verify, and will eventually fall back to
allowing it anyway, because with the pre-1.6 api there is no way to 'ask'.

All our initializers set this value one way or another, so I didn't change a
default. Or every svn invocation would have crashed since your change.
(See r1443562 for a reproduction recipe)

	Bert


Mime
View raw message