Skip to content

new feature: nng pub/sub bridging#1506

Open
JaylinYu wants to merge 10 commits intomainfrom
jaylin/dev
Open

new feature: nng pub/sub bridging#1506
JaylinYu wants to merge 10 commits intomainfrom
jaylin/dev

Conversation

@JaylinYu
Copy link
Copy Markdown
Member

@JaylinYu JaylinYu commented Apr 30, 2026

Summary by CodeRabbit

  • New Features

    • Added NNG proxy configuration support with pub/sub bridge node capabilities
    • Introduced MQTT message adapter for NNG SUB0 protocol transformation
  • Bug Fixes

    • Improved QoS database cleanup lifecycle by moving cleanup from pipe close to pipe finalization
    • Added guard to prevent processing on closed pipes
  • Removals

    • Removed BLF (Binary Log Format) support and associated components

JaylinYu added 10 commits April 23, 2026 19:54
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
set nano_qos_db as NULL while create new conn_param

Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
reuse old API

Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Must seperate Topic & Payload with "/"

Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
@JaylinYu JaylinYu requested a review from alvin1221 April 30, 2026 08:29
@JaylinYu
Copy link
Copy Markdown
Member Author

@alvin1221 take my work from now on

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 30, 2026

📝 Walkthrough

Walkthrough

This PR replaces BLF configuration with NNG proxy support (pub/sub/bridge nodes), refactors QoS database cleanup from pipe-close to pipe-fini lifecycle phases across multiple transport modules, introduces a SUB0-to-MQTT message adapter function, updates declaration specifiers from extern to NNG_DECL for consistency, and adds configuration parsing logic for NNG proxy nodes.

Changes

Cohort / File(s) Summary
MQTT Parser Adapter
include/nng/protocol/mqtt/mqtt_parser.h, src/sp/protocol/mqtt/mqtt_parser.c
Declares and implements nng_sub0_msg_adapter function to transform NNG SUB0 message bodies into MQTT PUBLISH packets by splitting topic from payload at first / separator.
Configuration Type Definitions
include/nng/supplemental/nanolib/conf.h
Introduces conf_nng_pub_node, conf_nng_sub_node, and conf_nng_bridge types; replaces conf_blf blf field with conf_nng_bridge nng_proxy in main config struct.
Nanolib Header Declarations
include/nng/supplemental/nanolib/nanolib.h
Updates linkage specifiers from extern to NNG_DECL for TLS, HTTP, session, and bridge initialization functions; adds new conf_bridge_snode_init and conf_bridge_pnode_init declarations.
Pipe Lifecycle & QoS DB
src/sp/transport/mqtt/broker_tcp.c, src/sp/transport/mqtts/broker_tls.c, src/sp/transport/mqttws/nmq_websocket.c
Moves NanoMQ QoS database cleanup (nano_qos_db) from pipe_close path to pipe_fini path with added null-pointer guards; adds explicit int cast for error code in websocket callback.
Pipe Timer Callback
src/sp/protocol/mqtt/nmq_mqtt.c
Adds early-termination guard in nano_pipe_timer_cb to skip keepalive/resend processing when pipe is marked closed.
Configuration Parsing
src/supplemental/nanolib/conf.c, src/supplemental/nanolib/conf_ver2.c
Removes BLF logging; adds conf_nng_proxy_init initialization; introduces NNG pub/sub node parsing functions (conf_nng_pnode_parse, conf_nng_snode_parse); fixes null-dereference in existing bridge topic length computation.
BLF Component Removal
src/supplemental/nanolib/CMakeLists.txt, src/supplemental/nanolib/blf/CMakeLists.txt, src/supplemental/nanolib/blf/blf.cc
Removes BLF build configuration and deletes entire BLF implementation including file-range tracking, async writes, and key-span lookup functions.

Sequence Diagram(s)

sequenceDiagram
    participant Client as NNG SUB0 Client
    participant Adapter as nng_sub0_msg_adapter()
    participant Encoder as nano_encode_publish_msg()
    participant MQTT as MQTT Broker/Network
    
    Client->>Adapter: nng_msg (body: "topic/payload")
    Adapter->>Adapter: scan for '/' separator
    Adapter->>Adapter: allocate and copy topic
    Adapter->>Encoder: topic, payload, QoS, MQTT v3.1.1
    Encoder->>Encoder: encode PUBLISH packet
    Encoder-->>Adapter: nng_msg (MQTT PUBLISH frame)
    Adapter-->>Client: transformed message
    Client->>MQTT: send PUBLISH
Loading
sequenceDiagram
    participant Parser as Config Parser
    participant CfgStruct as conf_nng_proxy
    participant PubNode as conf_nng_pub_node
    participant SubNode as conf_nng_sub_node
    participant Init as Initializer
    
    Parser->>Parser: parse bridges.nng.pub from JSON
    Parser->>PubNode: conf_nng_pnode_parse()
    PubNode->>PubNode: populate topic list, validate QoS
    PubNode->>CfgStruct: push to pub_nodes vector
    Parser->>Parser: parse bridges.nng.sub from JSON
    Parser->>SubNode: conf_nng_snode_parse()
    SubNode->>SubNode: populate topic list, validate QoS
    SubNode->>CfgStruct: push to sub_nodes vector
    Init->>PubNode: conf_bridge_pnode_init()
    Init->>SubNode: conf_bridge_snode_init()
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • UAF bug fix #1503: Identical QoS database cleanup refactoring from pipe-close to pipe-fini across tcp, tls, and websocket transports with same file modifications.
  • multiple fixes #1483: Adds nano_encode_publish_msg API in mqtt_parser header/implementation that the new nng_sub0_msg_adapter function directly uses for MQTT PUBLISH encoding.
  • fix session swap & restore #1400: Modifies MQTT broker transport and session lifecycle handling, including nano_qos_db and pipe state management in the same transport files.

Suggested reviewers

  • wanghaEMQ

Poem

🐰 Hops of joy, the BLF takes its bow,
NNG proxy nodes sprout anew, and how!
SUB0 to MQTT, messages transform with flair,
QoS cleanup deferred—lifecycle repair!
From close to fini, the lifecycle flows,
A protocol garden where nano-mesh grows! 🌱

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request has no description provided by the author, violating the repository's required template that specifies issue references and change comments. Add a description following the template: include the issue number/synopsis (fixes #), comments explaining the changes, and confirmation of reading the contributing guidelines.
Docstring Coverage ⚠️ Warning Docstring coverage is 6.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'new feature: nng pub/sub bridging' directly and clearly describes the main feature being added, matching the primary changes throughout the pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jaylin/dev

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/sp/protocol/mqtt/mqtt_parser.c`:
- Around line 2154-2179: Check and validate inputs before dereferencing: ensure
origin is non-NULL before calling nng_msg_body/nng_msg_len in the mqtt_parser.c
function, and ensure topic is non-NULL/valid before passing it into
nano_encode_publish_msg for the fallback branch; if origin is NULL return NULL
(or an appropriate error) early, and if topic is NULL use a safe default (or
pass NULL explicitly only if nano_encode_publish_msg supports it) or abort the
fallback publish, updating the early-return path that logs "No valid
topic/payload separator..." to perform these checks and avoid dereferencing
invalid pointers.

In `@src/sp/transport/mqtt/broker_tcp.c`:
- Around line 224-226: The code reads p->npipe->nano_qos_db into nano_qos_db
before checking p->npipe for NULL, which can crash; change the logic to first
ensure p->npipe is not NULL (and p->conf is not NULL) then read
p->npipe->nano_qos_db (or simply move the nano_qos_db assignment below the if
that checks p->npipe and p->conf), and ensure the conditional uses the existing
symbols p->npipe, p->conf, and nano_qos_db so that nano_qos_db is only
dereferenced when p->npipe is valid.

In `@src/sp/transport/mqtts/broker_tls.c`:
- Around line 231-233: The code reads p->npipe->nano_qos_db into nano_qos_db
before checking p->npipe for NULL, risking a null-deref; modify the teardown in
broker_tls.c so you only access p->npipe->nano_qos_db after confirming p and
p->npipe are non-NULL (e.g., move the assignment of nano_qos_db inside the if
that checks p->npipe != NULL and p->conf != NULL), and ensure subsequent logic
that uses nano_qos_db still handles a NULL nano_qos_db safely.

In `@src/sp/transport/mqttws/nmq_websocket.c`:
- Around line 1338-1340: The code reads p->npipe->nano_qos_db into nano_qos_db
before checking p->npipe for NULL, which can dereference a NULL pointer during
teardown; fix by guarding p->npipe first (e.g., check p != NULL && p->npipe !=
NULL) before accessing p->npipe->nano_qos_db in the fini/cleanup path, or move
the nano_qos_db assignment to after the existing p->npipe and p->conf null
checks; update the relevant block that references p, p->npipe, p->conf and
nano_qos_db so that nano_qos_db is only read when p->npipe is confirmed
non-NULL.

In `@src/supplemental/nanolib/conf_ver2.c`:
- Around line 1470-1477: The code resets config->nng_proxy.pnodes (and similarly
snodes) to NULL before parsing, leaking previously allocated
conf_nng_pub_node/conf_nng_sub_node objects and their internal strings/topic
lists; add and call destroy helpers (e.g., conf_nng_pub_node_destroy and
conf_nng_sub_node_destroy) to free each node and its internal allocations,
iterate over the existing cvector (config->nng_proxy.pnodes and
config->nng_proxy.snodes) to destroy and clear elements before reassigning or
reinitializing, and then proceed to allocate and push new nodes with
conf_bridge_pnode_init/conf_nng_pnode_parse and cvector_push_back; apply the
same destruction/clear logic to the block that handles snodes (the code around
the other similar parse loop).
- Around line 1192-1225: conf_nng_pnode_parse (and likewise
conf_nng_snode_parse) currently allocates a topics struct and pushes it into
node->pub_list/node->sub_list even when remote_topic or local_topic is missing;
update these functions to guard like conf_bridge_node_parse: after populating s,
check that both s->remote_topic and s->local_topic (or their lengths) are
non-NULL/non-empty before calling cvector_push_back, and if the check fails free
the allocated topics struct and any strings inside it instead of pushing it to
the list so incomplete mappings are skipped.

In `@src/supplemental/nanolib/conf.c`:
- Around line 3094-3115: The init helpers leave some fields uninitialized;
update conf_nng_proxy_init to set proxy->pub_count = 0 and proxy->sub_count = 0
in addition to pub_enable, sub_enable, pnodes, snodes, and update
conf_bridge_pnode_init and conf_bridge_snode_init to explicitly initialize every
field of conf_nng_pub_node and conf_nng_sub_node (set all pointer fields to NULL
and all count/size/integer fields to 0 and booleans to false) so they no longer
rely on the caller providing zeroed storage; modify the bodies of
conf_nng_proxy_init, conf_bridge_pnode_init, and conf_bridge_snode_init
accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d95072f3-9c69-4ad5-8285-030255fd6ee9

📥 Commits

Reviewing files that changed from the base of the PR and between 3da2251 and bd5a075.

📒 Files selected for processing (13)
  • include/nng/protocol/mqtt/mqtt_parser.h
  • include/nng/supplemental/nanolib/conf.h
  • include/nng/supplemental/nanolib/nanolib.h
  • src/sp/protocol/mqtt/mqtt_parser.c
  • src/sp/protocol/mqtt/nmq_mqtt.c
  • src/sp/transport/mqtt/broker_tcp.c
  • src/sp/transport/mqtts/broker_tls.c
  • src/sp/transport/mqttws/nmq_websocket.c
  • src/supplemental/nanolib/CMakeLists.txt
  • src/supplemental/nanolib/blf/CMakeLists.txt
  • src/supplemental/nanolib/blf/blf.cc
  • src/supplemental/nanolib/conf.c
  • src/supplemental/nanolib/conf_ver2.c
💤 Files with no reviewable changes (3)
  • src/supplemental/nanolib/CMakeLists.txt
  • src/supplemental/nanolib/blf/CMakeLists.txt
  • src/supplemental/nanolib/blf/blf.cc

Comment on lines +2154 to +2179
const uint8_t *body = nng_msg_body(origin);
size_t body_len = nng_msg_len(origin);

if (body == NULL || body_len == 0) {
log_error("Empty origin message");
return NULL;
}

// Define '/' to separate topic and payload.
// NNG SUB msg format: "topic/path/payload" -> Topic: "topic", Payload: "path/payload"
const char *ptr = (const char *)body;
char *sep = NULL;

for (size_t i = 0; i < body_len; i++) {
if (ptr[i] == '/') {
sep = &ptr[i];
break;
}
}

// If no separator found, or starts with '/', fallback to using the whole message as payload.
if (sep == NULL || sep == ptr) {
log_warn("No valid topic/payload separator found in NNG sub0 msg.");
mqtt_msg = nano_encode_publish_msg(MQTT_PROTOCOL_VERSION_v311,
0, false, false, body, body_len, NULL, topic, NULL);
return mqtt_msg;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate adapter inputs before dereference/fallback publish.

origin is dereferenced before any null check, and fallback publish can use topic without validating it. Both can trigger crashes on bad inputs.

Suggested fix
 nng_msg *
 nng_sub0_msg_adapter(nng_msg *origin, char *topic)
 {
 	nng_msg *mqtt_msg = NULL;
+	if (origin == NULL) {
+		log_error("origin message is NULL");
+		return NULL;
+	}
 	const uint8_t *body     = nng_msg_body(origin);
 	size_t         body_len = nng_msg_len(origin);
@@
 	if (sep == NULL || sep == ptr) {
+		if (topic == NULL || topic[0] == '\0') {
+			log_error("Default topic is required when separator is missing");
+			return NULL;
+		}
 		log_warn("No valid topic/payload separator found in NNG sub0 msg.");
 		mqtt_msg = nano_encode_publish_msg(MQTT_PROTOCOL_VERSION_v311,
 		    0, false, false, body, body_len, NULL, topic, NULL);
 		return mqtt_msg;
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sp/protocol/mqtt/mqtt_parser.c` around lines 2154 - 2179, Check and
validate inputs before dereferencing: ensure origin is non-NULL before calling
nng_msg_body/nng_msg_len in the mqtt_parser.c function, and ensure topic is
non-NULL/valid before passing it into nano_encode_publish_msg for the fallback
branch; if origin is NULL return NULL (or an appropriate error) early, and if
topic is NULL use a safe default (or pass NULL explicitly only if
nano_encode_publish_msg supports it) or abort the fallback publish, updating the
early-return path that logs "No valid topic/payload separator..." to perform
these checks and avoid dereferencing invalid pointers.

Comment on lines +224 to +226
void *nano_qos_db = p->npipe->nano_qos_db;
if (p->npipe != NULL && p->conf != NULL &&
!p->conf->sqlite.enable && nano_qos_db != NULL) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Guard p->npipe before accessing nano_qos_db.

Line 224 dereferences p->npipe before the null check. In teardown/error paths this can crash.

Suggested fix
-	void *nano_qos_db = p->npipe->nano_qos_db;
-	if (p->npipe != NULL && p->conf != NULL &&
+	void *nano_qos_db = NULL;
+	if (p->npipe != NULL) {
+		nano_qos_db = p->npipe->nano_qos_db;
+	}
+	if (p->npipe != NULL && p->conf != NULL &&
 	    !p->conf->sqlite.enable && nano_qos_db != NULL) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void *nano_qos_db = p->npipe->nano_qos_db;
if (p->npipe != NULL && p->conf != NULL &&
!p->conf->sqlite.enable && nano_qos_db != NULL) {
void *nano_qos_db = NULL;
if (p->npipe != NULL) {
nano_qos_db = p->npipe->nano_qos_db;
}
if (p->npipe != NULL && p->conf != NULL &&
!p->conf->sqlite.enable && nano_qos_db != NULL) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sp/transport/mqtt/broker_tcp.c` around lines 224 - 226, The code reads
p->npipe->nano_qos_db into nano_qos_db before checking p->npipe for NULL, which
can crash; change the logic to first ensure p->npipe is not NULL (and p->conf is
not NULL) then read p->npipe->nano_qos_db (or simply move the nano_qos_db
assignment below the if that checks p->npipe and p->conf), and ensure the
conditional uses the existing symbols p->npipe, p->conf, and nano_qos_db so that
nano_qos_db is only dereferenced when p->npipe is valid.

Comment on lines +231 to +233
void *nano_qos_db = p->npipe->nano_qos_db;
if (p->npipe != NULL && p->conf != NULL &&
!p->conf->sqlite.enable && nano_qos_db != NULL) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix null-deref risk in QoS DB teardown.

Line 231 reads p->npipe->nano_qos_db before confirming p->npipe is non-null.

Suggested fix
-	void *nano_qos_db = p->npipe->nano_qos_db;
-	if (p->npipe != NULL && p->conf != NULL &&
+	void *nano_qos_db = NULL;
+	if (p->npipe != NULL) {
+		nano_qos_db = p->npipe->nano_qos_db;
+	}
+	if (p->npipe != NULL && p->conf != NULL &&
 		!p->conf->sqlite.enable && nano_qos_db != NULL) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void *nano_qos_db = p->npipe->nano_qos_db;
if (p->npipe != NULL && p->conf != NULL &&
!p->conf->sqlite.enable && nano_qos_db != NULL) {
void *nano_qos_db = NULL;
if (p->npipe != NULL) {
nano_qos_db = p->npipe->nano_qos_db;
}
if (p->npipe != NULL && p->conf != NULL &&
!p->conf->sqlite.enable && nano_qos_db != NULL) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sp/transport/mqtts/broker_tls.c` around lines 231 - 233, The code reads
p->npipe->nano_qos_db into nano_qos_db before checking p->npipe for NULL,
risking a null-deref; modify the teardown in broker_tls.c so you only access
p->npipe->nano_qos_db after confirming p and p->npipe are non-NULL (e.g., move
the assignment of nano_qos_db inside the if that checks p->npipe != NULL and
p->conf != NULL), and ensure subsequent logic that uses nano_qos_db still
handles a NULL nano_qos_db safely.

Comment on lines +1338 to +1340
void *nano_qos_db = p->npipe->nano_qos_db;
if (p->npipe != NULL && p->conf != NULL &&
!p->conf->sqlite.enable && nano_qos_db != NULL) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Guard p->npipe before reading nano_qos_db in fini.

Line 1338 dereferences p->npipe before the null check, which can crash in partial-init teardown paths.

Suggested fix
-	void *nano_qos_db = p->npipe->nano_qos_db;
-	if (p->npipe != NULL && p->conf != NULL &&
+	void *nano_qos_db = NULL;
+	if (p->npipe != NULL) {
+		nano_qos_db = p->npipe->nano_qos_db;
+	}
+	if (p->npipe != NULL && p->conf != NULL &&
 		!p->conf->sqlite.enable && nano_qos_db != NULL) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sp/transport/mqttws/nmq_websocket.c` around lines 1338 - 1340, The code
reads p->npipe->nano_qos_db into nano_qos_db before checking p->npipe for NULL,
which can dereference a NULL pointer during teardown; fix by guarding p->npipe
first (e.g., check p != NULL && p->npipe != NULL) before accessing
p->npipe->nano_qos_db in the fini/cleanup path, or move the nano_qos_db
assignment to after the existing p->npipe and p->conf null checks; update the
relevant block that references p, p->npipe, p->conf and nano_qos_db so that
nano_qos_db is only read when p->npipe is confirmed non-NULL.

Comment on lines +1192 to +1225
void
conf_nng_pnode_parse(
conf_nng_pub_node *node, cJSON *obj)
{
node->name = nng_strdup(obj->string);
hocon_read_str(node, clientid, obj);
hocon_read_str(node, pub_url, obj);
cJSON *forward = NULL;
cJSON *forwards = hocon_get_obj("forwards", obj);
cJSON_ArrayForEach(forward, forwards)
{
topics *s = NNI_ALLOC_STRUCT(s);
s->retain = NO_RETAIN;
s->qos = NO_QOS;
hocon_read_str(s, remote_topic, forward);
hocon_read_str(s, local_topic, forward);
hocon_read_num(s, qos, forward);
cJSON *jso_key2 = cJSON_GetObjectItem(forward, "qos");
if (cJSON_IsNumber(jso_key2) &&
(jso_key2->valuedouble == 0 || jso_key2->valuedouble == 1 ||
jso_key2->valuedouble == 2)) {
s->qos = jso_key2->valuedouble;
} else {
if (jso_key2 != NULL)
log_warn("invalid qos level detected in pub list");
}
if (s->remote_topic)
s->remote_topic_len = strlen(s->remote_topic);
if (s->local_topic)
s->local_topic_len = strlen(s->local_topic);
cvector_push_back(node->pub_list, s);
}
node->forwards_count = cvector_size(node->pub_list);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Skip incomplete NNG topic mappings instead of storing them.

Unlike conf_bridge_node_parse(), these helpers push entries even when either topic is missing. That leaves null-bearing topics objects in pub_list / sub_list, which will break later bridge handling.

♻️ Suggested fix
 	cJSON_ArrayForEach(forward, forwards)
 	{
 		topics *s = NNI_ALLOC_STRUCT(s);
 		s->retain = NO_RETAIN;
 		s->qos    = NO_QOS;
 		hocon_read_str(s, remote_topic, forward);
 		hocon_read_str(s, local_topic, forward);
+		if (!s->remote_topic || !s->local_topic) {
+			log_warn("remote_topic/local_topic not found");
+			nng_strfree(s->remote_topic);
+			nng_strfree(s->local_topic);
+			NNI_FREE_STRUCT(s);
+			continue;
+		}
 		hocon_read_num(s, qos, forward);
 		...
 	}

Apply the same guard in conf_nng_snode_parse().

Also applies to: 1227-1260

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/supplemental/nanolib/conf_ver2.c` around lines 1192 - 1225,
conf_nng_pnode_parse (and likewise conf_nng_snode_parse) currently allocates a
topics struct and pushes it into node->pub_list/node->sub_list even when
remote_topic or local_topic is missing; update these functions to guard like
conf_bridge_node_parse: after populating s, check that both s->remote_topic and
s->local_topic (or their lengths) are non-NULL/non-empty before calling
cvector_push_back, and if the check fails free the allocated topics struct and
any strings inside it instead of pushing it to the list so incomplete mappings
are skipped.

Comment on lines +1470 to +1477
config->nng_proxy.pnodes = NULL;
cJSON_ArrayForEach(node_item, node_array)
{
conf_nng_pub_node *node = NNI_ALLOC_STRUCT(node);
conf_bridge_pnode_init(node);
conf_nng_pnode_parse(node, node_item);
cvector_push_back(config->nng_proxy.pnodes, node);
config->nng_proxy.pub_enable = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't drop the previous NNG proxy config on reparse.

Resetting config->nng_proxy.pnodes / snodes to NULL here loses ownership of the previously parsed nodes without freeing their strings, topic lists, or any attached runtime state. On hot reload this leaks the old proxy config and orphans its resources.

Add destroy helpers for NNG pub/sub nodes and clear the existing side before resetting the vector.

Also applies to: 1491-1498

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/supplemental/nanolib/conf_ver2.c` around lines 1470 - 1477, The code
resets config->nng_proxy.pnodes (and similarly snodes) to NULL before parsing,
leaking previously allocated conf_nng_pub_node/conf_nng_sub_node objects and
their internal strings/topic lists; add and call destroy helpers (e.g.,
conf_nng_pub_node_destroy and conf_nng_sub_node_destroy) to free each node and
its internal allocations, iterate over the existing cvector
(config->nng_proxy.pnodes and config->nng_proxy.snodes) to destroy and clear
elements before reassigning or reinitializing, and then proceed to allocate and
push new nodes with conf_bridge_pnode_init/conf_nng_pnode_parse and
cvector_push_back; apply the same destruction/clear logic to the block that
handles snodes (the code around the other similar parse loop).

Comment on lines +3094 to +3115
static void
conf_nng_proxy_init(conf_nng_bridge *proxy)
{
proxy->pub_enable = false;
proxy->sub_enable = false;
proxy->pnodes = NULL;
proxy->snodes = NULL;
}

void
conf_bridge_pnode_init(conf_nng_pub_node *node)
{
node->pub_url = NULL;
node->pub_list = NULL;
}

void
conf_bridge_snode_init(conf_nng_sub_node *node)
{
node->sub_url = NULL;
node->sub_list = NULL;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fully initialize the new NNG config structs.

These helpers currently rely on the caller passing zeroed storage. conf_nng_proxy_init() leaves pub_count/sub_count indeterminate, and conf_bridge_pnode_init() / conf_bridge_snode_init() only clear a subset of the public struct fields. That makes stack-allocated callers easy to break.

♻️ Suggested fix
 static void
 conf_nng_proxy_init(conf_nng_bridge *proxy)
 {
-	proxy->pub_enable = false;
-	proxy->sub_enable = false;
-	proxy->pnodes = NULL;
-	proxy->snodes = NULL;
+	*proxy = (conf_nng_bridge) { 0 };
 }
 
 void
 conf_bridge_pnode_init(conf_nng_pub_node *node)
 {
-	node->pub_url = NULL;
-	node->pub_list = NULL;
+	*node = (conf_nng_pub_node) { 0 };
 }
 
 void
 conf_bridge_snode_init(conf_nng_sub_node *node)
 {
-	node->sub_url = NULL;
-	node->sub_list = NULL;
+	*node = (conf_nng_sub_node) { 0 };
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/supplemental/nanolib/conf.c` around lines 3094 - 3115, The init helpers
leave some fields uninitialized; update conf_nng_proxy_init to set
proxy->pub_count = 0 and proxy->sub_count = 0 in addition to pub_enable,
sub_enable, pnodes, snodes, and update conf_bridge_pnode_init and
conf_bridge_snode_init to explicitly initialize every field of conf_nng_pub_node
and conf_nng_sub_node (set all pointer fields to NULL and all count/size/integer
fields to 0 and booleans to false) so they no longer rely on the caller
providing zeroed storage; modify the bodies of conf_nng_proxy_init,
conf_bridge_pnode_init, and conf_bridge_snode_init accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant