[BUG] OpenSIPS 3.6.3: AMQP shortstr length includes trailing NUL (exchange ends with \x00)

[kertorffoto-dot]

OpenSIPS version you are running

3.6.3~20260210~0e56b0ca4-1

Describe the bug

After upgrading a test registrar to OpenSIPS 3.6.3, EVI events published via event_rabbitmq started to fail routing in RabbitMQ. Packet captures show the exchange is encoded as an AMQP shortstr with length +1, causing a trailing NUL byte (\x00) to be included in the value (e.g., events.topic.name\x00). The same configuration on PROD (older OpenSIPS build) publishes without the extra NUL.

I am using code:

subscribe_event("E_REG_STAT", "rabbitmq:user:pass@1.1.1.1:5672/exchange=events.user.reg?");

On 3.3 OpenSIPS pass this in rmq packet - it is correct:

events.user.reg\x00\x00\xce

On 3.6.3 OpenSIPS pass this in rmq packet - it is broken:

events.user.reg\x00\x00\x01\xce

Possible problem is here in "modules/event_rabbitmq/event_rabbitmq.c":

dst->s[dst->len] = 0;
dst->len++;          /* <----- HERE */

To Reproduce

Just use 3.6.3~20260210~0e56b0ca4-1 and example of above 'subscribe_event()'

Expected behavior

On 3.6.3~20260210~0e56b0ca4-1 should work in same way, that it happened on 3.3.

Relevant System Logs

OS/environment information

• Operating System:
• OpenSIPS installation:
• other relevant information:

Additional context

[NormB]

Root cause fix for the off-by-one in dupl_string(), plus 4 additional issues found during testing: a memory leak in dupl_string() error paths, a latent NUL-termination bug in default user allocation, an undocumented compensation site (exchange.len--), and a strlen() - 1 truncation in rmq_print() that was wrong even before this bug existed.

---

Root Cause

The dupl_string() helper in event_rabbitmq.c increments dst->len after NUL-terminating the unescaped string:

/* NULL-terminate the string */
dst->s[dst->len] = 0;
dst->len++;           /* <-- root cause */

This causes every field populated through dupl_string() to carry a .len that includes the trailing NUL byte — one greater than the actual content length.

The bug was introduced in commit 0260de0c1 ("event_rabbitmq: URI-decode fields in rabbitmq socket", 2014). The earlier memcpy-based implementation used dst->len = end - begin + 1, which also counted the NUL. The refactor to un_escape() preserved the off-by-one via dst->len++.

The immediate visible effect is on the AMQP wire: amqp_basic_publish() receives conn->exchange as an amqp_bytes_t whose .len includes the NUL. AMQP 0-9-1 encodes exchange and routing-key as shortstr fields (1-byte length prefix + N content bytes). The broker receives a shortstr containing the exchange name followed by a 0x00 byte, which either fails to match the declared exchange or creates an exchange with an embedded NUL in its name.

Reporter's hex comparison (exchange events.user.reg, 14 chars)

OpenSIPS 3.3 (correct):    65 76 65 6e 74 73 2e 75 73 65 72 2e 72 65 67 00 00 ce
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^ ^^
                            exchange content (14 bytes)                    empty  frame-end
                                                                          rkey

OpenSIPS 3.6.3 (broken):   65 76 65 6e 74 73 2e 75 73 65 72 2e 72 65 67 00 00 01 ce
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^ ^^
                            exchange content + NUL (15 bytes!)              rkey   frame-end

The shortstr length prefix (not shown, precedes the content) is 0x0e (14) in 3.3 and 0x0f (15) in 3.6.3. The extra 0x00 shifts all subsequent fields by one byte.

---

The Patch

The fix is small: remove the len++ in dupl_string(), then remove every downstream site that compensated for it or was confused by it. Three additional issues surfaced during testing.

dupl_string() — Remove the len++. Also add shm_free cleanup on un_escape() failure, which previously leaked the already-allocated buffer.

rmq_parse() — Remove the exchange.len-- and tls_dom_name.len-- compensations. Also fix the default-user allocation (shm_malloc(len) → shm_malloc(len + 1)) to guarantee NUL termination — pre-patch, "guest" was memcpy'd without its NUL, and amqp_login() calls strlen() on the result.

rmq_print() — Remove the - 1 adjustments on address and routing key lengths. Also fix strlen(user) - 1 → strlen(user) — this was a pre-existing bug, not a compensation. The original comment says /* skip 0 */, as if strlen() counts the NUL. It doesn't; this truncated the last character of the username in rmq_print() output (cosmetic — auth uses the char* directly via amqp_login(), not this display path).

Per-site detail

Function	Change	Category
dupl_string()	Remove dst->len++	Root cause
dupl_string()	shm_free(dst->s) on un_escape() failure	Error-path fix
rmq_parse()	Remove exchange.len--	Compensation removal
rmq_parse()	Remove tls_dom_name.len--	Compensation removal
rmq_parse()	shm_malloc(len+1) + memcpy(len+1) for default user	Latent bug fix
rmq_print()	strlen(user) - 1 → strlen(user)	Pre-existing bug fix
rmq_print()	address.len - 1 → address.len	Compensation removal
rmq_print()	routing_key.len - 1 → routing_key.len	Compensation removal

Note: the exchange DO_PRINT in rmq_print() already uses exchange.len without - 1 — its compensation was in rmq_parse(), not here.

---

Consumer Impact

Regression analysis — no behavioral changes except the fixes

Consumer	Pre-Patch	Post-Patch	Change
amqp_basic_publish exchange	.len includes NUL → wrong wire encoding	.len correct → correct wire encoding	Fixed (the bug)
Default user amqp_login	strlen() on non-NUL-terminated buffer → reads past allocation	strlen() on NUL-terminated buffer → correct	Fixed
rmq_print user	strlen(user) - 1 truncated last char in display (pre-existing, cosmetic)	strlen(user) → correct	Fixed (cosmetic)
%.*s logging of address as cid	.len includes NUL → NUL in log output	.len correct → clean output	Improved
tls_find_domain_by_name	len++ then len-- cancel → correct lookup	len correct, no len-- → correct lookup	Neutral
rmq_print address, routing_key	- 1 compensations → visually correct	Direct .len → correct	Neutral
rmq_print exchange	Already correct (len-- in parse, no - 1 here)	Still correct (len direct, no len--)	Neutral
rmq_match (address, exchange, routing_key)	Both sides inflated symmetrically → match works	Both sides correct → match works	Neutral
amqp_login parsed user/password	strlen() on char* → correct	Same	Neutral
amqp_cstring_bytes routing_key	strlen() on .s → correct	Same	Neutral

Full per-consumer trace (user, password, address, exchange, routing_key, tls_dom_name)

User (char* only, .len discarded after assignment):

• Parsed via dupl_string(&tmp, ...), then param->conn.uri.user = tmp.s; tmp.len = 0.
• Consumed by amqp_login() as char* (calls strlen()), rmq_match() via strlen(), rmq_print() via strlen().
• Impact of root cause fix: None — .len is discarded.
• Impact of Change 7: Fixes pre-existing strlen() - 1 truncation in rmq_print().

Password (char* only, .len discarded):

• Same pattern as user. Consumed only by amqp_login() via strlen().
• Impact: None.

Address (str, used in multiple contexts):

• rmq_match(): symmetric — both sides from dupl_string(), no behavioral change.
• rmq_print(): Change 8 removes - 1 compensation.
• rmq_sendmsg() as host: .s used as char* for uri.host, unaffected.
• rmq_sendmsg() as cid: %.*s with inflated .len printed NUL into log; now clean.

Exchange (amqp_bytes_t — the reported bug):

• amqp_basic_publish(): .len determines AMQP shortstr wire length. This is the bug.
• rmq_match(): symmetric, no behavioral change.
• rmq_print(): Already used exchange.len without - 1 (compensation was via len-- in rmq_parse, Change 3). No change needed here.

Routing Key (str, used via amqp_cstring_bytes):

• rmq_sendmsg(): amqp_cstring_bytes(.s) calls strlen(), .len not used. Unaffected.
• rmq_match(): symmetric, no behavioral change.
• rmq_print(): Change 9 removes - 1 compensation.

TLS Domain Name (str, strict length+memcmp lookup):

• tls_find_domain_by_name() in tls_mgm/tls_domain.c performs name->len == d->name.len && memcmp(...).
• Pre-patch: len++ then len-- cancelled out. Post-patch: len correct, len-- removed.
• Regression if len-- kept but len++ removed: len = actual - 1, lookup fails.

---

Testing

Test A: AMQP wire encoding — tshark + rabbitmqadmin verification

Configuration — server_id path with a dedicated exchange and routing key:

modparam("event_rabbitmq", "server_id",
    "[test3828] uri = amqp://opensips:opensips@BROKER:5672; exchange = events.test.3828")

route {
    if (is_method("OPTIONS")) {
        rabbitmq_publish("test3828", "test.routing.key", "$ci");
        sl_send_reply(200, "OK");
    }
}

Procedure:

1. Declared exchange events.test.3828 (type: topic), bound queue test.3828.queue with routing key test.routing.key.
2. Started patched OpenSIPS, forced connection close via rabbitmqctl close_all_connections for clean captures.
3. Captured with tcpdump on broker host, sent 3 SIP OPTIONS requests.
4. Decoded with tshark -r capture.pcap -V.

Result: All 3 Basic.Publish frames show Length: 41.

AMQP method frame payload: 2 (class) + 2 (method) + 2 (ticket) + 1 (exchange len) + N (exchange) + 1 (rkey len) + M (rkey) + 1 (flags) = 9 + N + M.

With Length: 41: N + M = 32. Exchange events.test.3828 = 16, routing key test.routing.key = 16. Exact match. If NUL were still included, each shortstr would be 17 bytes → Length: 43.

rabbitmqadmin verification:

$ rabbitmqadmin get queue=test.3828.queue count=5
| test.routing.key | events.test.3828 | ... | 27 | string |
| test.routing.key | events.test.3828 | ... | 23 | string |

No embedded NUL bytes. Messages routed correctly to the bound queue.

Test B: TLS domain name lookup — instrumented debug + regression simulation

Configuration — subscribe_event path with TLS:

modparam("tls_mgm", "client_domain", "rmq_test_domain")
modparam("event_rabbitmq", "use_tls", 1)
subscribe_event("E_CORE_THRESHOLD",
    "rabbitmq:opensips:opensips@BROKER:5671/tls_domain=rmq_test_domain?tls.routing.key")

Instrumented rmq_parse with debug print after dupl_string for tls_dom_name.

Correct fix (remove len++, remove len--):

DBG: tls_dom_name='rmq_test_domain' len=15

Length 15 matches. tls_find_domain_by_name() succeeds.

Regression simulation (remove len++, keep len--):

DBG: tls_dom_name='rmq_test_domai' len=14
ERROR: TLS domain: rmq_test_domai not found

Length 14 is one short. Strict len == len && memcmp fails on the length check.

Test C: Standalone test harness — test_rmq_patch.c (1019 lines) + edge cases (487 lines)

Built a standalone test harness that extracts the patched dupl_string() and related parsing logic, linked against real OpenSIPS shm_malloc/shm_free. Tests:

• Correctness: Normal strings, percent-encoded strings (%2F, %40), empty strings, single-character strings — all produce correct .len (content length, no NUL counted).
• NUL termination: Every output dst->s[dst->len] == '\0' verified.
• Memory leak detection: 100K-iteration stress test with shm usage snapshots before/after. Malformed percent-encoding (triggering un_escape() failure) — pre-patch leaked shm on every call; post-patch (Change 2) returns cleanly with dst->s = NULL.
• Default user path: Verifies strlen() on the shm_malloc'd buffer returns exactly 5 for "guest" — pre-patch this depended on shm_malloc coincidentally zeroing memory; post-patch (Changes 5–6) the NUL is explicitly copied.
• Round-trip: dupl_string() → simulated rmq_print() → verify output matches input (catches any remaining - 1 compensation sites).

Edge case file (487 lines) covers boundary conditions: maximum-length strings, double percent-encoding, consecutive %00 sequences, allocation failure simulation.

Build verification: module compiles cleanly with the patch applied.

---

How Additional Issues Were Found

The initial static analysis identified the root cause (len++ removal), the tls_dom_name.len-- and exchange.len-- compensations in rmq_parse(), and the - 1 adjustments in rmq_print() for address and routing key.

Building the standalone test harness revealed the dupl_string() memory leak: shm_malloc is called before un_escape(), but on un_escape() failure the allocated buffer was never freed. The 100K-iteration stress test with malformed input confirmed the leak against real shm.

While tracing default-user allocation for the test harness, the latent NUL-termination bug became apparent: shm_malloc(5) + memcpy(..., 5) for "guest" produced a buffer with no guaranteed NUL terminator, yet the pointer is passed to amqp_login() → strlen(). This was promoted from "noted but not patched" to an actual fix.

The rmq_print() round-trip test then caught strlen(param->conn.uri.user) - 1. Unlike the .len - 1 compensations on address and routing key, this one was applied to a strlen() call. Since strlen() never counts the NUL byte, the - 1 was wrong regardless of dupl_string()'s behavior — it always truncated the last character of the username in rmq_print() output. The /* skip 0 */ comment in the original code confirms the author believed strlen() was including the NUL, a misunderstanding propagated by the len++ bug's mental model.

---

Patch

See PR #3834.

Full diff

--- a/modules/event_rabbitmq/event_rabbitmq.c
+++ b/modules/event_rabbitmq/event_rabbitmq.c
@@ -336,12 +336,15 @@
 		return -1;
 	}

-	if (un_escape(&tmp, dst) < 0)
+	if (un_escape(&tmp, dst) < 0) {
+		shm_free(dst->s);
+		dst->s = NULL;
+		dst->len = 0;
 		return -1;
+	}

 	/* NULL-terminate the string */
 	dst->s[dst->len] = 0;
-	dst->len++;

 	return 0;
 }
@@ -508,7 +511,6 @@
 							goto err;
 						param->conn.exchange.bytes = tmp.s;
 						param->conn.exchange.len = tmp.len;
-						param->conn.exchange.len--;
 						tmp.s = NULL;
 						tmp.len = 0;
 						param->conn.flags |= RMQ_PARAM_EKEY;
@@ -517,7 +519,6 @@
 						if (dupl_string(&param->conn.tls_dom_name,
 							it->s.s+RMQ_TLS_DOM_LEN, it->s.s + it->s.len) < 0)
 							goto err;
-						param->conn.tls_dom_name.len--;
 						param->conn.flags |= RMQ_PARAM_TLS;
 					} else if (it->s.len == RMQ_PERSISTENT_LEN &&
 						!memcmp(it->s.s, RMQ_PERSISTENT_S, RMQ_PERSISTENT_LEN)) {
@@ -559,11 +561,11 @@
 		sock->flags |= EVI_PORT;
 	}
 	if (!(param->conn.flags & RMQ_PARAM_USER) || !param->conn.uri.user) {
-		param->conn.uri.user = shm_malloc(rmq_static_holder.len);
+		param->conn.uri.user = shm_malloc(rmq_static_holder.len + 1);
 		if (!param->conn.uri.user) {
 			goto err;
 		}
-		memcpy(param->conn.uri.user, rmq_static_holder.s, rmq_static_holder.len);
+		memcpy(param->conn.uri.user, rmq_static_holder.s, rmq_static_holder.len + 1);
 		param->conn.uri.password = param->conn.uri.user;
 		param->conn.flags |= RMQ_PARAM_USER|RMQ_PARAM_PASS;
 	}
@@ -661,11 +663,11 @@

 	param = sock->params;
 	if (param->conn.flags & RMQ_PARAM_USER) {
-		DO_PRINT(param->conn.uri.user, strlen(param->conn.uri.user) - 1 /* skip 0 */);
+		DO_PRINT(param->conn.uri.user, strlen(param->conn.uri.user));
 		DO_PRINT("@", 1);
 	}
 	if (sock->flags & EVI_ADDRESS)
-		DO_PRINT(sock->address.s, sock->address.len - 1);
+		DO_PRINT(sock->address.s, sock->address.len);

 	DO_PRINT("/", 1); /* needs to be changed if it can print a key without RMQ_PARAM_RKEY */

@@ -675,7 +677,7 @@
 	}

 	if (param->conn.flags & RMQF_MAND) {
-		DO_PRINT(param->routing_key.s, param->routing_key.len - 1);
+		DO_PRINT(param->routing_key.s, param->routing_key.len);
 	}
 end:
 	return rmq_print_s;

[github-actions]

Any updates here? No progress has been made in the last 15 days, marking as stale. Will close this issue if no further updates are made in the next 30 days.