Skip to content

[C] az_mqtt adapter: static clients, no heap (AZ_IOT_AZ_MQTT_STATIC_CLIENTS) - #396

Open
Ewerton Scaboro da Silva (ewertons) wants to merge 10 commits into
mainfrom
feat/az-mqtt-static-clients
Open

Ewerton Scaboro da Silva (ewertons) wants to merge 10 commits into
mainfrom
feat/az-mqtt-static-clients

Conversation

@ewertons

Copy link
Copy Markdown
Contributor

With AZ_IOT_AZ_MQTT_STATIC_CLIENTS=N (N > 0), the az_mqtt adapter allocates nothing. Each MQTT version has N clients in static storage, and its factory is static. The default, 0, is unchanged.

Macro (also a CMake option) Default Effect
AZ_IOT_AZ_MQTT_STATIC_CLIENTS 0 Clients per MQTT version. create() returns NULL when all are in use.
AZ_IOT_AZ_MQTT_TRANSPORT_SIZE 200 KiB on Windows, 8 KiB elsewhere Bytes reserved for the transport. create() returns NULL when az_mqtt_transport_sizeof() is larger. Measured: OpenSSL 560 B, mbedTLS 4.2-4.6 KB, Schannel about 197 KB.
AZ_IOT_AZ_MQTT_CONNECT_STRINGS_SIZE 8 KiB Copies of a connect's strings. A connect that does not fit returns AZ_IOT_ERR_NOT_ENOUGH_SPACE.
AZ_IOT_AZ_MQTT_MESSAGE_STORAGE_SIZE send size + 18 Now may be 0: a connect whose session outlives the connection then returns AZ_IOT_ERR_NOT_SUPPORTED.
  • az_mqtt needs no change. Its transport size depends on the TLS library's version and configuration, so the adapter reserves an area of configurable size and checks it in create().
  • In static mode, create() and destroy() must not run concurrently.
  • The TLS library still allocates (OpenSSL, Schannel; mbedTLS unless given a static pool). This is documented.
  • Sizes: with CONSTRAINED and message storage 0, an MQTT 5 client takes about 290 KiB of .bss.

Verified on Linux (gcc, Debug):

  • ctest: 63/63 with OpenSSL, 62/62 with mbedTLS, and 62/62 for the whole SDK built with AZ_IOT_AZ_MQTT_STATIC_CLIENTS=4 (including the conformance suites against a broker).
  • nm: in the static build the adapter library references no malloc, calloc, realloc or free.
  • New tests: az_iot_tests_az_mqtt_adapter_static, _static_tiny, and az_iot_tests_az_mqtt_adapter_sizes_static.
    • Each fails when its corresponding check is removed: the string-area bound, slot release, string release, and the transport-size check.
    • They pass under ASan and UBSan.
  • MinGW -Wall -Wextra -Wpedantic -Werror syntax check of the static paths. MSVC is covered by CI.

Copilot AI left a comment

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.

🟡 Changes recommended

Static credential storage is not wiped, session-lifetime behavior contradicts the documented contract, and two tests miscalculate required storage.

3 open findings
What changed in this PR

Adds configurable heap-free static client pools to the C az_mqtt adapter while preserving dynamic allocation by default.

Changes:

  • Adds static factories, bounded client slots, transport storage, and connection-string storage.
  • Documents new build-time sizing options and zero message-storage behavior.
  • Adds static-mode, capacity, transport-size, and asymmetric-buffer tests.
File Description
c/​tests/​unit/​az_mqtt_adapter_test.c Adapts connection-failure tests for static storage.
c/​tests/​unit/​az_mqtt_adapter_static_test.c Tests static factories, slots, strings, and transport limits.
c/​tests/​support/​az_mqtt_static_v5.c Builds the MQTT 5 static test adapter.
c/​tests/​support/​az_mqtt_static_v3.c Builds the MQTT 3.1.1 static test adapter.
c/​tests/​support/​az_mqtt_static_config.h Defines constrained static test settings.
c/​tests/​support/​az_mqtt_static_common.c Builds shared static adapter code.
c/​tests/​support/​az_mqtt_asymmetric_sizes.h Enables static asymmetric-buffer testing.
c/​tests/​CMakeLists.txt Registers and configures new test targets.
c/​inc/​azure/​iot/​adapters/​az_iot_adapter_az_mqtt.h Documents static factory behavior.
c/​docs/​client-configuration.md Documents static sizing and limitations.
c/​CHANGELOG.md Announces heap-free static clients.
c/​adapters/​az_mqtt/​CMakeLists.txt Exposes new CMake sizing options.
c/​adapters/​az_mqtt/​az_iot_mqtt_az_mqtt.c Removes shared duplication allocation and gates factory freeing.
c/​adapters/​az_mqtt/​az_iot_mqtt_az_mqtt_internal.h Updates allocation-dependent declarations.
c/​adapters/​az_mqtt/​az_iot_mqtt_az_mqtt_client.h Implements static slots and bounded connection storage.
c/​adapters/​az_mqtt/​az_iot_az_mqtt_config.h Defines and validates static configuration macros.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread c/adapters/az_mqtt/az_iot_mqtt_az_mqtt_client.h Outdated
Comment thread c/adapters/az_mqtt/az_iot_mqtt_az_mqtt_client.h
Comment thread c/tests/unit/az_mqtt_adapter_test.c Outdated
Copilot AI balanced review requested due to automatic review settings October 9, 2026 04:50

Copilot AI left a comment

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.

🟡 Changes recommended

Concurrent factory creation writes shared static state and can cause a data race.

1 open finding
3 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Low severity Correct default descriptions for expanded CMake options

c/​docs/​client-configuration.md:25

The expanded option list makes the existing default description inaccurate: the new static-client, transport, connect-string, and message-storage options do not derive from AZ_IOT_AZ_MQTT_FOOTPRINT; they default respectively to 0, platform-specific storage, 8 KiB, and send size + 18. Describe these as each option's documented default so CMake users are not led to expect the footprint to control them.

🧠 Review effort: Balanced

Comment thread c/adapters/az_mqtt/az_iot_mqtt_az_mqtt_client.h Outdated
Copilot AI balanced review requested due to automatic review settings October 9, 2026 04:57
@ewertons

Copy link
Copy Markdown
Contributor Author

Copilot "Correct default descriptions for expanded CMake options": fixed in 329332e. The table now gives each option's own default. The footprint sets only the send and receive sizes and the in-flight and user-property limits. The others default to send size + 18 (message store), 0 static clients, 200 KiB on Windows or 8 KiB elsewhere (transport) and 8 KiB (connect strings).

Copilot AI left a comment

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.

🔵 Needs a closer look

Message storage sizes above INT32_MAX are accepted but cannot be represented by the downstream span API.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject MQTT storage sizes exceeding INT32_MAX

c/​adapters/​az_mqtt/​az_iot_mqtt_az_mqtt_client.h:74

This validation accepts AZ_IOT_AZ_MQTT_MESSAGE_STORAGE_SIZE values above INT32_MAX, but _azm_connect() later passes the macro directly as the int32_t length to az_span_create(). A large CMake value can therefore convert to a negative span length and hit az_core's precondition handler instead of being rejected as an invalid build configuration. Cap this newly exposed setting at INT32_MAX.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 9, 2026 05:03
@ewertons

Copy link
Copy Markdown
Contributor Author

Copilot "Reject MQTT storage sizes exceeding INT32_MAX": fixed in 0acffe1. AZ_IOT_AZ_MQTT_MESSAGE_STORAGE_SIZE above 2147483647 is now a compile error.

Copilot AI left a comment

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.

🔵 Needs a closer look

The documented credential-wiping behavior lacks a regression test that distinguishes wiping from merely releasing storage.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Add regression test to verify credentials are erased on client destruction

c/​adapters/​az_mqtt/​az_iot_mqtt_az_mqtt_client.h:1167

The documented credential-erasure behavior is not covered by the new tests: connect_strings_must_fit only proves that the cursor/capacity is released, so the earlier implementation that reset the cursor while leaving credentials in the static slot would still pass. Add a regression test that stores a recognizable password, destroys the client, and verifies that the reusable slot no longer contains those bytes.

🧠 Review effort: Balanced

@ewertons

Copy link
Copy Markdown
Contributor Author

Copilot "Add regression test to verify credentials are erased on client destruction": added in 5850620 as az_iot_tests_az_mqtt_adapter_static_wipe. After a second connect with a shorter password, the first password is no longer in the slot; after destroy(), the slot is all zeros. The test fails if either wipe is removed.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 05:12

Copilot AI left a comment

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.

🔵 Needs a closer look

A rejected persistent-session reconnect can retain prior credentials in static storage.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Release owned credentials on rejected persistent-session reconnect

c/​adapters/​az_mqtt/​az_iot_mqtt_az_mqtt_client.h:797

A rejected persistent-session reconnect returns before _azm_release_owned(). If this client previously disconnected, its copied password, proxy credentials, or TLS material therefore remains in the process-wide static slot until a later accepted connect or destroy, despite the new wipe-on-each-connect guarantee. Release the prior owned strings on this rejection path as well.

🧠 Review effort: Balanced

@ewertons

Copy link
Copy Markdown
Contributor Author

Copilot "Release owned credentials on rejected persistent-session reconnect": fixed in b91e009. connect() now releases and wipes the previous connect's strings right after the busy check, so a connect refused for invalid options or a persistent session is covered as well. The wipe test now checks the refused case, and fails without the change.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 05:22

Copilot AI left a comment

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.

🟢 Approval recommended

The static allocation paths, cleanup behavior, configuration, documentation, and boundary tests are consistent and complete.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 9, 2026 06:14

Copilot AI left a comment

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.

🟡 Changes recommended

The central no-heap contract lacks automated regression enforcement, and the changelog misstates its enabling value.

2 open findings

🧠 Review effort: Balanced

Comment thread c/tests/CMakeLists.txt
Comment on lines +696 to +701
foreach(_t az_iot_tests_az_mqtt_adapter_static az_iot_tests_az_mqtt_adapter_static_tiny)
az_iot_add_cmocka_test(${_t}
unit/az_mqtt_adapter_static_test.c
support/az_mqtt_static_common.c
support/az_mqtt_static_v3.c
support/az_mqtt_static_v5.c)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 8123f3b. With AZ_IOT_AZ_MQTT_STATIC_CLIENTS > 0, the adapter sources apply #pragma GCC poison malloc calloc realloc free (GCC and Clang) after their includes. The static test targets therefore fail to compile if the static path makes any heap call. Checked by adding malloc(1) to the static create(): both GCC and Clang rejected it, and the default build is unaffected.

Comment thread c/CHANGELOG.md Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:24

Copilot AI left a comment

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.

🔵 Needs a closer look

Platform-dependent storage alignment, lifetime management, and no-heap guarantees warrant final human review.

1 open finding

🧠 Review effort: Balanced

Copilot AI left a comment

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.

🔵 Needs a closer look

Low-level static storage and platform-dependent transport sizing warrant final human validation despite comprehensive tests.

1 open finding

🧠 Review effort: Balanced

Copilot AI left a comment

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.

🟢 Approval recommended

The static allocation lifecycle, configuration bounds, secure cleanup, documentation, and targeted tests are consistent and complete.

1 open finding

🧠 Review effort: Balanced

Copilot AI left a comment

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.

🔵 Needs a closer look

The cross-platform static-storage and credential-lifecycle changes require final human validation despite strong targeted coverage.

1 open finding

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 08:13
…LIENTS)

- AZ_IOT_AZ_MQTT_STATIC_CLIENTS=N (> 0): each MQTT version has N clients in
  static storage (buffers, transport, message store, connect strings) and a
  static factory. The adapter references no malloc, calloc or free.
  create() returns NULL when all are in use. 0, the default, is unchanged.
- AZ_IOT_AZ_MQTT_TRANSPORT_SIZE (200 KiB on Windows, 8 KiB elsewhere):
  bytes reserved for the transport, checked against az_mqtt_transport_sizeof()
  by create().
- AZ_IOT_AZ_MQTT_CONNECT_STRINGS_SIZE (8 KiB): copies of a connect's strings;
  a connect that does not fit returns AZ_IOT_ERR_NOT_ENOUGH_SPACE.
- AZ_IOT_AZ_MQTT_MESSAGE_STORAGE_SIZE may be 0: a connect whose session
  outlives the connection then returns AZ_IOT_ERR_NOT_SUPPORTED. It is now a
  CMake option too.
- Tests: static clients (limits, reuse, string area, message store,
  transport area too small) and the asymmetric-size test with static clients.
…sion rule; test sizes

- Copied connect strings are wiped when released; destroy() wipes the send buffer (heap) or the whole slot (static). Uses az_iot_crypto__wipe().
- Message storage 0 refuses clean_start false (MQTT 5: with session_expiry_seconds > 0); docs and log say so. Clean start with an expiry is accepted (tested).
- Adapter tests count the copied connect bytes exactly.
…on defaults documented

- The static factory is never written (no data race between concurrent factory_create calls); destroy is NULL.
- client-configuration.md: each option's default, not only the footprint's.
…2_MAX

It sizes an az_span; a larger value now fails the build.
A released password is gone from the slot (a shorter one does not overwrite it), and destroy() leaves the slot zeroed. Fails without either wipe.
…strings

Released (and wiped) right after the busy check, so invalid options or a refused persistent session no longer leave them in place. Test extended.
…CC/Clang)

#pragma GCC poison malloc calloc realloc free in the adapter sources when AZ_IOT_AZ_MQTT_STATIC_CLIENTS > 0, so the static test targets enforce the no-heap contract in CI. CHANGELOG: > 0 enables it.
It compiles the adapter sources, which now use az_iot_crypto__wipe().
Measured: 1.5-4 KiB beyond the configured areas on 64-bit Linux.
…ap and static

clean_start false (MQTT 5: expiry 60): a QoS 1 PUBLISH is accepted (kept in the message store) and sent, for MQTT 3.1.1 and 5.

Copilot AI left a comment

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.

🟢 Approval recommended

The static lifecycle, bounds, secure wiping, compatibility path, documentation, and tests are consistent with the stated design.

1 open finding

🧠 Review effort: Balanced

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants