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 all commits
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
Collaborator

@dsahlberg-apache-org dsahlberg-apache-org 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 */
svn_error_t *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,23 @@ 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, apr_status_t status,
const char *message)
{
svn_ra_serf__connection_t *conn = baton;
svn_ra_serf__session_t *session = conn->session;

session->ssl_error = svn_error_createf(status,
session->ssl_error,
_("TLS: %s"),
message);

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 +584,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
Collaborator

@dsahlberg-apache-org dsahlberg-apache-org 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 +982,14 @@ 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 sess->ssl_error;
}
else
{
return svn_ra_serf__wrap_err(status, _("Error running context"));
}
Copy link
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
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
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