Skip to content

Report SSL error messages from serf #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
minfrin wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Report SSL error messages from serf #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
512a61b
Report SSL error messages from serf
minfrin Jul 1, 2025
675e294
Build a svn_error_t from SSL error messages.
minfrin Jul 1, 2025
bbe2fe3
Merge branch 'apache:trunk' into serf-sslsb
minfrin Jul 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions build/ac-macros/serf.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ AC_DEFUN(SVN_LIB_SERF,

svn_lib_serf=$serf_found

if test "$svn_lib_serf" = "yes"; then
save_ldflags="$LDFLAGS"
LDFLAGS="$LDFLAGS $SVN_SERF_LIBS"
AC_CHECK_FUNCS(serf_ssl_error_cb_set)
LDFLAGS="$save_ldflags"
fi

Copy link
Copy Markdown
Collaborator

@d-sahlberg d-sahlberg Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SVN_DOT_CLANGD([$SVN_SERF_INCLUDES])
AC_SUBST(SVN_SERF_INCLUDES)
AC_SUBST(SVN_SERF_LIBS)
Expand Down
3 changes: 3 additions & 0 deletions subversion/libsvn_ra_serf/ra_serf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ struct svn_ra_serf__session_t {
/* Are we using ssl */
svn_boolean_t using_ssl;

/* What was the underlying detail of the last SSL failure, if any */
const char *ssl_error;

/* Tristate flag that indicates if we should use compression for
network transmissions. If svn_tristate_true or svn_tristate_false,
the compression should be enabled and disabled, respectively.
Expand Down
35 changes: 33 additions & 2 deletions subversion/libsvn_ra_serf/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,20 @@ ssl_server_cert_cb(void *baton, int failures,
return save_error(session, err);
}

#if defined(HAVE_SERF_SSL_ERROR_CB_SET)
static apr_status_t
ssl_error_cb(void *baton,
const char *message)
{
svn_ra_serf__connection_t *conn = baton;
svn_ra_serf__session_t *session = conn->session;

session->ssl_error = apr_pstrdup(session->pool, message);
Copy link
Copy Markdown
Member

@brainy brainy Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@brainy brainy Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@brainy brainy Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@minfrin minfrin Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return APR_SUCCESS;
}
#endif

static svn_error_t *
load_authorities(svn_ra_serf__connection_t *conn, const char *authorities,
apr_pool_t *pool)
Expand Down Expand Up @@ -567,7 +581,14 @@ conn_setup(apr_socket_t *sock,
SERF_CONNECTION_FRAMING_TYPE_NONE);
}
#endif
}

#if defined(HAVE_SERF_SSL_ERROR_CB_SET)
serf_ssl_error_cb_set(conn->ssl_context,
ssl_error_cb,
conn);
#endif

}
Copy link
Copy Markdown
Collaborator

@d-sahlberg d-sahlberg Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (write_bkt)
{
Expand Down Expand Up @@ -958,7 +979,17 @@ svn_ra_serf__context_run(svn_ra_serf__session_t *sess,
_("Error running context"));
}

return svn_ra_serf__wrap_err(status, _("Error running context"));
if (sess->ssl_error)
{
return svn_error_createf(status,
svn_ra_serf__wrap_err(status, _("Error running context")),
_("TLS: %s"),
sess->ssl_error);
}
else
{
return svn_ra_serf__wrap_err(status, _("Error running context"));
}
Copy link
Copy Markdown
Member

@brainy brainy Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ...`.

Copy link
Copy Markdown
Contributor Author

@minfrin minfrin Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@brainy brainy Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return SVN_NO_ERROR;
Expand Down
Loading