Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ef15a2
Initial plan
Copilot Mar 11, 2026
d709bc0
Fix potential bugs: asn1_encode constant, mqtt_head_unpack bounds che…
Copilot Mar 11, 2026
2bdf6f1
Address review: use long long for package length, make mqtt_next_mid …
Copilot Mar 11, 2026
3e30f13
Add documentation comment for mid_ field in mqtt_client_s
Copilot Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base/hmath.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ static inline int asn1_encode(long long value, unsigned char* buf) {
*p = (unsigned char)value;
return 3;
}
else if (value < 16777126)
else if (value < 16777216)
{
*p = 0x83;
p++;
Expand Down
9 changes: 8 additions & 1 deletion event/unpack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,14 @@ int hio_unpack_by_length_field(hio_t* io, void* buf, int readbytes) {
hio_close(io);
return -1;
}
package_len = head_len + body_len + setting->length_adjustment;
int signed_package_len = (int)head_len + (int)body_len + setting->length_adjustment;
if (signed_package_len <= 0 || signed_package_len > (int)setting->package_max_length) {
hloge("Invalid package length %d!", signed_package_len);
io->error = ERR_OVER_LIMIT;
hio_close(io);
return -1;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
package_len = (unsigned int)signed_package_len;
if (remain >= package_len) {
hio_read_cb(io, (void*)p, package_len);
handled += package_len;
Expand Down
4 changes: 3 additions & 1 deletion mqtt/mqtt_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@

static unsigned short mqtt_next_mid() {
static unsigned short s_mid = 0;
return ++s_mid;
if (++s_mid == 0) s_mid = 1;
return s_mid;
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

static int mqtt_client_send(mqtt_client_t* cli, const void* buf, int len) {
Expand Down Expand Up @@ -231,6 +232,7 @@ static void mqtt_client_add_reconnect_timer(mqtt_client_t* cli) {

static void on_close(hio_t* io) {
mqtt_client_t* cli = (mqtt_client_t*)hevent_userdata(io);
if (cli == NULL) return;
cli->connected = 0;
if (cli->cb) {
cli->head.type = MQTT_TYPE_DISCONNECT;
Expand Down
1 change: 1 addition & 0 deletions mqtt/mqtt_protocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ int mqtt_head_pack(mqtt_head_t* head, unsigned char buf[]) {
}

int mqtt_head_unpack(mqtt_head_t* head, const unsigned char* buf, int len) {
if (len < 2) return 0;
head->type = (buf[0] >> 4) & 0x0F;
head->dup = (buf[0] >> 3) & 0x01;
head->qos = (buf[0] >> 1) & 0x03;
Expand Down
Loading