Skip to content

Documentation and cleanup fixes#376

Merged
AniruddhaKanhere merged 6 commits into
FreeRTOS:mainfrom
AniruddhaKanhere:documentation-and-cleanup-fixes
Jul 6, 2026
Merged

Documentation and cleanup fixes#376
AniruddhaKanhere merged 6 commits into
FreeRTOS:mainfrom
AniruddhaKanhere:documentation-and-cleanup-fixes

Conversation

@AniruddhaKanhere

@AniruddhaKanhere AniruddhaKanhere commented Jul 1, 2026

Copy link
Copy Markdown
Member

Description

Test Steps

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AniruddhaKanhere
AniruddhaKanhere force-pushed the documentation-and-cleanup-fixes branch 2 times, most recently from 2ce273d to 7593ea5 Compare July 2, 2026 00:09
archigup
archigup previously approved these changes Jul 2, 2026
View reviewed changes
@AniruddhaKanhere
AniruddhaKanhere force-pushed the documentation-and-cleanup-fixes branch from 548ba4b to 20d5248 Compare July 2, 2026 20:21
@AniruddhaKanhere
Fix documentation and example-code defects in public headers
f752dd9 
Corrects Doxygen comments and in-comment code examples reported by review:
- transport_interface.h: TransportWritev_t doc referenced nonexistent
  TransportIoVector_t (should be TransportOutVector_t).
- core_mqtt.h: missing comma in MQTT_GetSubAckStatusCodes example callback;
  MQTT_InitRetransmits example prototypes matched to the actual retransmit
  typedefs; SubAck/UnsubAck example loops use size_t index; removed empty
  MQTT_CheckConnectStatus example block; replaced VLA-from-const in the
  Init examples with a literal size; clarified SUBACK reason-code note.
- core_mqtt_serializer.h: examples use uint32_t (not size_t) for
  remainingLength/packetSize out-params; added missing maxPacketSize args to
  Get*PacketSize example calls; fixed MQTT_ValidatePublishParams example arg
  order; MQTT_DeserializeAck pConnectProperties tag is @param[in] (const);
  declared packetSize in the SerializePingreq example; dropped the misleading
  'CONNACK' mention from the MQTT_DeserializeAck reason-code comment.

Comment-only changes; no executable code affected.
@AniruddhaKanhere
Fix non-security diagnostic and correctness defects
5338b49 
- addPropUint16/addPropUint32 (core_mqtt_prop_serializer.c): the shared
  helpers logged a hardcoded property name ("Receive Maximum"/"Subscription
  Id") for every property they serialize; log the actual propId instead.
- MQTT_SerializePublishHeader (core_mqtt_serializer.c): the buffer-too-small
  log subtracted payloadLength from packetSize, which already excludes the
  payload, causing a size_t underflow in the message; log packetSize directly.
- MQTT_GetDisconnectPacketSize (core_mqtt_serializer.c): accept a remaining
  length exactly equal to MQTT_MAX_REMAINING_LENGTH (inclusive max), matching
  calculateSubscriptionPacketSize's boundary handling.
- MQTT_ValidateSubscribeProperties (core_mqtt_serializer.c): advance pLocalIndex
  past the decoded Subscription Identifier (matching deserializePublishProperties)
  so subsequent properties are not misparsed.
- sendUnsubscribeWithoutCopy (core_mqtt.c): bound the property-buffer length
  against MQTT_MAX_PACKET_SIZE (as the per-topic branch and sendPublishWithoutCopy
  do) instead of MQTT_MAX_REMAINING_LENGTH.

Memory safety re-verified via the MQTT_GetDisconnectPacketSize,
MQTT_ValidateSubscribeProperties, and MQTT_Unsubscribe CBMC proofs.
@AniruddhaKanhere
Recompute publish-record limits from configured values on each connect
5b818f3 
MQTT_Connect capped incomingPublishRecordMaxCount / outgoingPublishRecordMaxCount
at the negotiated Receive Maximum by comparing against, and overwriting, the same
field. Because the field was only ever lowered and never restored, the limits
shrank monotonically across reconnects: after connecting to a broker advertising
a small Receive Maximum, a later reconnect to a broker advertising a larger one
kept the smaller cap for the life of the context, wrongly throttling concurrent
QoS1/QoS2 publishes.

Store the application-configured counts (from MQTT_InitStatefulQoS) separately in
new MQTTContext_t fields and, on every successful connect, reset the effective
limits to those configured values before applying the negotiated minimum. This
matches the behavior the surrounding comment already described.

Memory safety re-verified via the MQTT_InitStatefulQoS CBMC proof (the MQTT_Connect
proof does not build locally due to a pre-existing CBMC 6.9.0 stub declaration
conflict in deserialize_helpers_stubs.c, unrelated to this change).
@AniruddhaKanhere
Roll back reserved publish state when MQTT_Publish send fails
130f6f8 
For QoS>0, MQTT_Publish reserved an outgoing publish record via MQTT_ReserveState
and then called sendPublishWithoutCopy(). If that send failed, the state update
(MQTT_UpdateStatePublish) was skipped, but the reserved record was never released.
compactRecords() only reclaims MQTT_PACKET_ID_INVALID slots, so the MQTTPublishSend
record was leaked, and repeated send failures could exhaust the outgoing record
pool (MQTTNoMemory) on the reconnect-less retry path.

Track whether this call newly reserved a record and, if the send fails, release it
with MQTT_RemoveStateRecord while preserving the send-failure status for the caller.
Duplicate retransmits (pre-existing records reused via MQTTStateCollision) are left
intact, and update-failure after a successful send is unchanged (the packet is in
flight). Since the retransmit-store hook runs before the network send, the packet is
never transmitted on any sendPublishWithoutCopy failure, so rollback is always safe.

Updated two existing store-failure tests to assert the rollback and added a
send-failure regression test. Memory safety re-verified via the MQTT_Publish CBMC
proof.
kstribrnAmzn
kstribrnAmzn reviewed Jul 2, 2026
View reviewed changes
Comment thread source/include/core_mqtt.h Outdated
Comment thread source/core_mqtt.c
Comment thread test/unit-test/core_mqtt_serializer_utest.c
kstribrnAmzn
kstribrnAmzn reviewed Jul 2, 2026
View reviewed changes
Comment thread source/core_mqtt.c
@AniruddhaKanhere
AniruddhaKanhere force-pushed the documentation-and-cleanup-fixes branch from 20d5248 to 56a8397 Compare July 2, 2026 23:40
kstribrnAmzn
kstribrnAmzn reviewed Jul 6, 2026
View reviewed changes
Comment thread test/unit-test/core_mqtt_serializer_utest.c Outdated
@AniruddhaKanhere
AniruddhaKanhere force-pushed the documentation-and-cleanup-fixes branch from 56a8397 to fc3f6b9 Compare July 6, 2026 20:01
@AniruddhaKanhere
Set currentIndex when deserializing SUBACK/UNSUBACK properties
62cead8 
deserializeSubUnsubAckProperties populated the property buffer's pBuffer and
bufferLength but never set currentIndex, unlike its siblings
deserializeConnackProperties and deserializePublishProperties. Because the
property-get path treats currentIndex as the count of valid bytes
(MQTT_GetNextPropertyType / MQTTPropGet_* return MQTTEndOfProperties once the
read cursor reaches it), and the caller zero-initializes the buffer, every
property in a SUBACK/UNSUBACK (Reason String, User Property) was silently
unreadable through the public API.

Set currentIndex = propertyLength (matching the sibling deserializers) and
reorder the pBuffer/bufferLength assignments to match deserializePublishProperties.

Adds a serializer unit test that deserializes a SUBACK carrying a Reason String
and asserts it is reachable via MQTT_GetNextPropertyType (fails before this fix).
Memory safety re-verified via the deserializeSubUnsubAckProperties CBMC proof.
@AniruddhaKanhere
Reject SUBACK/UNSUBACK carrying zero reason codes
8710c3d 
Per MQTT 5 (sections 3.9 and 3.11) a SUBACK/UNSUBACK payload must contain one
reason code per topic filter, so at least one is always present. The library
previously accepted a packet whose remaining length was consumed entirely by the
packet identifier and properties:
- readSubackStatus() looped zero times for statusCount == 0, returned MQTTSuccess
  and set reasonCodeLength = 0.
- MQTT_GetSubAckStatusCodes / MQTT_GetUnsubAckStatusCodes could return MQTTSuccess
  with *pPayloadSize == 0 and pPayloadStart pointing one past the properties.

Reject these malformed packets with MQTTBadResponse in all three places.

Adds unit tests: a zero-reason-code SUBACK through MQTT_DeserializeAck
(serializer), and zero-payload cases for both public status-code getters.
Memory safety re-verified via the MQTT_DeserializeAck, MQTT_GetSubAckStatusCodes
and MQTT_GetUnsubAckStatusCodes CBMC proofs.
@AniruddhaKanhere
AniruddhaKanhere force-pushed the documentation-and-cleanup-fixes branch from fc3f6b9 to 8710c3d Compare July 6, 2026 20:17
@AniruddhaKanhere
AniruddhaKanhere enabled auto-merge (rebase) July 6, 2026 20:48
@AniruddhaKanhere
AniruddhaKanhere merged commit 5be5f95 into FreeRTOS:main Jul 6, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kstribrnAmzn kstribrnAmzn kstribrnAmzn approved these changes
@rawalexe rawalexe rawalexe approved these changes
@archigup archigup archigup left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AniruddhaKanhere @archigup @kstribrnAmzn @rawalexe