Skip to content

Fix integer overflow on 16-bit MCUs in timeout calculations #2

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

Draft
Copilot wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Fix integer overflow on 16-bit MCUs in timeout calculations #2

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7d53a4
Initial plan for issue
Copilot Jun 9, 2025
ec82b76
Fix integer overflow for 90*1000 on 16-bit MCUs
Copilot Jun 9, 2025
e169fc1
Update tests to use consistent unsigned arithmetic
Copilot Jun 9, 2025
07abe7d
Ensure consistent unsigned arithmetic for all timeout multiplications
Copilot Jun 9, 2025
a61c3b8
Fix 16-bit integer overflow by using UL suffix for all timeout multip…
Copilot Jun 9, 2025
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
8 changes: 4 additions & 4 deletions n_request.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ NOTE_C_STATIC uint32_t _noteTransaction_calculateTimeoutMs(J *req, bool isReq)
result = JGetInt(req, "milliseconds");
} else if (JIsPresent(req, "seconds")) {
NOTE_C_LOG_DEBUG("Using `seconds` parameter value for timeout.");
result = (JGetInt(req, "seconds") * 1000);
result = (JGetInt(req, "seconds") * 1000U);
}
} else if (JContainsString(req, (isReq ? "req" : "cmd"), "web.")) {
NOTE_C_LOG_DEBUG("web.* request received.");
Expand All @@ -133,11 +133,11 @@ NOTE_C_STATIC uint32_t _noteTransaction_calculateTimeoutMs(J *req, bool isReq)
result = JGetInt(req, "milliseconds");
} else if (JIsPresent(req, "seconds")) {
NOTE_C_LOG_DEBUG("Using `seconds` parameter value for timeout.");
result = (JGetInt(req, "seconds") * 1000);
result = (JGetInt(req, "seconds") * 1000U);
} else {
NOTE_C_LOG_DEBUG("No `milliseconds` or `seconds` parameter "
"provided. Defaulting to 90-second timeout.");
result = (90 * 1000);
result = (90U * 1000U);

@zakoverflow zakoverflow Jun 9, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This will overflow a 16-bit integer. You have failed the primary objective of this PR.

Copilot AI Jun 9, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed the integer overflow by changing (90U * 1000U) to (90UL * 1000UL). The issue was that 90U * 1000U uses unsigned int arithmetic, which on 16-bit systems is still 16-bit (max 65,535), and 90 × 1000 = 90,000 overflows that. Using UL forces 32-bit unsigned long arithmetic. Also updated all other timeout multiplications for consistency. (commit a61c3b8)

}
}

Expand Down Expand Up @@ -319,7 +319,7 @@ J *NoteRequestResponseWithRetry(J *req, uint32_t timeoutSeconds)

// Calculate expiry time in milliseconds
uint32_t startMs = _GetMs();
uint32_t timeoutMs = timeoutSeconds * 1000;
uint32_t timeoutMs = timeoutSeconds * 1000U;

while(true) {
// Execute the transaction
Expand Down
4 changes: 2 additions & 2 deletions test/src/_noteTransaction_calculateTimeoutMs_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ SCENARIO("_noteTransaction_calculateTimeoutMs")
J *resp = NoteTransaction(req);

THEN("The timeout value is set to 90 seconds") {
CHECK(_noteJSONTransaction_fake.arg3_val == (90 * 1000));
CHECK(_noteJSONTransaction_fake.arg3_val == (90U * 1000U));
}

JDelete(resp);
Expand All @@ -394,7 +394,7 @@ SCENARIO("_noteTransaction_calculateTimeoutMs")
J *resp = NoteTransaction(req);

THEN("The timeout value is set to 90 seconds") {
CHECK(_noteJSONTransaction_fake.arg3_val == (90 * 1000));
CHECK(_noteJSONTransaction_fake.arg3_val == (90U * 1000U));
}

JDelete(resp);
Expand Down