On 2. 1. 26 17:29, Branko Čibej wrote:
On 2. 1. 26 17:07, Daniel Sahlberg wrote:
Den tors 1 jan. 2026 kl 19:53 skrev Branko Čibej<[email protected]>:
On 1. 1. 26 19:45,[email protected] wrote:
Author: brane
Date: Thu Jan 1 18:45:34 2026
New Revision: 1931047
Log:
Added error callback infrastructure.
Discuss...
More seriously: this is only the first step. It removes the
ssl-context-specific error callback and adds a layered tree of
callbacks
instead. For now, the SSL error will still go to the global level.
The next step is to revise the SSL functions that log errors and change
their callers to provide an error reporting context.
I hope the design is clear enough from the comments in serf.h,
serf_private.h and src/error_callbacks.c.
Suggestions for improvement welcome. If we decide that this is the
right
way forward, I'll continue with he SSL code update and add some
tests to
verify that the errors get propagated correctly.
-- Brane
Just asking so I understand this correctly.
In the previous implementation there was a callback (and baton)
associated
with the serf_ssl_context_t. Now there is a single static global
callback.
Which can be replaced by the user.
If I understand this correctly: Is there any chance/risk that an
application is creating multiple serf_ssl_context_t's where it would be
beneficial to set a separate callback (or more likely baton) for each
context?
This is an intermediate state. I intend to add context-specific
callbacks, but in a way that integrates as well as possible with the
"non-SSL" callbacks, with the global callback being a fallback when no
other context is available. For example, there was one case in the SSL
code where errors were simply silenced due to the lack of an available
callback context. This is no longer the case now.
The Subversion servers being down due to a DDoS, here's the patch I
intend to commit. It ties the SSL context with the error callbacks
defined on the context/connection, except in the one place where we just
don't have enough information to use anything but the global callback. I
added some example code to serf_get.c.
Next step: add tests to verify that this all works together as expected.
Then, somewhere along the line, we can start to actually use the
callbacks outside of the SSL code, as appropriate.
-- Brane
Index: buckets/ssl_buckets.c
===================================================================
--- buckets/ssl_buckets.c (revision 1931058)
+++ buckets/ssl_buckets.c (working copy)
@@ -223,6 +223,9 @@
void *protocol_userdata;
serf_config_t *config;
+
+ /* The error callback context. */
+ serf__ssl_error_ctx_t err_ctx;
};
/* The fallback SSL error context which logs to the global error callback. */
@@ -231,6 +234,27 @@
NULL /* baton */
};
+void serf_ssl_use_context_error_callback(serf_ssl_context_t *ssl_ctx,
+ serf_context_t *ctx)
+{
+ ssl_ctx->err_ctx.dispatch = serf__context_ssl_error;
+ ssl_ctx->err_ctx.baton = ctx;
+}
+
+void serf_ssl_use_connection_error_callback(serf_ssl_context_t *ssl_ctx,
+ serf_connection_t *conn)
+{
+ ssl_ctx->err_ctx.dispatch = serf__connection_ssl_error;
+ ssl_ctx->err_ctx.baton = conn;
+}
+
+void serf_ssl_use_incoming_error_callback(serf_ssl_context_t *ssl_ctx,
+ serf_incoming_t *client)
+{
+ ssl_ctx->err_ctx.dispatch = serf__incoming_ssl_error;
+ ssl_ctx->err_ctx.baton = client;
+}
+
static apr_status_t dispatch_ssl_error(const serf__ssl_error_ctx_t *err_ctx,
apr_status_t status,
const char *message)
@@ -1102,7 +1126,7 @@
ctx->fatal_err = SERF_ERROR_SSL_COMM_FAILED;
status = ctx->fatal_err;
- log_ssl_error(&global_error_ctx, status);
+ log_ssl_error(&ctx->err_ctx, status);
}
break;
@@ -1116,7 +1140,7 @@
default:
status = ctx->fatal_err = SERF_ERROR_SSL_COMM_FAILED;
- log_ssl_error(&global_error_ctx, status);
+ log_ssl_error(&ctx->err_ctx, status);
break;
}
@@ -1223,7 +1247,7 @@
} else {
/* A fatal error occurred. */
ctx->fatal_err = status = SERF_ERROR_SSL_COMM_FAILED;
- log_ssl_error(&global_error_ctx, status);
+ log_ssl_error(&ctx->err_ctx, status);
}
} else {
*len = ssl_len;
@@ -1656,9 +1680,9 @@
char ebuf[1024];
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "could not open URI: %s", cert_uri);
- dispatch_ssl_error(&global_error_ctx, ctx->fatal_err, ebuf);
+ dispatch_ssl_error(&ctx->err_ctx, ctx->fatal_err, ebuf);
- log_ssl_error(&global_error_ctx, ctx->fatal_err);
+ log_ssl_error(&ctx->err_ctx, ctx->fatal_err);
UI_destroy_method(ui_method);
return 0;
}
@@ -1688,7 +1712,7 @@
char ebuf[1024];
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "could not read URI: %s",
cert_uri);
- dispatch_ssl_error(&global_error_ctx, ctx->fatal_err, ebuf);
+ dispatch_ssl_error(&ctx->err_ctx, ctx->fatal_err, ebuf);
store_error = 1;
break;
}
@@ -1739,7 +1763,7 @@
OSSL_STORE_close(store);
if (store_error || ERR_peek_error()) {
- log_ssl_error(&global_error_ctx, ctx->fatal_err);
+ log_ssl_error(&ctx->err_ctx, ctx->fatal_err);
result = 0;
goto cleanup;
}
@@ -1801,7 +1825,7 @@
}
if (ERR_peek_error()) {
- log_ssl_error(&global_error_ctx, ctx->fatal_err);
+ log_ssl_error(&ctx->err_ctx, ctx->fatal_err);
result = -1;
goto cleanup;
}
@@ -1902,9 +1926,9 @@
if (status) {
char ebuf[1024];
apr_snprintf(ebuf, sizeof(ebuf), "could not open PKCS12: %s",
cert_path);
- dispatch_ssl_error(&global_error_ctx, status, ebuf);
+ dispatch_ssl_error(&ctx->err_ctx, status, ebuf);
apr_strerror(status, ebuf, sizeof(ebuf));
- dispatch_ssl_error(&global_error_ctx, status, ebuf);
+ dispatch_ssl_error(&ctx->err_ctx, status, ebuf);
ctx->fatal_err = status;
return -1;
@@ -1989,9 +2013,9 @@
else {
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "could not parse
PKCS12: %s", cert_path);
- dispatch_ssl_error(&global_error_ctx,
ctx->fatal_err, ebuf);
+ dispatch_ssl_error(&ctx->err_ctx, ctx->fatal_err,
ebuf);
- log_ssl_error(&global_error_ctx, ctx->fatal_err);
+ log_ssl_error(&ctx->err_ctx, ctx->fatal_err);
return -1;
}
}
@@ -2001,9 +2025,9 @@
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
apr_snprintf(ebuf, sizeof(ebuf), "PKCS12 needs a password:
%s", cert_path);
- dispatch_ssl_error(&global_error_ctx, ctx->fatal_err, ebuf);
+ dispatch_ssl_error(&ctx->err_ctx, ctx->fatal_err, ebuf);
- log_ssl_error(&global_error_ctx, ctx->fatal_err);
+ log_ssl_error(&ctx->err_ctx, ctx->fatal_err);
return -1;
}
else {
@@ -2011,9 +2035,9 @@
bio_meth_free(biom);
ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
- dispatch_ssl_error(&global_error_ctx, ctx->fatal_err, ebuf);
+ dispatch_ssl_error(&ctx->err_ctx, ctx->fatal_err, ebuf);
- log_ssl_error(&global_error_ctx, ctx->fatal_err);
+ log_ssl_error(&ctx->err_ctx, ctx->fatal_err);
return -1;
}
}
@@ -2198,6 +2222,8 @@
ssl_ctx->handshake_done = FALSE;
ssl_ctx->hit_eof = FALSE;
+ ssl_ctx->err_ctx = global_error_ctx;
+
return ssl_ctx;
}
@@ -2402,6 +2428,8 @@
return APR_SUCCESS;
}
+ /* We don't have an ssl_context_t here, so log the errors to the global
+ callback. It's better than ignoring them. */
status = SERF_ERROR_SSL_CERT_FAILED;
log_ssl_error(&global_error_ctx, status);
return status;
@@ -2466,7 +2494,7 @@
result = X509_STORE_add_crl(store, crl);
if (!result) {
ssl_ctx->fatal_err = status = SERF_ERROR_SSL_CERT_FAILED;
- log_ssl_error(&global_error_ctx, status);
+ log_ssl_error(&ssl_ctx->err_ctx, status);
return status;
}
Index: serf.h
===================================================================
--- serf.h (revision 1931058)
+++ serf.h (working copy)
@@ -262,6 +262,8 @@
* context @a ctx and, since contexts may not be accessed from multiple
* threads, serialization is not a concern.
*
+ * The lifetime of @a baton must be longer than the lifetime of the context.
+ *
* @since New in 1.5.
*/
void serf_context_error_callback_set(
@@ -274,6 +276,8 @@
*
* Like serf_context_error_callback_set() but for connections.
*
+ * The lifetime of @a baton must be longer than the lifetime of the connection.
+ *
* @since New in 1.5.
*/
void serf_connection_error_callback_set(
@@ -286,6 +290,9 @@
*
* Like serf_context_error_callback_set() but for incoming connections.
*
+ * The lifetime of @a baton must be longer than the lifetime of the
+ * incoming connection.
+ *
* @since New in 1.5.
*/
void serf_incoming_error_callback_set(
Index: serf_bucket_types.h
===================================================================
--- serf_bucket_types.h (revision 1931058)
+++ serf_bucket_types.h (working copy)
@@ -585,9 +585,6 @@
#define SERF_SSL_SIGNATURE_FAILURE 0x0800
-extern const serf_bucket_type_t serf_bucket_type_ssl_encrypt;
-#define SERF_BUCKET_IS_SSL_ENCRYPT(b) SERF_BUCKET_CHECK((b), ssl_encrypt)
-
typedef struct serf_ssl_context_t serf_ssl_context_t;
typedef struct serf_ssl_certificate_t serf_ssl_certificate_t;
@@ -846,14 +843,6 @@
serf_ssl_context_t *ssl_ctx,
int enabled);
-serf_bucket_t *serf_bucket_ssl_encrypt_create(
- serf_bucket_t *stream,
- serf_ssl_context_t *ssl_context,
- serf_bucket_alloc_t *allocator);
-
-serf_ssl_context_t *serf_bucket_ssl_encrypt_context_get(
- serf_bucket_t *bucket);
-
/* ==================================================================== */
/**
@@ -1009,18 +998,86 @@
/* ==================================================================== */
+extern const serf_bucket_type_t serf_bucket_type_ssl_encrypt;
+#define SERF_BUCKET_IS_SSL_ENCRYPT(b) SERF_BUCKET_CHECK((b), ssl_encrypt)
+
+/**
+ * Create an SSL encryption bucket that wrapt @a stream.
+ *
+ * If @a ssl_context is not provided, a new one will be created.
+ */
+serf_bucket_t *serf_bucket_ssl_encrypt_create(
+ serf_bucket_t *stream,
+ serf_ssl_context_t *ssl_context,
+ serf_bucket_alloc_t *allocator);
+
+/**
+ * Return the SSL context from an encryption bucket.
+ */
+serf_ssl_context_t *serf_bucket_ssl_encrypt_context_get(
+ serf_bucket_t *bucket);
+
extern const serf_bucket_type_t serf_bucket_type_ssl_decrypt;
#define SERF_BUCKET_IS_SSL_DECRYPT(b) SERF_BUCKET_CHECK((b), ssl_decrypt)
+/**
+ * Create an SSL encryption bucket that wrapt @a stream.
+ *
+ * If @a ssl_context is not provided, a new one will be created.
+ */
serf_bucket_t *serf_bucket_ssl_decrypt_create(
serf_bucket_t *stream,
serf_ssl_context_t *ssl_context,
serf_bucket_alloc_t *allocator);
+/**
+ * Return the SSL context from an decryption bucket.
+ */
serf_ssl_context_t *serf_bucket_ssl_decrypt_context_get(
serf_bucket_t *bucket);
+/**
+ * Configure the SSL context to use the error cllback set on @a ctx.
+ *
+ * By default, new SSL contexts send error messages to the global
+ * error callback.
+ *
+ * @see serf_global_error_callback_set()
+ * @see serf_context_error_callback_set()
+ *
+ * @since New in 1.5.
+ */
+void serf_ssl_use_context_error_callback(serf_ssl_context_t *ssl_ctx,
+ serf_context_t *ctx);
+/**
+ * Configure the SSL context to use the error callback set on @a conn.
+ *
+ * By default, new SSL contexts send error messages to the global
+ * error callback.
+ *
+ * @see serf_global_error_callback_set()
+ * @see serf_connection_error_callback_set()
+ *
+ * @since New in 1.5.
+ */
+void serf_ssl_use_connection_error_callback(serf_ssl_context_t *ssl_ctx,
+ serf_connection_t *conn);
+
+/**
+ * Configure the SSL context to use the error callback set on @a client.
+ *
+ * By default, new SSL contexts send error messages to the global
+ * error callback.
+ *
+ * @see serf_global_error_callback_set()
+ * @see serf_incoming_error_callback_set()
+ *
+ * @since New in 1.5.
+ */
+void serf_ssl_use_incoming_error_callback(serf_ssl_context_t *ssl_ctx,
+ serf_incoming_t *client);
+
/* ==================================================================== */
Index: serf_private.h
===================================================================
--- serf_private.h (revision 1931058)
+++ serf_private.h (working copy)
@@ -178,21 +178,9 @@
apr_status_t serf__connection_ssl_error(const void *baton,
apr_status_t status,
const char *message);
-apr_status_t serf__request_ssl_error(const void *baton,
- apr_status_t status,
- const char *message);
-apr_status_t serf__response_ssl_error(const void *baton,
- apr_status_t status,
- const char *message);
apr_status_t serf__incoming_ssl_error(const void *baton,
apr_status_t status,
const char *message);
-apr_status_t serf__incoming_request_ssl_error(const void *baton,
- apr_status_t status,
- const char *message);
-apr_status_t serf__incoming_response_ssl_error(const void *baton,
- apr_status_t status,
- const char *message);
/*** Logging facilities ***/
Index: src/error_callbacks.c
===================================================================
--- src/error_callbacks.c (revision 1931058)
+++ src/error_callbacks.c (working copy)
@@ -208,28 +208,6 @@
conn, status, message);
}
-apr_status_t serf__request_ssl_error(const void *baton,
- apr_status_t status,
- const char *message)
-{
- const serf_request_t *const req = baton;
- return process_connection_error(SERF_ERROR_CB_SSL_CONTEXT
- | SERF_ERROR_CB_OUTGOING
- | SERF_ERROR_CB_REQUEST,
- req->conn, status, message);
-}
-
-apr_status_t serf__response_ssl_error(const void *baton,
- apr_status_t status,
- const char *message)
-{
- const serf_request_t *const req = baton;
- return process_connection_error(SERF_ERROR_CB_SSL_CONTEXT
- | SERF_ERROR_CB_OUTGOING
- | SERF_ERROR_CB_RESPONSE,
- req->conn, status, message);
-}
-
apr_status_t serf__incoming_ssl_error(const void *baton,
apr_status_t status,
const char *message)
@@ -239,25 +217,3 @@
| SERF_ERROR_CB_INCOMING,
client, status, message);
}
-
-apr_status_t serf__incoming_request_ssl_error(const void *baton,
- apr_status_t status,
- const char *message)
-{
- const serf_incoming_request_t *const req = baton;
- return process_incoming_error(SERF_ERROR_CB_SSL_CONTEXT
- | SERF_ERROR_CB_INCOMING
- | SERF_ERROR_CB_REQUEST,
- req->incoming, status, message);
-}
-
-apr_status_t serf__incoming_response_ssl_error(const void *baton,
- apr_status_t status,
- const char *message)
-{
- const serf_incoming_request_t *const req = baton;
- return process_incoming_error(SERF_ERROR_CB_SSL_CONTEXT
- | SERF_ERROR_CB_INCOMING
- | SERF_ERROR_CB_RESPONSE,
- req->incoming, status, message);
-}
Index: test/serf_get.c
===================================================================
--- test/serf_get.c (revision 1931058)
+++ test/serf_get.c (working copy)
@@ -29,6 +29,7 @@
#include <apr_version.h>
#include "serf.h"
+#include "serf_bucket_types.h"
/* Add Connection: close header to each request. */
/* #define CONNECTION_CLOSE_HDR */
@@ -230,6 +231,7 @@
c = serf_bucket_ssl_decrypt_create(c, conn_ctx->ssl_ctx,
ctx->bkt_alloc);
if (!conn_ctx->ssl_ctx) {
conn_ctx->ssl_ctx = serf_bucket_ssl_decrypt_context_get(c);
+ serf_ssl_use_connection_error_callback(conn_ctx->ssl_ctx,
conn_ctx->conn);
}
serf_ssl_server_cert_chain_callback_set(conn_ctx->ssl_ctx,
ignore_all_cert_errors,
@@ -509,6 +511,32 @@
}
}
+static apr_status_t
+global_error_callback(void *baton,
+ unsigned source,
+ apr_status_t status,
+ const char *message)
+{
+ fprintf(stderr, "ERROR%s <%d> %s\n",
+ ((source & SERF_ERROR_CB_SSL_CONTEXT) ? " (SSL)" : ""),
+ status, message);
+ return APR_SUCCESS;
+}
+
+static apr_status_t
+connection_error_callback(void *baton,
+ unsigned source,
+ apr_status_t status,
+ const char *message)
+{
+ const char *const conn_id = baton;
+ fprintf(stderr, "ERROR conn[%s]%s <%d> %s\n",
+ conn_id,
+ ((source & SERF_ERROR_CB_SSL_CONTEXT) ? " (SSL)" : ""),
+ status, message);
+ return APR_SUCCESS;
+}
+
/* Value for 'no short code' should be > 255 */
#define CERTFILE 256
#define CERTPWD 257
@@ -817,7 +845,7 @@
serf_config_credentials_callback(context, credentials_callback);
- /* Setup debug logging */
+ /* Setup debug logging and error callbacks*/
if (debug)
{
serf_log_output_t *output;
@@ -832,6 +860,8 @@
if (!status)
serf_logging_add_output(context, output);
+
+ serf_global_error_callback_set(global_error_callback, NULL);
}
/* ### Connection or Context should have an allocator? */
@@ -857,6 +887,12 @@
conn_ctx->conn = connections[i];
serf_connection_set_max_outstanding_requests(connections[i], inflight);
+ if (debug)
+ {
+ serf_connection_error_callback_set(connections[i],
+ connection_error_callback,
+ apr_psprintf(pool, "%02d", i));
+ }
}
handler_ctx.completed_requests = 0;
