commit f59d992032e2e2b698790e560d9518a0f280c728
parent 66635b91012b8430e9b06da33446b7b63478a7c2
Author: Christian Grothoff <christian@grothoff.org>
Date: Thu, 6 Aug 2026 19:19:56 +0200
misc robustness fixes
Diffstat:
4 files changed, 399 insertions(+), 22 deletions(-)
diff --git a/src/backend/paivana-httpd.c b/src/backend/paivana-httpd.c
@@ -402,6 +402,23 @@ run (void *cls,
GNUNET_SCHEDULER_shutdown ();
return;
}
+ /* Deliberately *not* run through strip_trailing_slashes(): a Taler
+ merchant API base URL is joined with relative paths, so it wants
+ the '/' the other base URLs above shed. TALER_url_join() insists
+ on it and returns NULL without it, which surfaces much later as
+ an assertion failure on the handle built from the joined URL --
+ so check for it here, where we can still name the option that is
+ wrong. */
+ if ('/' != PH_merchant_base_url[strlen (PH_merchant_base_url) - 1])
+ {
+ GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
+ "paivana",
+ "MERCHANT_BACKEND_URL",
+ "must end with a '/'");
+ PH_global_ret = EXIT_NOTCONFIGURED;
+ GNUNET_SCHEDULER_shutdown ();
+ return;
+ }
}
{
char *merchant_unix_path;
diff --git a/src/backend/paivana-httpd_reverse.c b/src/backend/paivana-httpd_reverse.c
@@ -49,6 +49,14 @@
__LINE__, \
curl_easy_strerror (rc))
+/**
+ * Value of `upstream_content_length` when the upstream declared no
+ * usable `Content-Length` for its response. Not a length any message
+ * can have: RFC 9110 §8.6 makes the field a count of octets, and we
+ * never buffer more than #GNUNET_MAX_MALLOC_CHECKED of them.
+ */
+#define PH_NO_CONTENT_LENGTH UINT64_MAX
+
/**
* State machine for HTTP requests (per request). MHD invokes the
@@ -251,6 +259,16 @@ struct HttpRequest
unsigned int response_code;
/**
+ * `Content-Length` the upstream declared for its response, or
+ * #PH_NO_CONTENT_LENGTH if it declared none we could parse.
+ * `curl_check_hdr()` drops the header itself — the length we
+ * forward has to describe the buffer we actually assembled — but
+ * the declared value is kept here because it is the only evidence
+ * that the buffer is short; see `curl_download_cb()`.
+ */
+ uint64_t upstream_content_length;
+
+ /**
* Request processing state machine.
*/
enum RequestState state;
@@ -261,6 +279,27 @@ struct HttpRequest
enum GNUNET_GenericReturnValue suspended;
/**
+ * Is @e response ours to destroy?
+ *
+ * False for the four shared error responses, which outlive any one
+ * request. This used to be inferred by comparing @e response
+ * against those four statics, which only worked while they were
+ * non-NULL: #PAIVANA_HTTPD_reverse_shutdown() clears them while MHD
+ * may still hold queued references, and from then on every
+ * comparison matched nothing and cleanup destroyed a response it
+ * never owned.
+ */
+ bool own_response;
+
+ /**
+ * Was the client's method HEAD? A response to HEAD never carries a
+ * body, however large a `Content-Length` it declares (RFC 9110
+ * §9.3.2), so the truncation check in `curl_download_cb()` must not
+ * be applied to it.
+ */
+ bool head_request;
+
+ /**
* Concatenated value of the client's Via header(s), if any. Per
* RFC 9110 §7.6.3 a proxy must *append* its own entry to this
* list, not replace it; we capture the inbound value here before
@@ -462,6 +501,14 @@ PAIVANA_HTTPD_reverse_shutdown (void)
MHD_resume_connection (hr->con);
}
}
+ /* Dropping *our* reference to the shared responses here is safe even
+ though MHD is stopped only afterwards: a queued response is
+ reference-counted (MHD_queue_response() takes its own reference),
+ so what follows merely relinquishes ownership. What must not
+ happen is a second release from
+ #PAIVANA_HTTPD_reverse_cleanup() — hence the explicit
+ `own_response` rather than a pointer comparison against the
+ statics we are about to NULL out. */
if (NULL != curl_failure_response)
{
MHD_destroy_response (curl_failure_response);
@@ -733,10 +780,24 @@ curl_check_hdr (void *buffer,
hdr_type,
hdr_val);
/* Skip "Content-length:" header as it will be wrong, given
- that we are man-in-the-middling the connection */
+ that we are man-in-the-middling the connection. Remember the
+ value first: it is what lets `curl_download_cb()' tell a complete
+ body from one libgnunetcurl stopped buffering (RFC 9110 §8.6 —
+ the field states the length of the body that follows). */
if (0 == strcasecmp (hdr_type,
MHD_HTTP_HEADER_CONTENT_LENGTH))
{
+ char *endptr;
+ unsigned long long cl;
+
+ errno = 0;
+ cl = strtoull (hdr_val,
+ &endptr,
+ 10);
+ if ( (0 == errno) &&
+ (endptr != hdr_val) &&
+ ('\0' == *endptr) )
+ hr->upstream_content_length = (uint64_t) cl;
GNUNET_free (ndup);
return bytes;
}
@@ -775,6 +836,26 @@ curl_check_hdr (void *buffer,
/**
+ * Can a response with status @a code carry a body at all?
+ *
+ * RFC 9110 §6.4.1: 1xx, 204 and 304 responses never do. §8.6 even
+ * allows a 304 to state the `Content-Length` the full representation
+ * would have, which is precisely a declared length that no body
+ * follows — so these have to be excluded before comparing the two.
+ *
+ * @param code HTTP status code the upstream returned
+ * @return true if a response body is to be expected
+ */
+static bool
+status_allows_body (unsigned int code)
+{
+ return (code >= MHD_HTTP_OK) &&
+ (MHD_HTTP_NO_CONTENT != code) &&
+ (MHD_HTTP_NOT_MODIFIED != code);
+}
+
+
+/**
* Handle response payload data from cURL.
*
* @param cls our `struct HttpRequest *`
@@ -804,6 +885,54 @@ curl_download_cb (void *cls,
hr->state = REQUEST_STATE_PROXY_DOWNLOAD_FAILED;
return;
}
+ /* libgnunetcurl stops buffering at #GNUNET_MAX_MALLOC_CHECKED (40
+ MiB) by returning 0 to libcurl, which fails the transfer — but
+ CURLINFO_RESPONSE_CODE still holds the status line libcurl parsed
+ long before, and GNUNET_CURL_perform2() hands us that code
+ together with the partial buffer. Nothing else here distinguishes
+ a truncated body from a complete one, and MHD would go on to
+ compute a perfectly consistent Content-Length for the fragment:
+ the client would receive a corrupt representation under the
+ upstream's 2xx with no signal at all. RFC 9112 §8.1.2 requires an
+ incomplete message to be treated as a failure, so fail it. The
+ same comparison catches an upstream whose declared length and body
+ simply disagree, which is the same framing error. */
+ if ( (PH_NO_CONTENT_LENGTH != hr->upstream_content_length) &&
+ (! hr->head_request) &&
+ (status_allows_body ((unsigned int) response_code)) &&
+ (hr->upstream_content_length != (uint64_t) body_size) )
+ {
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "Upstream declared %llu bytes for `%s' but delivered %llu;"
+ " failing the request rather than serving a partial body\n",
+ (unsigned long long) hr->upstream_content_length,
+ hr->url,
+ (unsigned long long) body_size);
+ hr->state = REQUEST_STATE_PROXY_DOWNLOAD_FAILED;
+ return;
+ }
+ /* A chunked upstream states no length at all, so the comparison
+ above cannot see the ceiling being reached. What is still visible
+ is how close we came to it: libgnunetcurl refuses the write that
+ *would* reach #GNUNET_MAX_MALLOC_CHECKED, and libcurl hands it at
+ most CURL_MAX_WRITE_SIZE per call, so every truncated body ends up
+ within that much of the ceiling. Bodies that merely happen to
+ fall in the same narrow band are refused along with them: at 16 kb
+ below a 40 MiB cap that is a fair price for not serving a
+ silently-cut representation. */
+ if ( ((uint64_t) body_size) + CURL_MAX_WRITE_SIZE >=
+ (uint64_t) GNUNET_MAX_MALLOC_CHECKED)
+ {
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "Response for `%s' reached the %llu byte buffering limit"
+ " at %llu bytes; failing the request rather than serving"
+ " a partial body\n",
+ hr->url,
+ (unsigned long long) GNUNET_MAX_MALLOC_CHECKED,
+ (unsigned long long) body_size);
+ hr->state = REQUEST_STATE_PROXY_DOWNLOAD_FAILED;
+ return;
+ }
hr->response = MHD_create_response_from_buffer_copy (body_size,
body);
if (NULL == hr->response)
@@ -814,6 +943,7 @@ curl_download_cb (void *cls,
hr->state = REQUEST_STATE_PROXY_DOWNLOAD_FAILED;
return;
}
+ hr->own_response = true;
hr->response_code = response_code;
/* RFC 9110 §7.6.1: drop the headers named by the upstream's
Connection list. Deferred to here (rather than done in
@@ -1185,6 +1315,7 @@ PAIVANA_HTTPD_reverse_create (struct MHD_Connection *connection,
hr = GNUNET_new (struct HttpRequest);
hr->state = REQUEST_STATE_HEADERS_PENDING;
+ hr->upstream_content_length = PH_NO_CONTENT_LENGTH;
hr->con = connection;
hr->url = GNUNET_strdup (url);
GNUNET_CONTAINER_DLL_insert (hr_head,
@@ -1218,11 +1349,10 @@ PAIVANA_HTTPD_reverse_cleanup (struct HttpRequest *hr)
hr->headers = NULL;
}
if ( (NULL != hr->response) &&
- (curl_failure_response != hr->response) &&
- (method_failure_response != hr->response) &&
- (upload_failure_response != hr->response) &&
- (internal_failure_response != hr->response) )
- /* Destroy non-error responses... (?) */
+ (hr->own_response) )
+ /* The shared error responses belong to this module, not to the
+ request that queued one; releasing them here would take a
+ reference we never held. */
MHD_destroy_response (hr->response);
for (header = hr->header_head;
@@ -1405,6 +1535,9 @@ configure_curl_method (struct HttpRequest *hr,
MHD_HTTP_METHOD_HEAD))
{
hr->state = REQUEST_STATE_PROXY_DOWNLOAD_STARTED;
+ /* The upstream still states the length the equivalent GET would
+ have had; nothing follows it (RFC 9110 §9.3.2). */
+ hr->head_request = true;
curl_easy_setopt (hr->curl,
CURLOPT_NOBODY,
1L);
diff --git a/src/backend/paivana-httpd_templates.c b/src/backend/paivana-httpd_templates.c
@@ -54,6 +54,25 @@ struct Template;
*/
#define MAX_RESPONSE_CACHE_ENTRIES 128
+/**
+ * How long we give the merchant backend to answer our template
+ * queries before abandoning the startup sequence.
+ *
+ * Nothing is served until they are in: #PAIVANA_HTTPD_serve_requests()
+ * — which binds the listen sockets — is only reached from the last of
+ * these callbacks, and neither `TALER_MERCHANT_curl_easy_get_()' nor
+ * anything above it arms CURLOPT_TIMEOUT. A backend that accepts the
+ * TCP connection and then never answers would otherwise leave paivana
+ * neither serving nor exiting, with no log line after the startup
+ * banner; under `SERVE = systemd' the listening socket already
+ * exists, so clients connect successfully and then hang forever with
+ * nothing accepting them. Generous enough for the round-trips a load
+ * takes (GET /private/templates, then one GET per template, issued in
+ * parallel).
+ */
+#define TEMPLATE_LOAD_TIMEOUT \
+ GNUNET_TIME_relative_multiply (GNUNET_TIME_UNIT_MINUTES, 2)
+
/**
* Entry in the cache of responses for a given template.
@@ -178,6 +197,56 @@ static struct Template *t_tail;
*/
static struct TALER_MERCHANT_GetPrivateTemplatesHandle *gpt;
+/**
+ * Watchdog for #TEMPLATE_LOAD_TIMEOUT, NULL once the templates are in
+ * (or once we have given up on them).
+ */
+static struct GNUNET_SCHEDULER_Task *load_timeout_task;
+
+
+/**
+ * Task run when the merchant backend did not answer our template
+ * queries within #TEMPLATE_LOAD_TIMEOUT.
+ *
+ * Treated exactly like any other failure to load the templates (an
+ * unauthorized or unexpected status from the backend): the daemon
+ * exits with a diagnosis instead of stalling. Whether that policy is
+ * the right one is a separate question — this task only makes sure the
+ * stall is not a third, silent outcome.
+ *
+ * @param cls NULL
+ */
+static void
+load_timeout (void *cls)
+{
+ (void) cls;
+ load_timeout_task = NULL;
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "Merchant backend at `%s' did not answer our template queries"
+ " within %s; giving up instead of never starting to serve\n",
+ PH_merchant_base_url,
+ GNUNET_STRINGS_relative_time_to_string (TEMPLATE_LOAD_TIMEOUT,
+ true));
+ PH_global_ret = EXIT_FAILURE;
+ GNUNET_SCHEDULER_shutdown ();
+}
+
+
+/**
+ * The templates are in (or there are none): stop the watchdog and
+ * open the listen sockets.
+ */
+static void
+templates_ready (void)
+{
+ if (NULL != load_timeout_task)
+ {
+ GNUNET_SCHEDULER_cancel (load_timeout_task);
+ load_timeout_task = NULL;
+ }
+ PAIVANA_HTTPD_serve_requests ();
+}
+
/**
* Check if two strings are equal, including both being NULL
@@ -347,13 +416,31 @@ load_paywall (struct MHD_Connection *conn,
NULL /* no Taler URI (needs dynamic paivana_id!) */,
data,
&reply);
+ json_decref (data);
+ if (GNUNET_NO == ret)
+ {
+ enum MHD_Result mret;
+
+ /* taler_templating_lib.h: #GNUNET_NO means an *error reply* was
+ built — typically because the paywall template is not
+ installed where TALER_TEMPLATING_init() looked. It is a live
+ MHD_Response and it is ours: returning #MHD_YES without
+ queuing it leaked it once per unauthenticated request and left
+ the client waiting for a status line that never came. */
+ GNUNET_break (0);
+ mret = MHD_queue_response (conn,
+ http_status,
+ reply);
+ MHD_destroy_response (reply);
+ return mret;
+ }
if (GNUNET_OK != ret)
{
+ /* #GNUNET_SYSERR: no reply was built, so there is nothing to
+ queue and nothing to free; MHD must close the connection. */
GNUNET_break (0);
- json_decref (data);
- return (GNUNET_NO == ret) ? MHD_YES : MHD_NO;
+ return MHD_NO;
}
- json_decref (data);
}
@@ -476,6 +563,30 @@ parse_template (struct Template *t,
en);
return false;
}
+ /* GNUNET_JSON_spec_array_const() only establishes that "choices" is
+ an array; what is *in* it is whatever the backend sent.
+ load_paywall() hands element 0 to
+ GNUNET_JSON_pack_object_incref(), which aborts on anything that is
+ not an object — and it does so from an unauthenticated GET of the
+ paywall page, so a backend that disagrees with us about the schema
+ would turn every visitor into a crash loop. We cross a network
+ trust boundary here and must not assert on what comes back. */
+ {
+ size_t idx;
+ json_t *choice;
+
+ json_array_foreach ((json_t *) choices, idx, choice)
+ {
+ if (json_is_object (choice))
+ continue;
+ GNUNET_break_op (0);
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Template %s has a non-object choice at index %u\n",
+ t->template_id,
+ (unsigned int) idx);
+ return false;
+ }
+ }
if ( (NULL != regex) &&
('\0' != regex[0]) )
{
@@ -560,7 +671,7 @@ setup_template (
/* all templates done, continue with main logic */
GNUNET_log (GNUNET_ERROR_TYPE_INFO,
"Templates loaded, starting to serve requests\n");
- PAIVANA_HTTPD_serve_requests ();
+ templates_ready ();
}
@@ -583,7 +694,7 @@ check_templates (
case MHD_HTTP_NO_CONTENT:
GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
"No templates found, starting to serve requests\n");
- PAIVANA_HTTPD_serve_requests ();
+ templates_ready ();
return;
case MHD_HTTP_UNAUTHORIZED:
GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
@@ -605,7 +716,7 @@ check_templates (
{
GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
"No templates found, starting to serve requests\n");
- PAIVANA_HTTPD_serve_requests ();
+ templates_ready ();
return;
}
@@ -636,6 +747,11 @@ check_templates (
void
PAIVANA_HTTPD_load_templates ()
{
+ GNUNET_assert (NULL == load_timeout_task);
+ load_timeout_task
+ = GNUNET_SCHEDULER_add_delayed (TEMPLATE_LOAD_TIMEOUT,
+ &load_timeout,
+ NULL);
gpt = TALER_MERCHANT_get_private_templates_create (PH_merchant_ctx,
PH_merchant_base_url);
GNUNET_assert (NULL != gpt);
@@ -679,17 +795,49 @@ PAIVANA_HTTPD_search_templates (struct MHD_Connection *connection,
char *enc = NULL;
char *url;
- if ( (NULL != t->regex) &&
- (0 != regexec (&t->ex,
- website,
- 0, NULL,
- 0)) )
+ if (NULL != t->regex)
{
- GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
- "Request for %s did not match template %s\n",
- website,
- t->template_id);
- continue;
+ int rc;
+
+ rc = regexec (&t->ex,
+ website,
+ 0, NULL,
+ 0);
+ if (REG_NOMATCH == rc)
+ {
+ GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
+ "Request for %s did not match template %s\n",
+ website,
+ t->template_id);
+ continue;
+ }
+ if (0 != rc)
+ {
+ char errbuf[128];
+
+ /* Not "did not match": regexec(3) also reports REG_ESPACE,
+ whose likelihood is a function of the merchant's pattern and
+ the client's URL. Taking the `continue' would drop this
+ template from consideration, and if it were the last one the
+ caller reads #GNUNET_SYSERR as "no paywall applies" and
+ serves the page for free. Fail closed instead. */
+ GNUNET_break (0);
+ (void) regerror (rc,
+ &t->ex,
+ errbuf,
+ sizeof (errbuf));
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "Failed to match template %s against %s: %s\n",
+ t->template_id,
+ website,
+ errbuf);
+ ret = TALER_MHD_reply_with_error (
+ connection,
+ MHD_HTTP_INTERNAL_SERVER_ERROR,
+ TALER_EC_GENERIC_INTERNAL_INVARIANT_FAILURE,
+ errbuf);
+ return (MHD_YES == ret) ? GNUNET_OK : GNUNET_NO;
+ }
}
if (! PAIVANA_HTTPD_get_base_url (connection,
@@ -778,6 +926,11 @@ PAIVANA_HTTPD_return_template (struct MHD_Connection *connection,
void
PAIVANA_HTTPD_unload_templates ()
{
+ if (NULL != load_timeout_task)
+ {
+ GNUNET_SCHEDULER_cancel (load_timeout_task);
+ load_timeout_task = NULL;
+ }
while (NULL != t_head)
{
struct Template *t = t_head;
diff --git a/src/tests/test_reverse_proxy.sh b/src/tests/test_reverse_proxy.sh
@@ -82,6 +82,7 @@ PY_PORT=18403
RS_PORT=18404
EARLY_PORT=18405
NODRAIN_PORT=18406
+TRUNC_PORT=18407
DEAD_PORT=18499 # nothing should be listening here
TMPDIR="$(mktemp -d -t paivana-tests.XXXXXX)"
@@ -796,6 +797,76 @@ function test_early_response_no_drain() {
}
+function start_truncating_upstream() {
+ # An upstream that declares more body than it delivers and then
+ # closes: the wire shape of a response that arrived incomplete.
+ # It is also exactly what paivana ends up holding when
+ # libgnunetcurl stops buffering at its 40 MiB ceiling -- libcurl
+ # fails the transfer, but the status line it parsed long before is
+ # still what CURLINFO_RESPONSE_CODE reports.
+ local port="$1"
+ cat >"$TMPDIR/truncating_upstream.py" <<'PYEOF'
+import socket
+import sys
+
+port = int(sys.argv[1])
+srv = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+srv.bind(("127.0.0.1", port))
+srv.listen(5)
+while True:
+ conn, _ = srv.accept()
+ try:
+ conn.recv(65536)
+ conn.sendall(b"HTTP/1.1 200 OK\r\n"
+ b"Content-Type: text/plain\r\n"
+ b"Content-Length: 100000\r\n"
+ b"\r\n"
+ + b"x" * 1000)
+ except OSError:
+ pass
+ conn.close()
+PYEOF
+ local log="$LOGDIR/truncating.log"
+ ( exec python3 "$TMPDIR/truncating_upstream.py" "$port" ) >"$log" 2>&1 &
+ PIDS+=("$!")
+ if ! wait_for_port 127.0.0.1 "$port";
+ then
+ echo "FAIL: truncating upstream did not start on port $port" >&2
+ tail -n 20 "$log" >&2
+ exit 1
+ fi
+}
+
+function test_short_body() {
+ # An incomplete message must be treated as a failure (RFC 9112
+ # section 8.1.2). Without that, paivana served the fragment under
+ # the upstream's 200 with a Content-Length MHD had computed for
+ # what was left -- a corrupt representation the client has no way
+ # of telling apart from a complete one.
+ msg "upstream body shorter than its Content-Length yields 502"
+ if ! command -v python3 >/dev/null 2>&1;
+ then
+ echo "SKIP (python3 missing)"
+ return
+ fi
+ stop_paivana
+ start_truncating_upstream "$TRUNC_PORT"
+ start_paivana "http://127.0.0.1:$TRUNC_PORT"
+ local status
+ status="$(curl -sS -o "$TMPDIR/body" -w '%{http_code}' \
+ --max-time 30 \
+ "$(PAIVANA_URL /short)" 2>"$TMPDIR/err")" \
+ || fail "curl: $(cat "$TMPDIR/err")"
+ [ "$status" = "502" ] || \
+ fail "status=$status want=502 (a 200 here means the partial body was served as if complete)"
+ grep -qi 'bad gateway' "$TMPDIR/body" || \
+ fail "no 'Bad Gateway' in body"
+ ok
+ stop_paivana
+}
+
+
function test_upstream_down() {
msg "upstream down yields 502 Bad Gateway"
# Re-point paivana at a port with nothing listening.
@@ -1233,6 +1304,9 @@ fi
test_early_response
test_early_response_no_drain
+# --- Truncated-response test (restarts paivana) -----------------------
+test_short_body
+
# --- Upstream-down test (runs last because it restarts paivana) -------
test_upstream_down
stop_paivana
