You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2012/04/01 09:36:48 UTC
Re: svn commit: r1307923 -
/subversion/trunk/subversion/libsvn_subr/crypto.c
gstein@apache.org wrote on Sat, Mar 31, 2012 at 22:24:00 -0000:
> @@ -137,9 +159,15 @@ svn_crypto__context_create(svn_crypto__c
>
> apr_err = apr_crypto_get_driver(&driver, "openssl", NULL, &apu_err,
> result_pool);
> - if (apr_err != APR_SUCCESS || driver == NULL)
> + /* Potential bugs in get_driver() imply we might get APR_SUCCESS and NULL.
> + Sigh. Just be a little more careful in error generation here. */
> + if (apr_err != APR_SUCCESS)
> return svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
The code still dereferences APU_ERR when APR_ERR != APR_SUCCESS..
> _("OpenSSL crypto driver error"));
> + if (driver == NULL)
> + return svn_error_create(APR_EGENERAL,
> + err_from_apu_err(APR_EGENERAL, apu_err),
> + _("Bad return value while loading"));
The error message doesn't say much when seen without context. (Consider
the SASL error messages that we had to s/%s/SASL said: %s/.) IMO it
should say something about "Creating crypto context" or such.
Re: svn commit: r1307923 -
/subversion/trunk/subversion/libsvn_subr/crypto.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Mon, Apr 02, 2012 at 05:51:19 -0400:
> On Sun, Apr 1, 2012 at 03:36, Daniel Shahaf <da...@elego.de> wrote:
> > gstein@apache.org wrote on Sat, Mar 31, 2012 at 22:24:00 -0000:
> >> @@ -137,9 +159,15 @@ svn_crypto__context_create(svn_crypto__c
> >>
> >> apr_err = apr_crypto_get_driver(&driver, "openssl", NULL, &apu_err,
> >> result_pool);
> >> - if (apr_err != APR_SUCCESS || driver == NULL)
> >> + /* Potential bugs in get_driver() imply we might get APR_SUCCESS and NULL.
> >> + Sigh. Just be a little more careful in error generation here. */
> >> + if (apr_err != APR_SUCCESS)
> >> return svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
> >
> > The code still dereferences APU_ERR when APR_ERR != APR_SUCCESS..
>
> That's fine. The first thing get_driver() does is to set apu_err to
> NULL. So it will be NULL or valid.
>
> Yes, this is an implementation detail, but apr(util) doesn't have the
> same sorts of API specification like we have (ie. if an error is
> returned, don't count on any OUT params). Without those details, we
> gotta look at the code. (sigh)
>
OK, thanks.
> >> _("OpenSSL crypto driver error"));
> >> + if (driver == NULL)
> >> + return svn_error_create(APR_EGENERAL,
> >> + err_from_apu_err(APR_EGENERAL, apu_err),
> >> + _("Bad return value while loading"));
> >
> > The error message doesn't say much when seen without context. (Consider
> > the SASL error messages that we had to s/%s/SASL said: %s/.) IMO it
> > should say something about "Creating crypto context" or such.
>
> Yeah. I made it a bit lame on purpose because I wasn't so sure what to
> tell users, and for the translators. I figured as long as it was
> unique, we would know what was going on, even if the user didn't. How
> about "Bad return value while loading crypto driver" ?
>
+1.
May want even to say "openssl" in the error, too --- the name of the
driver we load --- it'll probably save me telling some people "That
error probably means you have multiple OpenSSL .so's" on users@ :)
> Cheers,
> -g
Re: svn commit: r1307923 - /subversion/trunk/subversion/libsvn_subr/crypto.c
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Apr 1, 2012 at 03:36, Daniel Shahaf <da...@elego.de> wrote:
> gstein@apache.org wrote on Sat, Mar 31, 2012 at 22:24:00 -0000:
>> @@ -137,9 +159,15 @@ svn_crypto__context_create(svn_crypto__c
>>
>> apr_err = apr_crypto_get_driver(&driver, "openssl", NULL, &apu_err,
>> result_pool);
>> - if (apr_err != APR_SUCCESS || driver == NULL)
>> + /* Potential bugs in get_driver() imply we might get APR_SUCCESS and NULL.
>> + Sigh. Just be a little more careful in error generation here. */
>> + if (apr_err != APR_SUCCESS)
>> return svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
>
> The code still dereferences APU_ERR when APR_ERR != APR_SUCCESS..
That's fine. The first thing get_driver() does is to set apu_err to
NULL. So it will be NULL or valid.
Yes, this is an implementation detail, but apr(util) doesn't have the
same sorts of API specification like we have (ie. if an error is
returned, don't count on any OUT params). Without those details, we
gotta look at the code. (sigh)
>> _("OpenSSL crypto driver error"));
>> + if (driver == NULL)
>> + return svn_error_create(APR_EGENERAL,
>> + err_from_apu_err(APR_EGENERAL, apu_err),
>> + _("Bad return value while loading"));
>
> The error message doesn't say much when seen without context. (Consider
> the SASL error messages that we had to s/%s/SASL said: %s/.) IMO it
> should say something about "Creating crypto context" or such.
Yeah. I made it a bit lame on purpose because I wasn't so sure what to
tell users, and for the translators. I figured as long as it was
unique, we would know what was going on, even if the user didn't. How
about "Bad return value while loading crypto driver" ?
Cheers,
-g