Report SSL error messages from serf

[minfrin]

Add the most detailed underlying crypto library error string to the error stack when the context fails due to an SSL failure.

SSL errors are no longer reduced to "an error has occurred".

This relies on the serf_ssl_error_cb_t callback as provided by serf in apache/serf#9.

Example:

[minfrin@rocky9 subversion]$ svn info https://svn.example.com/svn/example/core/
svn: E170013: Unable to connect to a repository at URL 'https://svn.example.com/svn/example/core'
svn: E120171: TLS: error:0308010C:digital envelope routines::unsupported
svn: E120171: Error running context: An error occurred during SSL communication

[brainy]

Can this be called more than once before serf_context_run() returns?

[brainy]

Sorry, never mind, of course it can.

The right thing to do here would be to make session->ssl_error an svn_error_t, then just chain those errors together as they arrive, and wrap the chain when reporting the error. Could even drop the if there because wrapping a null svn_error_t is just fine. The nice thing about chaining errors is that the whole chain uses just the one (standalone) pool that's created for the first error.

[brainy]

Something like this:

    session->ssl_error = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR,
                                          session->ssl_error, message);

The trick is that svn_error_create() will copy the message to the error's internal pool; instead of the messages polluting the session pool without any reasonable bounds, they'll just vanish along with the error chain when it's cleared. You'd also get the messages in a more natural order, with the one returned from serf_context_run() on the top of the stack instead of the bottom.

[minfrin]

That's a far better option, it has been made so.

[brainy]

sess->ssl_error should be set to NULL before returning. Even if sess is discarded, it doesn't hurt to be future safe.

It also strikes me that the if here is not necessary. Instead, svn_ra_serf__wrap_err() should take a new parameter for the child error, and wrap the whole chain. Something like this, in util_error.c:

--- util_error.c	(revision 1926861)
+++ util_error.c	(working copy)
@@ -44,6 +44,7 @@
 
 svn_error_t *
 svn_ra_serf__wrap_err(apr_status_t status,
+                      svn_error_t *child,
                       const char *fmt,
                       ...)
 {
@@ -51,7 +52,7 @@ svn_ra_serf__wrap_err(apr_status_t status,
   svn_error_t *err;
   va_list ap;
 
-  err = svn_error_create(status, NULL, NULL);
+  err = svn_error_create(status, child, NULL);
 
   if (serf_err_msg || fmt)
     {

then you can just call svn_ra_serf__wrap_error(status, sess->ssl_error, ...`.

[minfrin]

Change makes a lot of sense, but we touch a lot of files.

subversion/libsvn_ra_serf/xml.c:        return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/xml.c:          return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/sb_bucket.c:        return svn_ra_serf__wrap_err(status, _("Failed to read the request"));
subversion/libsvn_ra_serf/util.c:      return svn_ra_serf__wrap_err(why, NULL);
subversion/libsvn_ra_serf/util.c:      return svn_ra_serf__wrap_err(status, _("Error running context"));
subversion/libsvn_ra_serf/util.c:    return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/util.c:                                  svn_ra_serf__wrap_err(status, NULL),
subversion/libsvn_ra_serf/multistatus.c:            return svn_ra_serf__wrap_err(result, NULL);
subversion/libsvn_ra_serf/update.c:        return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/update.c:          return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/update.c:                return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/update.c:        return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/update.c:            return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/update.c:            return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/update.c:    return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/ra_serf.h:svn_ra_serf__wrap_err(apr_status_t status,
subversion/libsvn_ra_serf/ra_serf.h:#define svn_ra_serf__wrap_err \
subversion/libsvn_ra_serf/ra_serf.h:  (svn_error__locate(__FILE__,__LINE__), (svn_ra_serf__wrap_err))
subversion/libsvn_ra_serf/commit.c:        return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/util_error.c:#undef svn_ra_serf__wrap_err
subversion/libsvn_ra_serf/util_error.c:svn_ra_serf__wrap_err(apr_status_t status,
subversion/libsvn_ra_serf/get_file.c:          return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/get_file.c:                  return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/get_file.c:          return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/serf.c:          return svn_ra_serf__wrap_err(
subversion/libsvn_ra_serf/serf.c:    return svn_ra_serf__wrap_err(status, NULL);
subversion/libsvn_ra_serf/serf.c:    return svn_ra_serf__wrap_err(status, NULL);

Is it ok to commit this in one step, or should this be a separate change?

[brainy]

You're right. This can wait until after this PR is merged, and can happen on trunk in one commit. I suspect that when the proposal for svn_ra_serf__wrap_err gets reviewed in detail, it'll turn out that other call sites could make good use of a child error parameter.

[minfrin]

serf_ssl_error_cb_t callback available in serf since r1926972.

[dsahlberg-apache-org]

(The CMake comment can be left alone, I'd like to take a look at this after merge...)

[dsahlberg-apache-org]

Formatting-nerd: Is there a tab vs space issue hiding here?

[dsahlberg-apache-org]

Reminder to self: Do we need to implement something similar for CMake ?