Web server code Review
1. Resource Management Issues
Bug: Memory Leak in File Handlers
- Both `root_get_handler` and `static_file_get_handler` don't close files in all error paths
- If `httpd_resp_send_chunk` fails after file is opened, file handle leaks
Fix:
FILE *file = fopen(filepath, "rb");
if (!file) {
// ... error handling ...
}
// Add goto or cleanup block for all error paths
Bug: Double-Free Risk in Config Handler
- `get_config_handler` frees `config_json_data` but doesn't null the pointer afterward
- If function is called again with error between free and null, could double-free
Fix:
free(config_json_data);
config_json_data = NULL; // Immediately null after free
2. Security Vulnerabilities
Bug: Directory Traversal Risk
- `static_file_get_handler` concatenates URI directly to base path
- Malicious request like `../../../secret_file` could access unauthorized files
Fix:
// Add path sanitization:
if (strstr(uri, "..") != NULL) {
httpd_resp_send_err(req, HTTPD_403_FORBIDDEN, "Path traversal not allowed");
return ESP_FAIL;
}
Bug: No POST Size Validation
- `update_config_post_handler` checks length but doesn't verify minimum valid size
- Empty or very small configs might pass length check but fail parsing
Fix:
if (total_len <= MIN_VALID_CONFIG_SIZE) { // Define appropriate minimum
httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Config too small");
return ESP_FAIL;
}
3. Error Handling Issues
Bug: Unchecked System Calls
- `fread`, `fclose` return values not checked in file handlers
- Could silently fail on filesystem errors
Fix:
if (fread(chunk, 1, sizeof(chunk), file) != sizeof(chunk)) {
if (ferror(file)) {
// Handle read error
}
}
Bug: Timer Resource Leak
- In `update_config_post_handler`, timer handle isn't deleted if start fails
- Could leak timer resources on repeated failures
Fix:
if (timer_err == ESP_OK) {
timer_err = esp_timer_start_once(restart_timer, 500 * 1000);
if (timer_err != ESP_OK) {
esp_timer_delete(restart_timer); // Clean up if start fails
}
}
4. Operational Issues
Bug: Wildcard Route Conflicts
- The `/*` wildcard handler is registered last but might intercept API routes
- If route registration order changes, static handler could override API endpoints
Fix:
// Register static handler with more specific pattern:
httpd_uri_t static_uri = {
.uri = "/static/*", // Or other appropriate prefix
// ...
};
Bug: No Connection Abort Handling
- File handlers don't check if client disconnected during large file transfers
- Wastes resources sending data to disconnected clients
Fix:
if (httpd_req_get_hdr_value_len(req, "Connection") == 0) {
// Client disconnected
fclose(file);
return ESP_FAIL;
}
5. Memory Safety Issues
Bug: Potential Buffer Overflow
- MAC address string in `get_device_id_handler` has tight 18-byte buffer
- `snprintf` protection good, but no validation of MAC read operation
Fix:
char mac_str[24]; // Extra padding
// Check esp_read_mac return value
Bug: Uninitialized Struct Members
- `temp_config` in `update_config_post_handler` is zero-initialized but...
- No guarantee all members are properly set before use
Fix:
device_config_t temp_config = {
.wifi_ssid = NULL,
.wifi_password = NULL,
// Explicitly initialize all members
};
6. HTTP Protocol Issues
Bug: Missing Content-Length Header
- File handlers don't set Content-Length header
- Clients can't show progress for large files
Fix:
fseek(file, 0, SEEK_END);
long fsize = ftell(file);
fseek(file, 0, SEEK_SET);
httpd_resp_set_hdr(req, "Content-Length", fsize);
Bug: No Cache Control Headers
- Static files served without caching directives
- Could cause unnecessary retransmission of unchanged files
Fix:
httpd_resp_set_hdr(req, "Cache-Control", "public, max-age=3600");
7. Configuration Issues
Bug: Hardcoded Paths
- `/storage/web` path hardcoded in multiple places
- Makes testing and configuration changes difficult
Fix:
// Define in header:
#define WEB_ROOT_PATH CONFIG_WEB_ROOT_PATH
// Then use:
snprintf(filepath, sizeof(filepath), "%s%s", WEB_ROOT_PATH, uri);
8. Logging Issues
Bug: Sensitive Data Exposure
- Config POST handler logs full received JSON
- Could expose passwords in debug logs
Fix:
ESP_LOGD(TAG, "Received %d bytes of config data", total_len);
// Instead of:
// ESP_LOGD(TAG, "Received POST JSON data: %s", buf);
Recommended Improvements:
1. Add Connection Pooling
// For frequent requests, reuse connections
httpd_config_t config = {
.enable_so_linger = true,
.linger_timeout = 5,
.keep_alive_enable = true
};
2. Implement Rate Limiting
// Prevent abuse of API endpoints
static uint32_t last_request_time = 0;
if (xTaskGetTickCount() - last_request_time < MIN_REQUEST_INTERVAL) {
httpd_resp_send_err(req, HTTPD_429_TOO_MANY_REQUESTS, "Slow down");
return ESP_FAIL;
}
last_request_time = xTaskGetTickCount();
3. Add Request Validation
// Verify expected content type
if (httpd_req_get_hdr_value_len(req, "Content-Type") == 0 ||
strstr(httpd_req_get_hdr_value_str(req, "Content-Type"), "application/json") == NULL) {
httpd_resp_send_err(req, HTTPD_415_UNSUPPORTED_MEDIA_TYPE, "JSON required");
return ESP_FAIL;
}
Potential logical errors that could lead to incorrect behavior:
1. Race Condition in Restart Sequence
Location: `update_config_post_handler` → `restart_timer_callback`
Issue:
static void restart_timer_callback(void *arg) {
stop_mdns_service();
stop_webserver(server); // Uses global 'server' variable
server = NULL; // Modifies global
stop_wifi_ap();
deinit_storage();
esp_restart();
}
Problem:
- The global `server` variable is accessed and modified without synchronization
- If another thread accesses `server` during this sequence, it could lead to:
- Use-after-free if server is stopped but pointer isn't yet NULL
- Memory leaks if new server starts during shutdown
Fix:
static void restart_timer_callback(void *arg) {
httpd_handle_t local_server = server; // Take local copy
server = NULL; // Atomic clear first
stop_mdns_service();
stop_webserver(local_server); // Use local copy
stop_wifi_ap();
deinit_storage();
esp_restart();
}
2. Incorrect MIME Type Detection
Location: `static_file_get_handler`
Issue:
if (strstr(uri, ".html")) { // Problematic check
MimeType = "text/html";
}
Problem:
- Simple `strstr` could match `.html` in middle of filename (e.g., `file.html.backup`)
- No length check for extension matching
- Could serve JS/CSS without proper MIME types
Fix:
const char *ext = strrchr(uri, '.'); // Find last dot
if (ext) {
if (strcmp(ext, ".html") == 0) MimeType = "text/html";
else if (strcmp(ext, ".js") == 0) MimeType = "application/javascript";
// ... other exact matches
}
3. Faulty Connection State Handling
Location: All file transfer handlers
Issue:
do {
chunksize = fread(chunk, 1, sizeof(chunk), file);
if (chunksize > 0) {
ret = httpd_resp_send_chunk(req, chunk, chunksize);
// Missing client disconnect check
}
} while (chunksize > 0);
Problem:
- No verification if client is still connected during long file transfers
- Wastes resources sending data to disconnected clients
- ESP-HTTPD may not immediately detect closed connections
Fix:
do {
chunksize = fread(chunk, 1, sizeof(chunk), file);
if (chunksize > 0) {
if (httpd_req_get_hdr_value_len(req, "Connection") == 0) {
// Client disconnected
fclose(file);
return ESP_FAIL;
}
ret = httpd_resp_send_chunk(...);
// ... existing checks
}
} while (chunksize > 0);
4. Incomplete JSON Validation
Location: `update_config_post_handler`
Issue:
esp_err_t validation_result = parse_device_config(buf, &temp_config);
if (validation_result != ESP_OK) {
httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Invalid config");
// Jumps to cleanup but temp_config may contain partial data
}
Problem:
- If validation fails after some fields are parsed but before others:
- `temp_config` may contain allocated strings that need cleanup
- Current flow might leak memory
Fix:
if (validation_result != ESP_OK) {
// Ensure all possible allocations are freed
free(temp_config.wifi_ssid);
free(temp_config.wifi_password);
// ... other fields
memset(&temp_config, 0, sizeof(temp_config));
httpd_resp_send_err(...);
free(buf);
return ESP_FAIL;
}
5. Timer Resource Leak
Location: `update_config_post_handler`
Issue:
esp_timer_handle_t restart_timer;
esp_timer_create(&timer_args, &restart_timer);
esp_timer_start_once(restart_timer, 500 * 1000);
// Timer never deleted if start fails or after execution
Problem:
- Timer object leaks if:
- `esp_timer_start_once` fails
- After callback executes
- Could exhaust timer resources over time
Fix:
// In callback:
static void restart_timer_callback(void *arg) {
esp_timer_handle_t timer = (esp_timer_handle_t)arg;
esp_timer_delete(timer); // Delete self first
// ... rest of shutdown logic
}
// In handler:
esp_timer_create(&timer_args, &restart_timer);
esp_timer_start_once(restart_timer, 500 * 1000);
6. Incorrect Error Propagation
Location: `root_get_handler`/`static_file_get_handler`
Issue:
if (ret != ESP_OK) {
fclose(file);
return ret; // Returns raw esp_err_t
}
Problem:
- HTTPD expects specific HTTP error codes
- Directly returning ESP_FAIL (1) would send incorrect HTTP 500
Fix:
if (ret != ESP_OK) {
fclose(file);
httpd_resp_send_err(req, HTTPD_500_INTERNAL_SERVER_ERROR, "File send error");
return ESP_FAIL;
}
7. Buffer Size Miscalculation
Location: `static_file_get_handler`
Issue:
if (base_len + uri_len + 1 > sizeof(filepath))
Problem:
- Doesn't account for possible path separator between base and URI
- Could overflow by 1 byte if exact length match
Fix:
// Account for separator and null terminator
if ((base_len + uri_len + 2) > sizeof(filepath)) {
// +2 for '/' and null terminator
}
// And in concatenation:
snprintf(filepath, sizeof(filepath), "%s/%s", base_path, uri);
8. Incorrect Cleanup Order
Location: `restart_timer_callback`
Issue:
stop_webserver(server);
server = NULL;
stop_wifi_ap();
Problem:
- Web server stopped before WiFi
- Clients may get disconnected abruptly mid-request
- Better to stop accepting connections first
Fix:
server = NULL; // Prevent new connections
stop_wifi_ap(); // Disconnect clients gracefully
stop_webserver(server); // Now safely stop
9. Missing Atomic Operations
Location: Global `server` variable access
Issue: Multiple functions read/write `server` without protection:
httpd_handle_t start_webserver(void) {
if (server != NULL) // Unsafe read
Problem:
- Race condition if server starts/stops from different tasks
- Could lead to double-starts or use-after-free
Fix:
static portMUX_TYPE server_spinlock = portMUX_INITIALIZER_UNLOCKED;
httpd_handle_t start_webserver(void) {
portENTER_CRITICAL(&server_spinlock);
httpd_handle_t local_server = server;
portEXIT_CRITICAL(&server_spinlock);
if (local_server != NULL) {
return local_server;
}
// ... rest of init
}
