Skip to content

Fix unsigned comparisons, integer overflows, and add defensive checks #9

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
wants to merge 3 commits into
base: main
Choose a base branch
from
+108 −49
Open

Fix unsigned comparisons, integer overflows, and add defensive checks #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74afd49
Initial plan
Copilot Nov 10, 2025
db81c1c
Fix critical bugs: unsigned comparisons, overflow protection, null ch…
Copilot Nov 10, 2025
8d11a33
Add additional safety checks: placeholder detection, division by zero…
Copilot Nov 10, 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
15 changes: 15 additions & 0 deletions src/adaptive_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ size_t AdaptiveBuffer::calculateBufferSize(int32_t rssi)
//
// Note: We cap at 150% to avoid excessive memory usage on long-term weak signals

// Safety check: ensure base_buffer_size is valid
if (base_buffer_size == 0)
{
return 256; // Return minimum safe size
}

size_t new_size;

if (rssi >= -60)
Expand Down Expand Up @@ -83,6 +89,15 @@ void AdaptiveBuffer::updateBufferSize(int32_t rssi)
// Only log if size changed significantly (>10%)
if (new_size != current_buffer_size)
{
// Prevent division by zero
if (current_buffer_size == 0)
{
current_buffer_size = new_size;
adjustment_count++;
last_adjustment_time = now;
return;
}
Comment on lines +93 to +99
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The division-by-zero check for current_buffer_size is defensive but may hide a deeper issue. The static member current_buffer_size is initialized to 4096 at line 6 and set via initialize(), so it should never be zero in normal operation. If it becomes zero, this indicates a serious bug elsewhere. Consider adding a LOG_ERROR or LOG_WARN here to alert developers that an unexpected state was encountered.

Copilot uses AI. Check for mistakes.

int change_pct = ((int)new_size - (int)current_buffer_size) * 100 / (int)current_buffer_size;

if (abs(change_pct) >= 10)
Expand Down
51 changes: 30 additions & 21 deletions src/config_validator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ class ConfigValidator {
if (strlen(WIFI_SSID) == 0) {
LOG_ERROR("WiFi SSID is empty - must configure WIFI_SSID in config.h");
valid = false;
} else if (strcmp(WIFI_SSID, "YOUR_WIFI_SSID") == 0) {
LOG_ERROR("WiFi SSID is still set to default placeholder - must configure WIFI_SSID in config.h");
valid = false;
} else {
LOG_INFO(" ✓ WiFi SSID configured");
}
Expand All @@ -84,25 +87,28 @@ class ConfigValidator {
if (strlen(WIFI_PASSWORD) == 0) {
LOG_ERROR("WiFi password is empty - must configure WIFI_PASSWORD in config.h");
valid = false;
} else if (strcmp(WIFI_PASSWORD, "YOUR_WIFI_PASSWORD") == 0) {
LOG_ERROR("WiFi password is still set to default placeholder - must configure WIFI_PASSWORD in config.h");
valid = false;
} else {
LOG_INFO(" ✓ WiFi password configured");
}

// Validate retry parameters
if (WIFI_RETRY_DELAY <= 0) {
LOG_WARN("WIFI_RETRY_DELAY is 0 or negative - using default 500ms");
if (WIFI_RETRY_DELAY == 0) {
LOG_WARN("WIFI_RETRY_DELAY is 0 - using default 500ms");
} else {
LOG_INFO(" ✓ WiFi retry delay: %u ms", WIFI_RETRY_DELAY);
}

if (WIFI_MAX_RETRIES <= 0) {
LOG_WARN("WIFI_MAX_RETRIES is 0 or negative - using default 20");
if (WIFI_MAX_RETRIES == 0) {
LOG_WARN("WIFI_MAX_RETRIES is 0 - using default 20");
} else {
LOG_INFO(" ✓ WiFi max retries: %u", WIFI_MAX_RETRIES);
}

if (WIFI_TIMEOUT <= 0) {
LOG_WARN("WIFI_TIMEOUT is 0 or negative - using default 30000ms");
if (WIFI_TIMEOUT == 0) {
LOG_WARN("WIFI_TIMEOUT is 0 - using default 30000ms");
} else {
LOG_INFO(" ✓ WiFi timeout: %u ms", WIFI_TIMEOUT);
}
Expand All @@ -123,6 +129,9 @@ class ConfigValidator {
if (strlen(SERVER_HOST) == 0) {
LOG_ERROR("Server HOST is empty - must configure SERVER_HOST in config.h");
valid = false;
} else if (strcmp(SERVER_HOST, "YOUR_SERVER_IP") == 0) {
LOG_ERROR("Server HOST is still set to default placeholder - must configure SERVER_HOST in config.h");
valid = false;
} else {
LOG_INFO(" ✓ Server HOST configured: %s", SERVER_HOST);
}
Expand All @@ -136,15 +145,15 @@ class ConfigValidator {
}

// Validate reconnection timeouts
if (SERVER_RECONNECT_MIN <= 0) {
if (SERVER_RECONNECT_MIN == 0) {
LOG_WARN("SERVER_RECONNECT_MIN is %u ms - should be > 0", SERVER_RECONNECT_MIN);
} else if (SERVER_RECONNECT_MIN < 1000) {
LOG_WARN("SERVER_RECONNECT_MIN (%u ms) is very short - minimum recommended is 1000ms", SERVER_RECONNECT_MIN);
} else {
LOG_INFO(" ✓ Server reconnect min: %u ms", SERVER_RECONNECT_MIN);
}

if (SERVER_RECONNECT_MAX <= 0) {
if (SERVER_RECONNECT_MAX == 0) {
LOG_WARN("SERVER_RECONNECT_MAX is %u ms - should be > 0", SERVER_RECONNECT_MAX);
} else if (SERVER_RECONNECT_MAX < SERVER_RECONNECT_MIN) {
LOG_ERROR("SERVER_RECONNECT_MAX (%u ms) cannot be less than SERVER_RECONNECT_MIN (%u ms)",
Expand All @@ -154,7 +163,7 @@ class ConfigValidator {
LOG_INFO(" ✓ Server reconnect max: %u ms", SERVER_RECONNECT_MAX);
}

if (TCP_WRITE_TIMEOUT <= 0) {
if (TCP_WRITE_TIMEOUT == 0) {
LOG_WARN("TCP_WRITE_TIMEOUT is %u ms - should be > 0", TCP_WRITE_TIMEOUT);
} else if (TCP_WRITE_TIMEOUT < 1000) {
LOG_WARN("TCP_WRITE_TIMEOUT (%u ms) is very short", TCP_WRITE_TIMEOUT);
Expand All @@ -174,7 +183,7 @@ class ConfigValidator {

LOG_INFO("Checking I2S configuration...");

if (I2S_SAMPLE_RATE <= 0) {
if (I2S_SAMPLE_RATE == 0) {
LOG_ERROR("I2S_SAMPLE_RATE must be > 0, got %u", I2S_SAMPLE_RATE);
valid = false;
} else if (I2S_SAMPLE_RATE < 8000 || I2S_SAMPLE_RATE > 48000) {
Expand All @@ -183,7 +192,7 @@ class ConfigValidator {
LOG_INFO(" ✓ I2S sample rate: %u Hz", I2S_SAMPLE_RATE);
}

if (I2S_BUFFER_SIZE <= 0) {
if (I2S_BUFFER_SIZE == 0) {
LOG_ERROR("I2S_BUFFER_SIZE must be > 0, got %u", I2S_BUFFER_SIZE);
valid = false;
} else if ((I2S_BUFFER_SIZE & (I2S_BUFFER_SIZE - 1)) != 0) {
Expand All @@ -192,7 +201,7 @@ class ConfigValidator {
LOG_INFO(" ✓ I2S buffer size: %u bytes", I2S_BUFFER_SIZE);
}

if (I2S_DMA_BUF_COUNT <= 0) {
if (I2S_DMA_BUF_COUNT == 0) {
LOG_ERROR("I2S_DMA_BUF_COUNT must be > 0, got %u", I2S_DMA_BUF_COUNT);
valid = false;
} else if (I2S_DMA_BUF_COUNT < 2) {
Expand All @@ -201,14 +210,14 @@ class ConfigValidator {
LOG_INFO(" ✓ I2S DMA buffer count: %u", I2S_DMA_BUF_COUNT);
}

if (I2S_DMA_BUF_LEN <= 0) {
if (I2S_DMA_BUF_LEN == 0) {
LOG_ERROR("I2S_DMA_BUF_LEN must be > 0, got %u", I2S_DMA_BUF_LEN);
valid = false;
} else {
LOG_INFO(" ✓ I2S DMA buffer length: %u", I2S_DMA_BUF_LEN);
}

if (I2S_MAX_READ_RETRIES <= 0) {
if (I2S_MAX_READ_RETRIES == 0) {
LOG_WARN("I2S_MAX_READ_RETRIES is %u - should be > 0", I2S_MAX_READ_RETRIES);
} else {
LOG_INFO(" ✓ I2S max read retries: %u", I2S_MAX_READ_RETRIES);
Expand All @@ -226,21 +235,21 @@ class ConfigValidator {

LOG_INFO("Checking timing configuration...");

if (MEMORY_CHECK_INTERVAL <= 0) {
if (MEMORY_CHECK_INTERVAL == 0) {
LOG_WARN("MEMORY_CHECK_INTERVAL is %u ms - should be > 0", MEMORY_CHECK_INTERVAL);
} else if (MEMORY_CHECK_INTERVAL < 5000) {
LOG_WARN("MEMORY_CHECK_INTERVAL (%u ms) is very frequent", MEMORY_CHECK_INTERVAL);
} else {
LOG_INFO(" ✓ Memory check interval: %u ms", MEMORY_CHECK_INTERVAL);
}

if (RSSI_CHECK_INTERVAL <= 0) {
if (RSSI_CHECK_INTERVAL == 0) {
LOG_WARN("RSSI_CHECK_INTERVAL is %u ms - should be > 0", RSSI_CHECK_INTERVAL);
} else {
LOG_INFO(" ✓ RSSI check interval: %u ms", RSSI_CHECK_INTERVAL);
}

if (STATS_PRINT_INTERVAL <= 0) {
if (STATS_PRINT_INTERVAL == 0) {
LOG_WARN("STATS_PRINT_INTERVAL is %u ms - should be > 0", STATS_PRINT_INTERVAL);
} else {
LOG_INFO(" ✓ Stats print interval: %u ms", STATS_PRINT_INTERVAL);
Expand All @@ -258,14 +267,14 @@ class ConfigValidator {

LOG_INFO("Checking memory thresholds...");

if (MEMORY_CRITICAL_THRESHOLD <= 0) {
if (MEMORY_CRITICAL_THRESHOLD == 0) {
LOG_ERROR("MEMORY_CRITICAL_THRESHOLD must be > 0, got %u bytes", MEMORY_CRITICAL_THRESHOLD);
valid = false;
} else {
LOG_INFO(" ✓ Memory critical threshold: %u bytes", MEMORY_CRITICAL_THRESHOLD);
}

if (MEMORY_WARN_THRESHOLD <= 0) {
if (MEMORY_WARN_THRESHOLD == 0) {
LOG_ERROR("MEMORY_WARN_THRESHOLD must be > 0, got %u bytes", MEMORY_WARN_THRESHOLD);
valid = false;
} else {
Expand All @@ -288,7 +297,7 @@ class ConfigValidator {
LOG_INFO(" ✓ RSSI weak threshold: %d dBm", RSSI_WEAK_THRESHOLD);
}

if (MAX_CONSECUTIVE_FAILURES <= 0) {
if (MAX_CONSECUTIVE_FAILURES == 0) {
LOG_WARN("MAX_CONSECUTIVE_FAILURES is %u - should be > 0", MAX_CONSECUTIVE_FAILURES);
} else {
LOG_INFO(" ✓ Max consecutive failures: %u", MAX_CONSECUTIVE_FAILURES);
Expand All @@ -306,7 +315,7 @@ class ConfigValidator {

LOG_INFO("Checking watchdog configuration...");

if (WATCHDOG_TIMEOUT_SEC <= 0) {
if (WATCHDOG_TIMEOUT_SEC == 0) {
LOG_ERROR("WATCHDOG_TIMEOUT_SEC must be > 0, got %u seconds", WATCHDOG_TIMEOUT_SEC);
valid = false;
} else if (WATCHDOG_TIMEOUT_SEC < 30) {
Expand Down
12 changes: 12 additions & 0 deletions src/i2s_audio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ void I2SAudio::cleanup() {
}

bool I2SAudio::readData(uint8_t* buffer, size_t buffer_size, size_t* bytes_read) {
// Validate input parameters
if (buffer == nullptr || bytes_read == nullptr) {
LOG_ERROR("I2S readData: invalid parameters (buffer=%p, bytes_read=%p)", buffer, bytes_read);
return false;
}

if (buffer_size == 0) {
LOG_WARN("I2S readData: buffer_size is 0");
*bytes_read = 0;
return false;
}

if (!is_initialized) {
LOG_ERROR("I2S not initialized");
return false;
Expand Down
6 changes: 6 additions & 0 deletions src/logger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ void Logger::init(LogLevel level)

void Logger::log(LogLevel level, const char *file, int line, const char *fmt, ...)
{
// Validate input parameters
if (file == nullptr || fmt == nullptr)
{
return; // Silently ignore invalid calls to prevent recursion
}

if (level < min_level)
return;

Expand Down
10 changes: 8 additions & 2 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ struct SystemStats {
if (current_heap < min_heap) min_heap = current_heap;

// Detect heap trend (potential memory leak)
if (current_heap < last_heap - 1000) {
// Use signed comparison to avoid underflow with unsigned integers
if (last_heap > current_heap + 1000) {
heap_trend = -1; // Decreasing - potential leak
} else if (current_heap > last_heap + 1000) {
heap_trend = 1; // Increasing - memory recovered
Expand Down Expand Up @@ -118,7 +119,12 @@ void checkMemoryHealth() {
if (free_heap < MEMORY_CRITICAL_THRESHOLD) {
LOG_CRITICAL("Critical low memory: %u bytes - system may crash", free_heap);
// Consider restarting if critically low
if (free_heap < MEMORY_CRITICAL_THRESHOLD / 2) {
// Use a safer threshold calculation
uint32_t emergency_threshold = MEMORY_CRITICAL_THRESHOLD / 2;
if (emergency_threshold == 0) {
emergency_threshold = 1000; // Minimum 1KB emergency threshold
}
if (free_heap < emergency_threshold) {
LOG_CRITICAL("Memory critically low - initiating graceful restart");
gracefulShutdown();
ESP.restart();
Expand Down
14 changes: 11 additions & 3 deletions src/network.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,13 @@ static inline unsigned long apply_jitter(unsigned long base_ms)
int32_t jitter = (int32_t)(r % jitter_span) - jitter_range;

// Apply jitter and bounds-check the result
long with_jitter = (long)base_ms + jitter;
if (with_jitter < (long)SERVER_RECONNECT_MIN)
// Use int64_t to avoid overflow when adding jitter to base_ms
int64_t with_jitter = (int64_t)base_ms + jitter;
if (with_jitter < (int64_t)SERVER_RECONNECT_MIN)
{
with_jitter = SERVER_RECONNECT_MIN;
}
if ((unsigned long)with_jitter > SERVER_RECONNECT_MAX)
if (with_jitter > (int64_t)SERVER_RECONNECT_MAX)
{
with_jitter = SERVER_RECONNECT_MAX;
}
Expand Down Expand Up @@ -346,6 +347,13 @@ WiFiClient &NetworkManager::getClient()

bool NetworkManager::writeData(const uint8_t *data, size_t length)
{
// Validate input parameters
if (data == nullptr || length == 0)
{
LOG_WARN("Invalid write parameters: data=%p, length=%u", data, (unsigned)length);
return false;
}

if (!isServerConnected())
{
return false;
Expand Down
48 changes: 26 additions & 22 deletions src/serial_command.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ void SerialCommandHandler::processCommands() {

// Handle newline (end of command)
if (c == '\r' || c == '\n') {
if (buffer_index > 0) {
if (buffer_index > 0 && buffer_index < BUFFER_SIZE) {
command_buffer[buffer_index] = '\0';
Serial.println(); // Echo newline

Expand All @@ -56,27 +56,25 @@ void SerialCommandHandler::processCommands() {
args = space + 1;
}

if (cmd != nullptr) {
if (strcmp(cmd, "STATUS") == 0) {
handleStatusCommand();
} else if (strcmp(cmd, "CONFIG") == 0) {
handleConfigCommand(args);
} else if (strcmp(cmd, "RESTART") == 0) {
handleRestartCommand();
} else if (strcmp(cmd, "DISCONNECT") == 0) {
handleDisconnectCommand();
} else if (strcmp(cmd, "CONNECT") == 0) {
handleConnectCommand();
} else if (strcmp(cmd, "STATS") == 0) {
handleStatsCommand();
} else if (strcmp(cmd, "HEALTH") == 0) {
handleHealthCommand();
} else if (strcmp(cmd, "HELP") == 0) {
handleHelpCommand();
} else {
LOG_ERROR("Unknown command: %s", cmd);
handleHelpCommand();
}
if (strcmp(cmd, "STATUS") == 0) {
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The removal of the if (cmd != nullptr) check before using cmd in strcmp() introduces a potential null pointer dereference. Although cmd is initialized to command_buffer at line 51, if the buffer is empty or contains only whitespace after the null termination at line 55, cmd could theoretically point to a null-terminated empty string. However, the real issue is that this code is only reached when buffer_index > 0, so cmd will always point to valid memory. The removal is safe but the original defensive check was good practice.

Copilot uses AI. Check for mistakes.
handleStatusCommand();
} else if (strcmp(cmd, "CONFIG") == 0) {
handleConfigCommand(args);
} else if (strcmp(cmd, "RESTART") == 0) {
handleRestartCommand();
} else if (strcmp(cmd, "DISCONNECT") == 0) {
handleDisconnectCommand();
} else if (strcmp(cmd, "CONNECT") == 0) {
handleConnectCommand();
} else if (strcmp(cmd, "STATS") == 0) {
handleStatsCommand();
} else if (strcmp(cmd, "HEALTH") == 0) {
handleHealthCommand();
} else if (strcmp(cmd, "HELP") == 0) {
handleHelpCommand();
} else {
LOG_ERROR("Unknown command: %s", cmd);
handleHelpCommand();
}

clearBuffer();
Expand All @@ -88,6 +86,12 @@ void SerialCommandHandler::processCommands() {
if (buffer_index < BUFFER_SIZE - 1) {
command_buffer[buffer_index++] = c;
Serial.write(c); // Echo character
} else {
// Buffer full - warn user and ignore character
if (buffer_index == BUFFER_SIZE - 1) {
LOG_WARN("Command buffer full - command too long");
clearBuffer();
}
Comment on lines +91 to +94
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The warning message is only logged once when buffer_index == BUFFER_SIZE - 1, but if more characters continue to arrive, they will be silently ignored without further warning. After clearBuffer() resets buffer_index to 0, subsequent characters will be accepted again, potentially creating confusion. Consider moving the warning outside the inner if condition or tracking whether the warning has been shown.

Suggested change
if (buffer_index == BUFFER_SIZE - 1) {
LOG_WARN("Command buffer full - command too long");
clearBuffer();
}
LOG_WARN("Command buffer full - command too long");
clearBuffer();

Copilot uses AI. Check for mistakes.
}
}

Expand Down
1 change: 0 additions & 1 deletion src/serial_command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class SerialCommandHandler {
static void printStatus();
static void printHealth();
static void clearBuffer();
static char* getNextToken(char* str, const char* delim);
};

#endif // SERIAL_COMMAND_H