Hello Subversion Developers, We have identified and patched two stability issues in 'svnserve' that occur when running in threaded mode (--threads) under high concurrent load (e.g., polling from Jenkins CI). These crashes are triggered by abrupt client TCP disconnections (ECONNRESET) during the connection initialization or SASL authentication phase.
Environment:
- OS: Red Hat Enterprise Linux 8.10
- SVN Version: 1.14.1, 1.14.2, 1.15.0-rc4
- svnserve running with --threads -d
These issues appear to be the root cause behind SVN-4817 ("svnserve:
frequent segfaults in libapr"), which was previously closed as invalid due
to lack of mailing list discussion. We hope this analysis and patch provide
the necessary evidence to resolve it.
---
Bug 1: Segmentation Fault in cleanup_fs_access
Trigger: Client disconnects while construct_server_baton is calling
get_repos().
Cause: serve_interruptable catches the error and triggers pool destruction.
The cleanup handler 'cleanup_fs_access' attempts to call
svn_fs_set_access(baton->fs, NULL), but baton->fs is NULL or uninitialized
because the repo open was aborted.
Fix: Add a defensive NULL check for baton->fs in the cleanup handler.
Bug 2: Infinite Recursion / Stack Overflow in apr_pool_destroy
Trigger: Client disconnects during Cyrus SASL authentication setup.
Cause: The SASL cleanup handlers (sasl_dispose_cb and sasl_done_cb) are
registered across different pool lifecycles. On an aborted connection,
tearing down the partially initialized pool triggers a circular cleanup
dependency, leading to infinite recursion in libapr until the stack
overflows.
Fix: Add defensive checks in the SASL cleanup handlers to break the
circular dependency if the context is not fully initialized.
---
The following patch is against 1.14.2 but applies cleanly to 1.15.0-rc4 as
well.
[[[
* subversion/svnserve/serve.c
(cleanup_fs_access): Guard against NULL filesystem baton during aborted
connections.
* subversion/svnserve/cyrus_auth.c
(sasl_dispose_cb): Guard against NULL SASL context during aborted
connections.
* subversion/libsvn_ra_svn/cyrus_auth.c
(sasl_done_cb): Guard against uninitialized SASL pool to prevent cleanup
races.
]]]
--- subversion/svnserve/serve.c.orig
+++ subversion/svnserve/serve.c
@@ -604,6 +604,10 @@
svn_error_t *serr;
struct cleanup_fs_access_baton *baton = data;
+ /* If the connection was aborted before the FS was fully opened, do
nothing. */
+ if (!baton || !baton->fs)
+ return APR_SUCCESS;
+
serr = svn_fs_set_access(baton->fs, NULL);
if (serr)
{
--- subversion/svnserve/cyrus_auth.c.orig
+++ subversion/svnserve/cyrus_auth.c
@@ -249,6 +249,10 @@
static apr_status_t sasl_dispose_cb(void *data)
{
sasl_conn_t *sasl_ctx = (sasl_conn_t*) data;
+
+ if (!sasl_ctx)
+ return APR_SUCCESS;
+
svn_sasl__dispose(&sasl_ctx);
return APR_SUCCESS;
}
--- subversion/libsvn_ra_svn/cyrus_auth.c.orig
+++ subversion/libsvn_ra_svn/cyrus_auth.c
@@ -64,6 +64,9 @@
/* Pool cleanup called when sasl_pool is destroyed. */
static apr_status_t sasl_done_cb(void *data)
{
+ if (!sasl_pool)
+ return APR_SUCCESS;
+
/* Reset svn_ra_svn__sasl_status, in case the client calls
apr_initialize()/apr_terminate() more than once. */
svn_ra_svn__sasl_status = 0;
smime.p7s
Description: S/MIME Cryptographic Signature
