Conversation
Summary of ChangesHello @tavdog, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring effort by migrating the underlying HUB75 display driver library. This change impacts display initialization, rendering logic, and build configurations, aiming to leverage the capabilities of the new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the display driver from esp-hub75 to ESP32-HUB75-MatrixPanel-I2S-DMA. However, it introduces several critical security vulnerabilities, including potential remote memory corruption in display_draw when processing WebP images with malicious canvas dimensions, insecure OTA updates over HTTP, and a potential null pointer dereference in the graphics loop. Furthermore, the current implementation introduces significant performance and memory regressions due to inefficient pixel-by-pixel drawing, and includes critical bugs such as a potential race condition and a typo that will break the build. These issues must be addressed to ensure both the stability and security of the device.
I am having trouble creating individual review comments. Click here to see my feedback.
main/display.cpp (256-270)
The display_draw function has a critical security vulnerability: it fails to validate width and height from WebP images, leading to potential out-of-bounds writes to the display's internal buffer or other memory regions. Negative width or height can also cause excessive loops and out-of-bounds reads/writes. Additionally, this pixel-by-pixel drawing approach is a significant performance regression; consider using bulk drawing methods like drawRGBBitmap() from the ESP32-HUB75-MatrixPanel-I2S-DMA library.
main/main.c (41)
The volatile keyword has been removed from the declaration of isAnimating. This global variable is used to communicate between different tasks (e.g., the main task and the gfx_loop task) to control animation state. Without volatile, the compiler might optimize away reads of the variable within a loop, assuming its value doesn't change from within that loop's scope. This can lead to a race condition where one task updates the flag, but the other task never sees the change.
To prevent this bug, the variable must be declared as volatile.
volatile int32_t isAnimating = 1;main/sntp.c (31)
The function esp_sntp_servermode_dhcp has been incorrectly changed to sntp_servermode_dhcp. The latter function does not exist in the ESP-IDF SNTP API, and this will cause a compilation error.
esp_sntp_servermode_dhcp(1);
sdkconfig.defaults (9)
Enabling CONFIG_ESP_HTTPS_OTA_ALLOW_HTTP allows the device to perform firmware updates over unencrypted HTTP. This exposes the device to Man-in-the-Middle (MitM) attacks where an attacker can intercept the update request and serve a malicious firmware image, potentially gaining full control over the device.
main/gfx.c (67-78)
This change introduces a performance and memory regression. The previous implementation correctly used a direct pointer to the static boot animation asset stored in flash. This new implementation allocates a buffer on the heap and copies the asset data into it.
This is inefficient as it consumes heap memory unnecessarily and adds the overhead of a memcpy at startup. Please revert to the previous, more efficient approach of using the static asset directly.
main/gfx.c (329-349)
This function now allocates heap memory and copies static asset data (like the config screen or 404 image) before passing it to gfx_update. The previous implementation was more efficient as it passed a direct pointer to the static data in flash and used the is_static_asset helper to avoid freeing it.
This change introduces unnecessary memory allocations and copies for all static assets, increasing heap usage and fragmentation. It would be better to restore the previous logic of handling static and dynamic buffers differently.
main/display.cpp (288-292)
The display_fill_rect function lacks bounds checking for x, y, w, and h, which can lead to memory corruption if drawing outside the valid display area. This is a dangerous omission for an exported graphics function. Furthermore, the current pixel-by-pixel drawing is inefficient; consider using the optimized fillRect() method from the ESP32-HUB75-MatrixPanel-I2S-DMA library.
void display_fill_rect(int x, int y, int w, int h, uint8_t r, uint8_t g,
uint8_t b) {
if (_matrix != NULL) {
_matrix->fillRect(x, y, w, h, r, g, b);
}
}main/gfx.c (478-486)
A critical security issue exists where WebPAnimDecoderGetNext's return value is not checked. If decoding fails, pix could be NULL, leading to a null pointer dereference in display_draw and a Denial of Service (DoS) with malformed WebP images. Additionally, the vTaskDelay(10) logic is arbitrary and less portable than pdMS_TO_TICKS(1) or vTaskDelay(1) for yielding the CPU.
main/display.cpp (196-198)
In modern C++, new throws a std::bad_alloc exception on allocation failure rather than returning NULL (unless std::nothrow is used). Therefore, this null check is redundant and will never be true. The comment on line 197 even correctly points this out. You can remove this check to simplify the code and rely on exception handling for allocation failures.
if (!_matrix->begin()) {
ESP_LOGE(TAG, "MatrixPanel_I2S_DMA begin() failed");
delete _matrix;
_matrix = NULL;
return 1;
}sdkconfig.defaults.matrixportal-s3 (3)
The configuration CONFIG_HUB75_CLK_8MHZ belongs to the esp-hub75 library, which has been removed in this pull request. This setting no longer has any effect and should be removed to avoid confusion. The clock speed for the new library is configured directly in main/display.cpp.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the HUB75 display driver from esp-hub75 to the more common ESP32-HUB75-MatrixPanel-I2S-DMA library. While this is a significant step, critical security and stability issues were identified. Specifically, a signed/unsigned comparison in display_draw could lead to out-of-bounds reads, and a missing NULL check in display_initialize could cause a crash if memory allocation fails. Additionally, the new implementation introduces performance bottlenecks due to inefficient pixel-by-pixel drawing, hardcodes display configuration parameters reducing flexibility, and has missing header includes that could lead to build failures. There's also a minor inconsistency in refactoring is_static_asset checks. These issues should be addressed to ensure stability, performance, and robustness.
| for (unsigned int i = 0; i < height; i++) { | ||
| for (unsigned int j = 0; j < width; j++) { | ||
| const uint8_t *p = &pix[(i * width + j) * channels]; | ||
| uint8_t r = p[ixR]; | ||
| uint8_t g = p[ixG]; | ||
| uint8_t b = p[ixB]; |
There was a problem hiding this comment.
The display_draw function has a critical vulnerability: using unsigned int loop counters i and j compared against int parameters height and width can lead to out-of-bounds reads and device crashes if negative dimensions are provided. Additionally, there's a lack of validation for channels, ixR, ixG, and ixB parameters. Beyond this security concern, the current pixel-by-pixel drawing implementation with drawPixelRGB888 is highly inefficient and will cause performance bottlenecks. Consider using the ESP32-HUB75-MatrixPanel-I2S-DMA library's drawRGB888Bitmap(int16_t x, int16_t y, uint8_t *bitmap, int16_t w, int16_t h) for bulk drawing to improve performance.
| @@ -1,238 +1,205 @@ | |||
| #include "display.h" | |||
There was a problem hiding this comment.
This file uses logging functions like ESP_LOGI and ESP_LOGE, but it doesn't include esp_log.h. While this might compile due to transitive includes, it's best practice to explicitly include headers for the functionality you use. Please add #include "esp_log.h".
#include "display.h"
#include "esp_log.h"| // Draw pixel(s) based on scale | ||
| for (int sy = 0; sy < scale; sy++) { | ||
| for (int sx = 0; sx < scale; sx++) { | ||
| int px = cursor_x + (col * scale) + sx; | ||
| int py = cursor_y + (row * scale) + sy; | ||
|
|
||
| // Check bounds | ||
| if (px >= 0 && px < WIDTH && py >= 0 && py < HEIGHT) { | ||
| _matrix->drawPixelRGB888(px, py, r, g, b); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Drawing scaled text is done pixel-by-pixel, which is inefficient. The previous implementation used _matrix->fill for this. Once display_fill_rect is optimized to use the library's fillRectRGB888 function, you can use it here to draw scaled characters more efficiently.
int px = cursor_x + (col * scale);
int py = cursor_y + (row * scale);
if (scale > 1) {
_matrix->fillRectRGB888(px, py, scale, scale, r, g, b);
} else {
// Check bounds
if (px >= 0 && px < WIDTH && py >= 0 && py < HEIGHT) {
_matrix->drawPixelRGB888(px, py, r, g, b);
}
}| @@ -1,4 +1,3 @@ | |||
| #include <esp_heap_caps.h> | |||
There was a problem hiding this comment.
The include for esp_heap_caps.h was removed, but heap_caps_get_largest_free_block is still used in gfx_initialize. This can lead to compilation errors. Please re-add this include.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the display driver from esp-hub75 to ESP32-HUB75-MatrixPanel-I2S-DMA. A high-severity vulnerability was identified in the WebP decoding logic, where an uninitialized pointer dereference could lead to a device crash if decoding fails, potentially exploitable by malicious WebP files. Several drawing functions in display.cpp exhibit reduced performance due to inefficient pixel-by-pixel drawing, which may impact animation frame rates. Additionally, there's a dependency downgrade for espressif/esp_websocket_client that requires review, and a recommendation to use pdMS_TO_TICKS() for vTaskDelay for clarity and portability.
| for (unsigned int i = 0; i < height; i++) { | ||
| for (unsigned int j = 0; j < width; j++) { | ||
| const uint8_t *p = &pix[(i * width + j) * channels]; | ||
| uint8_t r = p[ixR]; | ||
| uint8_t g = p[ixG]; | ||
| uint8_t b = p[ixB]; | ||
|
|
||
| // Draw each pixel scaled up (2x2 pixels for each original pixel) | ||
| for (int sy = 0; sy < scale; sy++) { | ||
| for (int sx = 0; sx < scale; sx++) { | ||
| _matrix->drawPixelRGB888(j * scale + sx, i * scale + sy, r, g, b); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation of display_draw iterates over each pixel and makes a drawPixelRGB888 call, which is inefficient, especially for animations and scaled content. This can lead to significant performance issues like low frame rates. A more performant approach would be to prepare a frame buffer with the final pixel data (in RGB565 format) and then use a bulk drawing function like drawRGBBitmap to send it to the display in one go. This minimizes function call overhead and is better suited for animation.
| for (int iy = y; iy < y + h; iy++) { | ||
| for (int ix = x; ix < x + w; ix++) { | ||
| _matrix->drawPixelRGB888(ix, iy, r, g, b); | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation of display_fill_rect calls drawPixelRGB888 inside a nested loop. The drawPixelRGB888 function internally performs a color conversion from RGB888 to RGB565 on every call. This means the same color conversion is repeated for every pixel in the rectangle, which is inefficient. A more performant approach is to use the library's fillRect method, which will be faster as it avoids this repeated work and may have other internal optimizations.
_matrix->fillRect(x, y, w, h, _matrix->color565(r, g, b));| // Draw pixel(s) based on scale | ||
| for (int sy = 0; sy < scale; sy++) { | ||
| for (int sx = 0; sx < scale; sx++) { | ||
| int px = cursor_x + (col * scale) + sx; | ||
| int py = cursor_y + (row * scale) + sy; | ||
|
|
||
| // Check bounds | ||
| if (px >= 0 && px < WIDTH && py >= 0 && py < HEIGHT) { | ||
| _matrix->drawPixelRGB888(px, py, r, g, b); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The previous implementation had an optimization to use a fill operation for scaled text (scale > 1). This has been replaced by nested loops that call drawPixelRGB888 for each sub-pixel of a scaled character pixel. This is inefficient as it involves many function calls and repeated color conversions. Reintroducing the optimization using the new library's fillRect method would significantly improve performance when drawing scaled text.
int px = cursor_x + (col * scale);
int py = cursor_y + (row * scale);
if (scale > 1) {
// Optimize scaled text using fillRect for better performance
_matrix->fillRect(px, py, scale, scale, _matrix->color565(r, g, b));
} else {
// Draw single pixel, checking bounds
if (px >= 0 && px < WIDTH && py >= 0 && py < HEIGHT) {
_matrix->drawPixelRGB888(px, py, r, g, b);
}
}| // First frame or no delay: yield briefly to let other tasks run | ||
| vTaskDelay(pdMS_TO_TICKS(1)); | ||
| lastWakeTime = xTaskGetTickCount(); | ||
| vTaskDelay(10); // small delay for yield. |
There was a problem hiding this comment.
The argument to vTaskDelay is in FreeRTOS ticks, not milliseconds. Using a magic number like 10 makes the code dependent on the system's tick rate (FREERTOS_HZ). For clarity and portability, it's best practice to use the pdMS_TO_TICKS() macro to convert milliseconds to ticks.
vTaskDelay(pdMS_TO_TICKS(10)); // small delay for yield.
2 participants
No description provided.