[mdns]: Refactor stage #1 #776

david-cermak · 2025-03-05T16:21:09Z

Pending forward porting:

Fixes case where we create our own malloc/free allocators, therefore … (IDFGH-16053) #855
fix(mdns): put srv/txt records in additional section for ptr queries #813

MDNS Component Refactor

Introduction

The MDNS component has been refactored from a single monolithic file mdns.c into a set of focused modules with clear responsibilities. This restructuring maintains the same functionality while improving code organization, maintainability, and testability.

The refactor has been performed in thee major stages, to ensure smooth transition to production while avoiding major changes in untested paths. These stages are:

Restructuring into several modules while keeping the actual code changes to minimum
Cover functionality of each module by specifically focused unit tests.
Optimizations on module levels (string and queue operations, esp_timer dependency, socket task improvements, error checking)

Module Structure

mdns_service.c

The central service module that orchestrates the MDNS component operation. It manages the service task, timer, action queue, and system event handling. This module provides the entry points for the public API and coordinates actions between different modules. It's responsible for the general lifecycle of the MDNS service.

mdns_responder.c

Implements the responder functionality that answers incoming MDNS queries. It manages hostname and service registration, and generates appropriate response packets based on the queried information. The responder tracks all registered services and their associated metadata, handling hostname conflicts and service announcement.

mdns_browser.c

Handles browse/query functionality, managing browse requests for MDNS services on the network. It provides the infrastructure for adding, tracking, and removing browse requests, and processes incoming service discovery responses. The module manages browsing state and notifies applications when services appear or disappear.

mdns_querier.c

Handles outgoing query functionality, maintaining state for ongoing queries and processing query responses. It supports one-shot and continuous queries, manages query timeouts, and organizes results. The module provides the infrastructure for applications to discover services and resolve hostnames.

mdns_send.c

Manages packet generation and transmission. It constructs properly formatted MDNS packets, with correct headers and resource records, and handles the transmission scheduling and queueing of outgoing messages. This module encapsulates knowledge about the MDNS protocol packet structure.

mdns_receive.c

Processes incoming MDNS packets, parsing and validating their structure. It extracts queries and responses from received packets and dispatches them to the appropriate handling modules. This module focuses on packet parsing and initial classification.

mdns_pcb.c

Manages protocol control blocks (PCBs) that handle the state machine for MDNS interface operation. It coordinates the probing, announcing, and running states for each network interface and protocol combination. This module is responsible for service conflict detection, duplicate interface handling, and ensuring proper service announcements.

mdns_networking_lwip.c and mdns_networking_socket.c

These modules provide networking abstraction for different underlying network stacks. They handle multicast group management, packet reception, and transmission over the network interfaces. This abstraction allows the MDNS implementation to work with different networking stacks.

mdns_netif.c

Manages network interface registration and monitoring. It tracks network interface state changes and notifies the MDNS service when interfaces become available or unavailable, enabling dynamic adaptation to network configuration changes.

mdns_utils.c

Contains common utility functions used across multiple modules, such as string handling, data structure operations, and debug helpers. This module centralizes shared functionality to avoid code duplication.

mdns_mem_caps.c

Provides memory allocation functions with capability-based allocation support. It encapsulates memory management operations, making it easier to track and debug memory usage across the MDNS component.

mdns_debug.c

Contains debugging and logging utilities specific to the MDNS component. It provides structured logging and debug information for troubleshooting complex MDNS operations.

Testability Improvements

The refactored module structure significantly enhances testability by clearly separating different functional areas. Each module can now be tested in isolation with proper mocking of its dependencies. The separation of networking, packet handling, and business logic makes it possible to create focused unit tests without the complexity of managing the entire MDNS stack. Module-specific state is now contained within individual modules rather than distributed throughout a monolithic codebase, making test case setup and verification more straightforward. The refactoring also improves the ability to simulate various network conditions and edge cases by intercepting inter-module communication at well-defined boundaries.

Conclusion

The MDNS component refactoring provides a more maintainable and testable architecture while preserving the existing public API. By breaking down the monolithic implementation into focused modules with clear responsibilities, the codebase becomes easier to understand, extend, and maintain. The refactored structure makes it simpler to identify and fix bugs, implement new features, and adapt to future requirements. This architectural improvement supports long-term evolution of the component while maintaining backward compatibility for existing applications.

Adding fuzzer tests

example of a failed job https://github.com/espressif/esp-protocols/actions/runs/13990422842/job/39172942586 (with artifacts containing crashes)
example of a passing job https://github.com/espressif/esp-protocols/actions/runs/13990767171/job/39173999992?pr=776

components/mdns/mdns_packet.c

components/mdns/mdns_utils.c

components/mdns/private_include/mdns_utils.h

components/mdns/mdns_netif.c

zwx1995esp

For the whole architecture, LGTM. I reviewed only for mdns_service.c and mdns_utils.c and left some nits and questions. For the other files, I will finish this weekend.

components/mdns/private_include/mdns_private.h

components/mdns/mdns_utils.c

components/mdns/mdns_service.c

components/mdns/mdns_send.c

zwx1995esp · 2025-04-02T13:30:56Z

components/mdns/mdns_send.c

+ * @param  index        offset of uint16_t value
+ * @param  value        the value to set
+ */
+static inline void set_u16(uint8_t *packet, uint16_t index, uint16_t value)


Question: what is the difference between this function and mdns_utils_append_u16?

the append function is supposed to be used when composing a packet, so it keeps track of the current pointer and returns the position.

Ok, I just see the implementation of the two function are much the same, can we use the function mdns_utils_append_u16 for both set and append scenario?

Good point, but i'd again leave it for phase III, where we can address optimizations. I'd keep it here as is, just to reduce the complexity of this PR

components/mdns/mdns_send.c

components/mdns/mdns_service.c

.github/workflows/mdns__host-tests.yml

euripedesrocha · 2025-05-19T12:18:47Z

.github/workflows/mdns__host-tests.yml

+              exit 1;
+            fi
+          else
+            exit 1;


Should this path exit 0 ?

This is still a failure -- fuzzer returns another error code.

components/mdns/CMakeLists.txt

euripedesrocha · 2025-05-19T12:40:38Z

components/mdns/Kconfig

+        help
+            This buffer is used to output mDNS packets in debug prints.
+
    config MDNS_ENABLE_CONSOLE_CLI


Why this option defaults to y?

It's also y on current master (no change here).

Why? This option was added by a public contributor, before it was simply built unconditionally -- so the compatibility reasons.

euripedesrocha · 2025-05-19T14:35:16Z

components/mdns/mdns_service.c

+
+bool mdns_priv_queue_action(mdns_action_t *action)
+{
+    if (xQueueSend(s_action_queue, &action, (TickType_t)0) != pdPASS) {


Why make this non-blocking and treat the full queue as an error?

Again not functionality change -- the goal of this refactoring stage.
But the reason is the requirement of non-blocking API + some calls IIRC are from esp_timer callback (which should never block)

euripedesrocha · 2025-05-19T14:49:48Z

components/mdns/mdns_responder.c

+
+esp_err_t mdns_priv_responder_init(void)
+{
+    s_server = (mdns_server_t *)mdns_mem_malloc(sizeof(mdns_server_t));


Why not use _calloc?

This is a pretty common pattern in the entire mdns code -- I'll address in the upcoming stage-#2

components/mdns/tests/host_unit_test/unity/unit_test.cmake

components/mdns/tests/host_unit_test/stubs/mdns_engine.c

zwx1995esp

Some minor nitpicks — overall LGTM! Thanks to David for this huge refactor!

components/mdns/mdns_browser.c

components/mdns/mdns_debug.c

components/mdns/mdns_querier.c

components/mdns/mdns_receive.c

components/mdns/mdns_send.c

components/mdns/mdns_service.c

components/mdns/refactoring_details.md

zwx1995esp · 2025-10-27T09:54:29Z

Hi, @david-cermak Some of the comments are related to the format. I think we can add a pipeline jobs later to check if the code in new PR is pretty or not.

david-cermak · 2025-11-03T08:25:47Z

Hi, @david-cermak Some of the comments are related to the format. I think we can add a pipeline jobs later to check if the code in new PR is pretty or not.

Thanks for the final review! Yes, I agree -- the point of "stage #1" was to keep the actual changes to minimum to

avoid unwanted regression and focus only on structural changes
so the agent can provide better code review -- although models are way smarter than in Feb/2025, so it shouldn't be a big issue anymore.

feat(mdns): Make mdns-debug a separate module feat(mdns): Separate packet-tx and browse feat(mdns): Add responder module feat(mdns): Querier-search stuff, Responder-pcb stuff feat(mdns): action queue abstraction fix(mdns): Minor cleanup of packet parser fix(mdns): Cleanup and split interfaces between modules feat(mdns): Add fuzzing job with AFL++ fix(mdns): Rename and cleanup feat(mdns): Add service module fix(mdns): Forward porting 8ca45f3 delete race fix(mdns): Minor cleanup fix(mdns): Refactor fuzzer test suite fix(mdns): Add host unit tests fix(mdns): Add receiver unit tests feat(mdns): Add test template for mdns-sender fix(mdns): Minor cleanup fix(mdns): Cleanup querier and pcb fix(mdns): Cleanup mdns sender fix(mdns): Cleanup mdns sender fix(mdns): Add documentation about the refactor, remove old fuzz fix(mdns): Add refactoring details to help review changes fix(mdns): Add data-flow diagram fix(mdns): Make debug module more flexible and enable routing printfs to esp_log component fix(mdns): Address minor review comments fix(mdns): Debug prints with small buffer fix(mdns): Minor cleanup fix(mdns): Fix host tests by freezing idf-build-apps to 2.10 fix(mdns): per final code review

components/mdns/mdns_send.c

zwx1995esp · 2025-11-07T07:13:17Z

components/mdns/refactoring_details.md

+| [_mdns_service_match_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L409) | [mdns_utils_service_match_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L162) | The refactored function changed from static to public scope (removed static keyword), which could break encapsulation and allow unintended external access to internal service matching logic. Additionally, the helper function `mdns_utils_str_null_or_empty` might have different behavior than the original `_str_null_or_empty` if it was reimplemented during the utility extraction. |
+| [_mdns_get_service_item_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L422) | [mdns_utils_get_service_item_instance](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L143) |  |
+| [_mdns_read_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L452) | [mdns_utils_read_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_utils.c#L18) | The refactored function mdns_utils_parse_fqdn uses a static buffer (static char buf[MDNS_NAME_BUF_LEN]) which could cause thread safety issues in a multi-threaded environment or reentrancy problems if the function is called recursively. Additionally, the memory movement operations using memmove in mdns_utils_parse_fqdn appear to be incorrectly calculating the offsets - the destination and source calculations seem to assume the mdns_name_t structure fields are exactly MDNS_NAME_BUF_LEN bytes apart, which may not be correct if the structure has padding or different field sizes. |
+| [_mdns_set_u16](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L512) | ??? |  |


Do we need to fix the ???, I think this function should be the static function set_u16 in the file mdns_send.c. Or could we just remove this line?

These are my agentic "safe-guards" -- the reviewer takes a few attempts to review both versions and pull different portion of context, but if it couldn't confidently comment it would give this "???" --> and indication for us human-reviewer to take over.
Had to to this, to save some money 😄 -- this big review cost about 0.5 USD...

Ha! Okay, I don’t think I’m willing to spend half a day reviewing this(so did I do, just a cursory look). I'm willing to spend more time on the code. 😄 So let's keep the it as is.

I don’t think I’m willing to spend half a day reviewing this

I meant we should mainly review the "???" instances -> 3 out of 244 (not so bad IMO)

_mdns_set_u16()

_mdns_append_subtype_ptr_record()

_mdns_browse_result_add_txt()

zwx1995esp · 2025-11-07T07:13:51Z

components/mdns/refactoring_details.md

+| [append_fqdn_dots](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L711) | [append_fqdn_dots](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L455) | The refactored append_fqdn_dots function in Result 1 has a subtle bug: it always appends "arpa" suffix regardless of the input domain, whereas the original function's logic suggests this should only be used for reverse DNS queries. The function also ignores the 'last' parameter completely, which may indicate missing functionality for handling non-reverse DNS queries. Additionally, the hardcoded "arpa" suffix could cause incorrect FQDN formatting for regular forward DNS queries. |
+| [_mdns_append_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L750) | [append_fqdn](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L667) | The refactored function calls `append_string` instead of `_mdns_append_string`, but the search results show no implementation of `append_string` in the refactored codebase. This suggests the function may be missing, which would cause linking errors or runtime failures. Additionally, the recursive call to `append_fqdn` relies on this potentially missing function, creating a dependency chain that may be broken. |
+| [_mdns_append_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L823) | [append_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns_send.c#L781) |  |
+| [_mdns_append_subtype_ptr_record](https://github.com/espressif/esp-protocols/blob/main/components/mdns/mdns.c#L873) | ??? |  |


Also here ???

zwx1995esp · 2025-11-07T07:23:22Z

Hi @david-cermak I just found two strange places ??? in the AI generated refactor details. (Just took a cursory look, because I thought the AI might make some mistakes.). Do we need to fix them?

david-cermak · 2025-11-07T08:00:56Z

I just found two strange places ??? in the AI generated refactor details. (Just took a cursory look, because I thought the AI might make some mistakes.). Do we need to fix them?

It just means that we need to review this function manually (and carefully)

david-cermak added the mdns label Mar 5, 2025

david-cermak self-assigned this Mar 5, 2025

github-advanced-security bot found potential problems Mar 5, 2025

View reviewed changes

components/mdns/mdns_packet.c Fixed Show fixed Hide fixed

components/mdns/mdns_utils.c Fixed Show fixed Hide fixed

components/mdns/private_include/mdns_utils.h Fixed Show fixed Hide fixed

david-cermak force-pushed the mdns/refactor branch from f9d11ac to a173ff4 Compare March 5, 2025 16:28

david-cermak marked this pull request as draft March 5, 2025 16:33

david-cermak force-pushed the mdns/refactor branch 7 times, most recently from 2019c2f to 611c665 Compare March 9, 2025 17:25

github-advanced-security bot found potential problems Mar 9, 2025

View reviewed changes

components/mdns/mdns_netif.c Fixed Show fixed Hide fixed

components/mdns/mdns_netif.c Fixed Show fixed Hide fixed

components/mdns/mdns_netif.c Fixed Show fixed Hide fixed

david-cermak force-pushed the mdns/refactor branch 6 times, most recently from bf55daf to 89499cf Compare March 11, 2025 15:31

david-cermak added mdns-fuzz and removed mdns labels Mar 19, 2025

david-cermak force-pushed the mdns/refactor branch 7 times, most recently from e7072f5 to ae08137 Compare March 21, 2025 13:32

david-cermak added the mdns label Mar 24, 2025

david-cermak force-pushed the mdns/refactor branch from 779d1e8 to 2005f3f Compare March 24, 2025 12:51

zwx1995esp reviewed Apr 2, 2025

View reviewed changes

david-cermak requested review from abhik-roy85, alisitsyn, euripedesrocha, glmfe and kostaond April 4, 2025 08:56

github-advanced-security bot found potential problems Apr 4, 2025

View reviewed changes

components/mdns/mdns_service.c Fixed Show fixed Hide fixed

components/mdns/mdns_service.c Fixed Show fixed Hide fixed

david-cermak force-pushed the mdns/refactor branch from 736df15 to ed87fa2 Compare April 4, 2025 09:00

david-cermak force-pushed the mdns/refactor branch from 3270a27 to c42b578 Compare April 14, 2025 10:08

This was referenced Apr 23, 2025

mdns component does not support subtypes containing \0 bytes (IDFGH-15165) #804

Open

mdns component does not support query for subtypes (IDFGH-15167) #805

Open

euripedesrocha reviewed May 19, 2025

View reviewed changes

.github/workflows/mdns__host-tests.yml Outdated Show resolved Hide resolved

david-cermak mentioned this pull request May 28, 2025

fix(mdns): put srv/txt records in additional section for ptr queries #813

Merged

6 tasks

euripedesrocha reviewed Jun 12, 2025

View reviewed changes

zwx1995esp approved these changes Oct 27, 2025

View reviewed changes

david-cermak force-pushed the mdns/refactor branch from 60cb3d7 to 3c6098a Compare November 3, 2025 09:45

ci(mdns): Forward porting (8bba3a9)

eeb4a89

github-advanced-security bot found potential problems Nov 3, 2025

View reviewed changes

components/mdns/mdns_send.c Fixed Show fixed Hide fixed

components/mdns/mdns_send.c Fixed Show fixed Hide fixed

ci(mdns): Forward porting (b7b8c5d)

d3a83e9

david-cermak force-pushed the mdns/refactor branch from 767ebfc to d3a83e9 Compare November 3, 2025 11:12

david-cermak added 3 commits November 3, 2025 14:48

fix(ci): Fix host-test CI issue

7bc09fe

ci(mdns): forward porting fa96de3

403556d

fix(mdns): Regenerated detailed agentic code review

fd3bdb7

zwx1995esp reviewed Nov 7, 2025

View reviewed changes

[mdns]: Refactor stage #1 #776

Are you sure you want to change the base?

[mdns]: Refactor stage #1 #776

Uh oh!

Conversation

david-cermak commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pending forward porting:

MDNS Component Refactor

Introduction

Module Structure

mdns_service.c

mdns_responder.c

mdns_browser.c

mdns_querier.c

mdns_send.c

mdns_receive.c

mdns_pcb.c

mdns_networking_lwip.c and mdns_networking_socket.c

mdns_netif.c

mdns_utils.c

mdns_mem_caps.c

mdns_debug.c

Testability Improvements

Conclusion

Adding fuzzer tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zwx1995esp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zwx1995esp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-cermak commented Mar 5, 2025 •

edited

Loading