core: harden the setup channel against malformed and unanswerable requests by Ninjabippo1205 · Pull Request #29 · wineslab/libe3

Ninjabippo1205 · 2026-06-12T04:40:27Z

Summary

One bad setup request could disable dApp onboarding until the agent restarted ([Bug]: Malformed or wrong-encoding setup request permanently wedges the agent's setup socket #28). The setup REP socket must send one reply per request, and the agent bailed out without replying on undecodable bytes or an unexpected PDU type. Every such path now answers with an explicit negative SetupResponse.
The reply path is hardened so the response can always be encoded. Request ids outside E3-MessageID's 1..1000 are replaced with a fresh id (the decoders do not enforce the range; dApps do not correlate setup replies by id). A failed registration omits the dApp identifier instead of encoding 0. Any remaining setup-response encode failure aborts as an internal bug.
Regression test test_setup_bad_request, a raw ZMQ REQ peer against a real E3Agent. Garbage bytes receive a decodable negative response and the channel survives. A well-formed request with an out-of-range id receives a positive response with a substituted id. Both verified to fail on unfixed main.
CI fix, ASN.1-bound tests (test_asn1_size, test_e2e_report_path) build only when LIBE3_ENABLE_ASN1 is ON, so JSON-only builds stay green.
Per earlier review rounds, the empty ranFunctionData padding was dropped (tracked in C API allows empty ran_function_data but E3AP schema mandates SIZE(1..32768) #30) and the per-interface SmRegistry change was split out (Discussion: per-interface SmRegistry for multi-agent-in-one-process #31).

Type of change

Linked issue

Closes #28

Mandatory test checklist

./build_libe3 -c -d build -j $(nproc) -r -t passes (Release build + tests)
./build_libe3 -c -d build -j $(nproc) -g -t passes (Debug build + tests)
cd build && ctest --output-on-failure is clean
MPMC queue benchmark (./build/test_bench_mpmc_queue) shows no regression vs main
VERSION bumped per SemVer if the public API or ABI changed — not needed: no public API/ABI change (main is already 0.0.6)
If public headers under include/ were touched, ./build_libe3 --docs renders without new Doxygen warnings
If new build dependencies were added, they are installed by ./build_libe3 -I
If the libe3.pc interface changed, downstream consumers still link cleanly

CI checklist

Unit Tests workflow is green (Debug + Release matrix on ubuntu-latest)
MPMC Queue Benchmark job has posted results to this PR with no regression

Twin-repo coordination

This PR does not change the E3 wire protocol or public ABI, OR a paired PR exists in each affected twin repo. (The negative SetupResponse is a standard E3AP PDU that dApp peers already decode; no wire-format change.)

Paired PR(s): none needed

Workflow confirmation

Commits will be squashed at merge time. Updates to this PR will be applied via rebase only — no merge commits, no duplicated history.
I have read and followed CONTRIBUTING.md.

github-actions · 2026-06-12T04:41:49Z

⏱️ Full-loop Latency Benchmark (commit `64ac864`)

Full-loop latency benchmark (N=1005 after 50 warmup)

All values in microseconds (us). Link: zmq, transport: ipc, encoding: ASN.1 APER.

Phase	mean	p50	p99	max
1. Collect indication data	0	0	0	1
2. Create & encode indication	0	0	3	18
3. Deliver indication (RAN -> dApp)	91	84	169	1281
4. Decode indication	0	0	2	8
5. Process data	0	0	0	0
6. Create & encode control	0	0	1	16
7. Deliver control (dApp -> RAN)	101	87	173	869
8. Decode & handle control	0	0	1	10
Total round-trip	194	157	360	1447

Benchmarked on ubuntu-latest, Release build, ZMQ + IPC, ASN.1 APER.

github-actions · 2026-06-12T04:42:46Z

🔄 E2E dApp Integration Results (commit `64ac864`)

posix/ipc

dApp exit: 0
Indications received: 7

posix/tcp

dApp exit: 0
Indications received: 7

zmq/ipc

dApp exit: 0
Indications received: 7

zmq/tcp

dApp exit: 0
Indications received: 7

example_simple_agent + example_simple_dapp on ubuntu-latest, Release build

github-actions · 2026-06-12T04:43:07Z

🔀 E2E Topologies — multi-dApp / multi-RAN (commit `64ac864`)

zmq/ipc

✅ 1 RAN - 1 dApp: indications=5
- dapp peer=t11 ran=ran-solo sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=0.4 max=1 @seq=3) hist[<=1:5 2-5:0 6-10:0 >10:0]
✅ 1 RAN - 2 dApps: dApp#1 ind=5 sub=1, dApp#2 ind=6 sub=2, RAN saw 2 dApps
- dapp1 peer=t12 ran=ran-shared sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=0.4 max=1 @seq=3) hist[<=1:5 2-5:0 6-10:0 >10:0]
- dapp2 peer=t12 ran=ran-shared sub=2 indications=6 seq=[0..5] dropped=0 (0%) age_ms(avg=0.333333 max=1 @seq=4) hist[<=1:6 2-5:0 6-10:0 >10:0]
✅ 2 RANs - 1 dApp: from ran-a ind=5, from ran-b ind=5
- dapp peer=t2a ran=ran-a sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=0 max=0 @seq=0) hist[<=1:5 2-5:0 6-10:0 >10:0]
- dapp peer=t2b ran=ran-b sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=0 max=0 @seq=0) hist[<=1:5 2-5:0 6-10:0 >10:0]

zmq/tcp

✅ 1 RAN - 1 dApp: indications=5
- dapp peer=default ran=ran-solo sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=1 max=1 @seq=0) hist[<=1:5 2-5:0 6-10:0 >10:0]
✅ 1 RAN - 2 dApps: dApp#1 ind=5 sub=1, dApp#2 ind=6 sub=2, RAN saw 2 dApps
- dapp1 peer=default ran=ran-shared sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=0 max=0 @seq=0) hist[<=1:5 2-5:0 6-10:0 >10:0]
- dapp2 peer=default ran=ran-shared sub=2 indications=6 seq=[0..5] dropped=0 (0%) age_ms(avg=0 max=0 @seq=0) hist[<=1:6 2-5:0 6-10:0 >10:0]
✅ 2 RANs - 1 dApp: from ran-a ind=5, from ran-b ind=5
- dapp peer=default ran=ran-a sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=0 max=0 @seq=0) hist[<=1:5 2-5:0 6-10:0 >10:0]
- dapp peer=off100 ran=ran-b sub=1 indications=5 seq=[0..4] dropped=0 (0%) age_ms(avg=0 max=0 @seq=0) hist[<=1:5 2-5:0 6-10:0 >10:0]

example_simple_agent + example_simple_dapp on ubuntu-latest, Release. Indication age is report-only.

Thecave3 · 2026-06-12T14:36:06Z

let's use this branch to work exclusively on the fix

Thecave3

Thanks for the thorough writeup and the regression tests — the diagnosis in #28 is spot-on and the test harness is genuinely good. I'd like to split this before merge, though: #28 is only the setup-socket wedge, and the PR currently bundles two unrelated changes under it. Concretely:

Fix 1 (setup wedge) — keep, but reply with a real negative SetupResponse, not an empty frame. Our encoding is fixed and known a priori by every peer, so on a malformed/undecodable request we can always encode a proper SetupResponse with ResponseCode::NEGATIVE. When the bytes don't decode we don't have the original request_id — just default it (0 / next-available), since the dApp's lazy-pirate loop doesn't correlate replies by id anyway. That removes the need for the empty-frame helper entirely: the peer gets an explicit rejection instead of a silent timeout. test_setup_bad_request should then assert it receives a decodable negative response, not just n >= 0.

Fix 2 (0x00 padding) — please drop this. ranFunctionData being mandatory SIZE(1..32768) is correct; the issue is that an SM should never advertise empty data in the first place (a real SM always carries at least its name — see simple_agent / e3sm_simple), and ranFunctionList is OPTIONAL, so "nothing to advertise" means omitting the entry, not padding it. The only way an empty value reaches the encoder today is the C API's ran_function_data "may be NULL / len 0" contract, which directly contradicts the schema. Padding hides that mismatch (and makes a decoded {0x00} indistinguishable from real data). Tracked as #30 — let's fix it at the source there instead.

Fix 3 (per-interface SmRegistry) — let's pull this into its own PR. It's a structural change to a public header and ~10 call sites, unrelated to #28, and it carries a behavioral change (SMs destroyed without their destroy() hook if stop() isn't called). The multi-agent-in-one-process motivation is reasonable but deserves its own issue and review on its own merits — opened #31 to discuss whether/why we want to support that configuration. (For what it's worth, no external consumer references SmRegistry::instance(), so the API break is internal-only — splitting it costs nothing downstream.) test_multi_agent_registry moves with it.

The ASN.1-only CI gating (LIBE3_ENABLE_ASN1 test exclusion) is a clean, standalone improvement — happy to see that land either here or as its own tiny PR.

So: this PR becomes Fix 1 (negative-response variant) + test_setup_bad_request, closing #28; Fixes 2 and 3 each get their own issue/PR (#30, #31). Thanks again!

Ninjabippo1205 · 2026-07-03T19:47:48Z

Thanks for the review. All three points addressed; the branch is now rebased on latest main and reduced to Fix 1 + the CI gating.

Fix 1: reworked to an explicit negative SetupResponse. Every path that can't answer positively (undecodable bytes, wrong PDU type, response-encode failure) now encodes and sends a real SetupResponse with ResponseCode::NEGATIVE; the empty-frame helper is gone. One deviation from "default the id to 0": E3-MessageID is INTEGER (1..1000) in the grammar, so 0 is not APER-encodable, when the original id is unrecoverable the agent substitutes a freshly generated one (your "next-available" option). test_setup_bad_request now asserts the reply decodes to a negative SetupResponse (and that a well-formed setup on the same channel still succeeds afterwards); re-verified to fail on unfixed main.

The response-encode-failure path in handle_setup_request is exactly where the empty-ranFunctionData case (#30) bites, and the negative reply covers it cleanly, ranFunctionList is OPTIONAL, so a bare negative response always encodes. The peer gets an explicit rejection while #30 fixes the root cause at the C API boundary.

Fix 2: dropped as requested; nothing of it remains in this PR.

Fix 3: split out to its own branch (fix/per-interface-sm-registry) with test_multi_agent_registry; I'll open the PR against #31 once that discussion settles. Your lifecycle concern is addressed there: ~SmRegistry now runs clear() (stop-if-running, then destroy()) so the hooks fire on every teardown path, including an agent destroyed without a prior stop(), with a regression test for exactly that case.

Thecave3 · 2026-07-05T14:02:58Z

+        config_.e3ap_version,
+        std::nullopt,  // no dApp identifier assigned
+        config_.ran_identifier.empty() ? "unknown" : config_.ran_identifier,
+        {}             // no RAN functions advertised


I think probably #30 outcome is that this should be null, thoughts?

I agree on the intent, and the wire already behaves that way. Both encoders treat an empty list as the OPTIONAL ranFunctionList being absent. The ASN.1 encoder allocates the field only for a non-empty list, and the JSON encoder emits the key only for a non-empty list, so the negative reply sends no ranFunctionList at all. This also keeps the path clear of #30, since with no entries there is no ranFunctionData that could violate SIZE(1..32768).

Let's then set it at null

Done, encode_setup_response now takes the RAN function list as std::optional and the negative reply passes std::nullopt. Absent and empty both encode with the OPTIONAL ranFunctionList omitted, so the wire is unchanged.

Ninjabippo1205

Comments addressed

Ninjabippo1205 · 2026-07-09T14:43:49Z

+        config_.e3ap_version,
+        std::nullopt,  // no dApp identifier assigned
+        config_.ran_identifier.empty() ? "unknown" : config_.ran_identifier,
+        {}             // no RAN functions advertised


I agree on the intent, and the wire already behaves that way. Both encoders treat an empty list as the OPTIONAL ranFunctionList being absent. The ASN.1 encoder allocates the field only for a non-empty list, and the JSON encoder emits the key only for a non-empty list, so the negative reply sends no ranFunctionList at all. This also keeps the path clear of #30, since with no entries there is no ranFunctionData that could violate SIZE(1..32768).

Thecave3

Can you rewrite the PR and commits description as now this is is complicated to follow?

Ninjabippo1205 · 2026-07-09T15:24:18Z

Reworked, the branch is now two commits (test gating, setup channel hardening) and the description matches the current state.

Thecave3 · 2026-07-09T16:06:06Z

I'd just set NULL in the field related to #30 and then it's good for me

Ninjabippo1205

addressed comments

Ninjabippo1205 · 2026-07-09T17:04:29Z

+        config_.e3ap_version,
+        std::nullopt,  // no dApp identifier assigned
+        config_.ran_identifier.empty() ? "unknown" : config_.ran_identifier,
+        {}             // no RAN functions advertised


Done, encode_setup_response now takes the RAN function list as std::optional and the negative reply passes std::nullopt. Absent and empty both encode with the OPTIONAL ranFunctionList omitted, so the wire is unchanged.

Thecave3

After the latest branch update there are some merge errors, can you check the diff and rewrite the branch on top?

test_asn1_size drives the APER encoder directly and test_e2e_report_path hardcodes EncodingFormat::ASN1 for both the agent under test and its fake-dApp peer. On a JSON-only build (-DLIBE3_ENABLE_ASN1=OFF) the encoder factory returns nullptr for ASN.1, so both tests fail with 'encoder unavailable' (-12) rather than telling us anything about the build. Exclude the two targets at configure time when ASN.1 support is compiled out, mirroring the existing test_json_encoder skip. The tests themselves are unchanged, so ASN.1-enabled builds keep full coverage.

The RAN side of the setup channel is a ZMQ REP socket, it must send exactly one reply per received request before it can receive again. Bailing out of the setup loop without replying (undecodable bytes, wrong PDU type) left the socket stuck in the send state, wedging dApp onboarding for every subsequent peer until the agent restarted. Every such path now answers with a SetupResponse carrying ResponseCode::NEGATIVE. So that this reply always encodes, the values echoed into it are sanitized first. A request id outside E3-MessageID's 1..1000 (the decoders do not enforce the range) is replaced with a fresh one, dApps do not correlate setup replies by id. A failed registration omits the dApp identifier instead of encoding 0 against E3-DAppID (1..100). The RAN function list rides as std::nullopt, encode_setup_response takes it as an optional and absent and empty both encode with the OPTIONAL ranFunctionList omitted. After that, a setup-response encode failure can only be an internal bug and aborts. Regression test: test_setup_bad_request drives the setup channel of a real E3Agent with a raw ZMQ REQ peer. Garbage bytes get a decodable negative SetupResponse and the channel keeps serving. A well-formed request with an out-of-range id gets a positive reply with a substituted in-range id. Fails on unfixed main. Closes #28 Signed-off-by: Filippo Olimpieri <olimpieri.f@northeastern.edu>

Ninjabippo1205 requested a review from Thecave3 as a code owner June 12, 2026 04:40

This was referenced Jun 12, 2026

C API allows empty ran_function_data but E3AP schema mandates SIZE(1..32768) #30

Open

Discussion: per-interface SmRegistry for multi-agent-in-one-process #31

Open

Thecave3 requested changes Jun 12, 2026

View reviewed changes

Ninjabippo1205 force-pushed the fix/setup-robustness-and-multi-agent branch from 03e89ea to b884c25 Compare July 3, 2026 19:41

Ninjabippo1205 requested a review from Thecave3 July 3, 2026 19:49

Thecave3 reviewed Jul 5, 2026

View reviewed changes

Comment thread src/core/e3_interface.cpp

Thecave3 reviewed Jul 5, 2026

View reviewed changes

Comment thread src/core/e3_interface.cpp

Thecave3 reviewed Jul 5, 2026

View reviewed changes

Ninjabippo1205 commented Jul 9, 2026

View reviewed changes

Ninjabippo1205 requested a review from Thecave3 July 9, 2026 14:51

Thecave3 requested changes Jul 9, 2026

View reviewed changes

Ninjabippo1205 force-pushed the fix/setup-robustness-and-multi-agent branch from 2aa24c9 to cb441f3 Compare July 9, 2026 15:23

Ninjabippo1205 changed the title ~~Fix/setup robustness and multi agent~~ core: harden the setup channel against malformed and unanswerable requests Jul 9, 2026

Ninjabippo1205 force-pushed the fix/setup-robustness-and-multi-agent branch 3 times, most recently from c386b36 to b79aaf6 Compare July 9, 2026 17:07

Ninjabippo1205 commented Jul 9, 2026

View reviewed changes

Ninjabippo1205 requested a review from Thecave3 July 9, 2026 17:13

Thecave3 force-pushed the main branch from ab72dfe to 5c32de0 Compare July 10, 2026 14:40

Thecave3 reviewed Jul 10, 2026

View reviewed changes

Ninjabippo1205 added 2 commits July 10, 2026 11:32

Thecave3 force-pushed the fix/setup-robustness-and-multi-agent branch from b79aaf6 to 64ac864 Compare July 10, 2026 15:33

Thecave3 approved these changes Jul 10, 2026

View reviewed changes

Thecave3 merged commit 800fbba into main Jul 10, 2026
15 checks passed

Thecave3 deleted the fix/setup-robustness-and-multi-agent branch July 10, 2026 15:45

Uh oh!

Conversation

Ninjabippo1205 commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Linked issue

Mandatory test checklist

CI checklist

Twin-repo coordination

Workflow confirmation

Uh oh!

github-actions Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⏱️ Full-loop Latency Benchmark (commit 64ac864)

Full-loop latency benchmark (N=1005 after 50 warmup)

Uh oh!

github-actions Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔄 E2E dApp Integration Results (commit 64ac864)

posix/ipc

posix/tcp

zmq/ipc

zmq/tcp

Uh oh!

github-actions Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔀 E2E Topologies — multi-dApp / multi-RAN (commit 64ac864)

zmq/ipc

zmq/tcp

Uh oh!

Thecave3 commented Jun 12, 2026

Uh oh!

Thecave3 left a comment

Choose a reason for hiding this comment

Uh oh!

Ninjabippo1205 commented Jul 3, 2026

Uh oh!

Uh oh!

Uh oh!

Thecave3 Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

Ninjabippo1205 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Thecave3 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Ninjabippo1205 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Ninjabippo1205 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ninjabippo1205 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Thecave3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ninjabippo1205 commented Jul 9, 2026

Uh oh!

Thecave3 commented Jul 9, 2026

Uh oh!

Ninjabippo1205 left a comment

Choose a reason for hiding this comment

Uh oh!

Ninjabippo1205 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Thecave3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Ninjabippo1205 commented Jun 12, 2026 •

edited

Loading

github-actions Bot commented Jun 12, 2026 •

edited

Loading

⏱️ Full-loop Latency Benchmark (commit `64ac864`)

github-actions Bot commented Jun 12, 2026 •

edited

Loading

🔄 E2E dApp Integration Results (commit `64ac864`)

github-actions Bot commented Jun 12, 2026 •

edited

Loading

🔀 E2E Topologies — multi-dApp / multi-RAN (commit `64ac864`)

Thecave3 left a comment •

edited

Loading