Fix 5 bugs: asn1_encode threshold, MQTT bounds/mid, unpack underflow, null deref

[Copilot]

Bugs found during source code audit of core library paths — parsing, protocol handling, and arithmetic correctness.

• asn1_encode wrong constant (base/hmath.h): Threshold 16777126 is a typo for 16777216 (2²⁴). Values in [16777126, 16777215] get 4-byte encoding instead of 3-byte.

• mqtt_head_unpack buffer over-read (mqtt/mqtt_protocol.c): No bounds check before accessing buf[0] when len could be 0 or 1. Reachable from network input via on_packet.

• mqtt_next_mid() wraps to 0 and is not thread-safe (mqtt/mqtt_client.c, mqtt/mqtt_client.h): unsigned short increment wraps to 0 every 65536 calls. MQTT spec §2.2.1 requires non-zero packet identifiers. Additionally, the function-static counter had no synchronization, causing data races when concurrent calls to mqtt_client_publish/subscribe/unsubscribe occurred. Replaced with a per-client mid_ field in mqtt_client_s protected by cli->mutex_.

• hio_unpack_by_length_field unsigned underflow (event/unpack.c): package_len = head_len + body_len + length_adjustment where length_adjustment is a signed short but package_len is unsigned int. Negative results wrap to ~4GB, causing OOB read or massive allocation. Now computed with long long signed arithmetic to avoid overflow even when package_max_length > INT_MAX, and validated before use. Uses ERR_INVALID_PACKAGE for non-positive lengths and ERR_OVER_LIMIT for exceeding max length.

• on_close null dereference (mqtt/mqtt_client.c): cli from hevent_userdata(io) dereferenced without null check, unlike peer callbacks connect_timeout_cb and reconnect_timer_cb which both guard against this.

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[Copilot]

Pull request overview

This PR fixes several correctness and safety issues in core parsing/protocol paths (ASN.1 length encoding, MQTT packet handling, and generic length-field unpacking) found during a source audit.

Changes:

• Fixes ASN.1 length encoding threshold for 3-byte vs 4-byte encoding.
• Hardens MQTT handling (fixed-header bounds check, non-zero message IDs, null-guard in close callback).
• Prevents unsigned underflow in hio_unpack_by_length_field by validating a signed computed package length.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
mqtt/mqtt_protocol.c	Adds a minimal-length guard before decoding MQTT fixed header fields.
mqtt/mqtt_client.c	Ensures generated MQTT packet identifiers never wrap to 0; adds null-check in on_close.
event/unpack.c	Computes package length using signed arithmetic and validates it before converting to unsigned.
base/hmath.h	Corrects ASN.1 encode threshold constant to 2^24.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

signed_package_len is computed and compared using int casts. This can overflow (or mis-compare) if package_max_length is set above INT_MAX, since (int)setting->package_max_length becomes negative. Consider computing length in int64_t (or long long) and comparing against setting->package_max_length without narrowing; also consider setting io->error to ERR_INVALID_PACKAGE when the computed length is <= 0, reserving ERR_OVER_LIMIT for the > max-length case.

[Copilot]

mqtt_next_mid() uses a function-static s_mid without any synchronization. Public APIs like mqtt_client_publish/subscribe/unsubscribe call this outside cli->mutex_, so concurrent calls can data-race and produce duplicate/undefined MIDs. Consider making the counter per-client (e.g., a cli->mid field) and incrementing it under cli->mutex_, or using an atomic counter with the same non-zero wrap behavior.

[ithewei]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied both review changes in commit 2bdf6f1:

1. event/unpack.c: Changed signed_package_len from int to long long to prevent overflow when package_max_length > INT_MAX. Split the error handling into ERR_INVALID_PACKAGE (for ≤0 length) and ERR_OVER_LIMIT (for exceeding max length).

2. mqtt/mqtt_client.c: Replaced the static counter in mqtt_next_mid() with a per-client mid_ field (added to mqtt_client_s in mqtt_client.h) protected by cli->mutex_, eliminating the data race on concurrent publish/subscribe/unsubscribe calls.