fix(zigbee): memory leak issue with malloc #11196

SuGlider · 2025-03-28T14:01:14Z

Description of Change

In libraries/Zigbee/src/ZigbeeEP.cpp file, void ZigbeeEP::zbReadBasicCluster(const esp_zb_zcl_attribute_t *attribute) has a memory leak due to a malloc() with no free().

The same is applied to bool ZigbeeEP::setManufacturerAndModel(const char *name, const char *model) with necessary NULL test of basic_cluster.

Changed code to use std::vector<char> whenever possible.

Tests scenarios

CI only.

Related links

None

github-actions · 2025-03-28T14:14:30Z

	Messages
📖	🎉 Good Job! All checks are passing!

👋 Hello SuGlider, we appreciate your contribution to this project!

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against d09c3ee

github-actions · 2025-03-28T14:25:24Z

Memory usage test (comparing PR against master branch)

The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.

Memory	FLASH [bytes]		FLASH [%]		RAM [bytes]		RAM [%]
Target	DEC	INC	DEC	INC	DEC	INC	DEC	INC
ESP32S3	0	⚠️ +392	0.00	⚠️ +0.06	0	0	0.00	0.00
ESP32S2	0	⚠️ +400	0.00	⚠️ +0.07	0	0	0.00	0.00
ESP32C3	0	⚠️ +376	0.00	⚠️ +0.06	0	0	0.00	0.00
ESP32C6	💚 -4	⚠️ +380	0.00	⚠️ +0.07	0	0	0.00	0.00
ESP32H2	💚 -4	⚠️ +380	0.00	⚠️ +0.07	0	0	0.00	0.00
ESP32	0	⚠️ +524	0.00	⚠️ +0.08	0	0	0.00	0.00

Click to expand the detailed deltas report [usage change in BYTES]

Target	ESP32S3		ESP32S2		ESP32C3		ESP32C6		ESP32H2		ESP32
Example	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM
libraries/Zigbee/examples/Zigbee_Color_Dimmer_Switch	⚠️ +388	0	⚠️ +396	0	⚠️ +364	0	⚠️ +376	0	⚠️ +362	0	⚠️ +512	0
libraries/Zigbee/examples/Zigbee_Gateway	⚠️ +332	0	⚠️ +388	0	⚠️ +366	0	-	-	-	-	⚠️ +428	0
libraries/Zigbee/examples/Zigbee_On_Off_Switch	⚠️ +388	0	⚠️ +400	0	⚠️ +358	0	⚠️ +356	0	⚠️ +368	0	⚠️ +500	0
libraries/Zigbee/examples/Zigbee_Range_Extender	⚠️ +392	0	⚠️ +400	0	⚠️ +376	0	⚠️ +374	0	⚠️ +364	0	⚠️ +524	0
libraries/Zigbee/examples/Zigbee_Thermostat	⚠️ +388	0	⚠️ +400	0	⚠️ +364	0	⚠️ +362	0	⚠️ +372	0	⚠️ +520	0
libraries/Zigbee/examples/Zigbee_Analog_Input_Output	-	-	-	-	-	-	⚠️ +372	0	⚠️ +370	0	-	-
libraries/Zigbee/examples/Zigbee_CarbonDioxide_Sensor	-	-	-	-	-	-	⚠️ +360	0	⚠️ +370	0	-	-
libraries/Zigbee/examples/Zigbee_Color_Dimmable_Light	-	-	-	-	-	-	⚠️ +380	0	⚠️ +364	0	-	-
libraries/Zigbee/examples/Zigbee_Contact_Switch	-	-	-	-	-	-	⚠️ +366	0	⚠️ +374	0	-	-
libraries/Zigbee/examples/Zigbee_Dimmable_Light	-	-	-	-	-	-	⚠️ +366	0	⚠️ +376	0	-	-
libraries/Zigbee/examples/Zigbee_Illuminance_Sensor	-	-	-	-	-	-	⚠️ +366	0	⚠️ +370	0	-	-
libraries/Zigbee/examples/Zigbee_OTA_Client	-	-	-	-	-	-	⚠️ +370	0	⚠️ +364	0	-	-
libraries/Zigbee/examples/Zigbee_Occupancy_Sensor	-	-	-	-	-	-	⚠️ +362	0	⚠️ +372	0	-	-
libraries/Zigbee/examples/Zigbee_On_Off_Light	-	-	-	-	-	-	⚠️ +362	0	⚠️ +368	0	-	-
libraries/Zigbee/examples/Zigbee_Pressure_Flow_Sensor	-	-	-	-	-	-	⚠️ +374	0	⚠️ +376	0	-	-
libraries/Zigbee/examples/Zigbee_Scan_Networks	-	-	-	-	-	-	💚 -4	0	💚 -4	0	-	-
libraries/Zigbee/examples/Zigbee_Temp_Hum_Sensor_Sleepy	-	-	-	-	-	-	⚠️ +370	0	⚠️ +364	0	-	-
libraries/Zigbee/examples/Zigbee_Temperature_Sensor	-	-	-	-	-	-	⚠️ +368	0	⚠️ +372	0	-	-
libraries/Zigbee/examples/Zigbee_Vibration_Sensor	-	-	-	-	-	-	⚠️ +374	0	⚠️ +368	0	-	-
libraries/Zigbee/examples/Zigbee_Wind_Speed_Sensor	-	-	-	-	-	-	⚠️ +360	0	⚠️ +370	0	-	-
libraries/Zigbee/examples/Zigbee_Window_Covering	-	-	-	-	-	-	⚠️ +374	0	⚠️ +380	0	-	-

lucasssvaz

VLAs like char zb_name[name_length + 2] are considered Undefined Behavior in C++ (although GCC supports it as an extension).

Better just to add the missing free to avoid the memory leak.

github-actions · 2025-03-28T14:30:43Z

Test Results

76 files 76 suites 12m 48s ⏱️
38 tests 38 ✅ 0 💤 0 ❌
241 runs 241 ✅ 0 💤 0 ❌

Results for commit d09c3ee.

♻️ This comment has been updated with latest results.

SuGlider · 2025-03-28T14:41:17Z

VLAs like char zb_name[name_length + 2] are considered Undefined Behavior in C++

In such case, using std::vector<char> declaration is even better in that case. No need to consider free().

SuGlider · 2025-03-28T14:42:53Z

~~GCC supporting it is important. I would prefer allocating memory for a few bytes in Stack instead of Heap. Reduces memory fragmentation and it is faster.~~

let's go with C++ way. Safer.
Reviewing the whole _name and _model functionality. More fixes to make sure it doesn't leak memory.

…alization

libraries/Zigbee/src/ZigbeeEP.cpp

SuGlider · 2025-03-28T16:04:17Z

@P-R-O-C-H-Y - I can't test it here to make sure that it works. Please test it using HA and the Zigbee dongle. Thanks!

SuGlider · 2025-03-28T16:38:13Z

Reviewing it over again... code change is wrong.

SuGlider · 2025-03-28T16:41:52Z

Sorry guys. Current code is 100% fine. No issues. My bad.

me-no-dev · 2025-03-28T21:18:52Z

libraries/Zigbee/src/ZigbeeEP.cpp

-  if (_read_manufacturer != nullptr) {
-    free(_read_manufacturer);
-  }
+  free(_read_manufacturer);  // CPP tests it for nullptr


what does the comment mean?

me-no-dev · 2025-03-28T21:19:19Z

libraries/Zigbee/src/ZigbeeEP.cpp

-  char *zb_name = new char[name_length + 2];
-  char *zb_model = new char[model_length + 2];
+  std::vector<char> zb_name(name_length + 2);
+  std::vector<char> zb_model(model_length + 2);


what will happen if there was not enough memory?

me-no-dev · 2025-03-28T21:20:33Z

libraries/Zigbee/src/ZigbeeEP.cpp

+    zb_manufacturer[zbstr->len] = '\0';
+    log_i("Peer Manufacturer is \"%s\"", zb_manufacturer.data());
+    free(_read_manufacturer);  // Free any previously allocated memory
+    _read_manufacturer = strdup(zb_manufacturer.data());  // Duplicate the information for persistent storage


strdup will return NULL if there was not enough memory

fix(zigbee): memory leak issue with malloc

4211b57

SuGlider self-assigned this Mar 28, 2025

fix(zigbee): test basic_cluster value for null to avoid crash

cb581f3

SuGlider added the Area: Zigbee Issues and Feature Request about Zigbee label Mar 28, 2025

SuGlider added this to the 3.2.0 milestone Mar 28, 2025

lucasssvaz requested changes Mar 28, 2025

View reviewed changes

SuGlider added 2 commits March 28, 2025 12:04

feat(zigbee): use stack memory for name and model

031f117

feat(zigbee): use std::vector for memory allocation and explicit init…

f921e2a

…alization

me-no-dev reviewed Mar 28, 2025

View reviewed changes

libraries/Zigbee/src/ZigbeeEP.cpp Outdated Show resolved Hide resolved

SuGlider added 5 commits March 28, 2025 12:49

fix(zigbee): using std::vector<char> and fix memory leak

7d8cb69

fix(zigbe): removing unecessary empty line

d3a1c5c

fix(zigbee): fixing TAB identation

2a21a13

fix(zigbee): unecessary duplicated comment

c7e7387

fix(zigbee): not using reserved word string

558747d

SuGlider requested review from me-no-dev, lucasssvaz and P-R-O-C-H-Y March 28, 2025 15:59

SuGlider added 2 commits March 28, 2025 13:05

fix(zigbee): keeps the same prefix style along the code

0540823

fix(zigbee): missing char * data() for logging

d09c3ee

SuGlider added the Status: Review needed Issue or PR is awaiting review label Mar 28, 2025

SuGlider marked this pull request as draft March 28, 2025 16:37

SuGlider removed the Status: Review needed Issue or PR is awaiting review label Mar 28, 2025

SuGlider closed this Mar 28, 2025

SuGlider deleted the zigbee_memory_leak branch March 28, 2025 16:56

SuGlider mentioned this pull request Mar 28, 2025

fix(zigbeeEP): review of names and memory allocation #11199

Open

me-no-dev reviewed Mar 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(zigbee): memory leak issue with malloc #11196

fix(zigbee): memory leak issue with malloc #11196

SuGlider commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

lucasssvaz left a comment

github-actions bot commented Mar 28, 2025 •

edited

Loading

SuGlider commented Mar 28, 2025

SuGlider commented Mar 28, 2025 •

edited

Loading

SuGlider commented Mar 28, 2025

SuGlider commented Mar 28, 2025

SuGlider commented Mar 28, 2025

me-no-dev Mar 28, 2025

me-no-dev Mar 28, 2025

me-no-dev Mar 28, 2025

fix(zigbee): memory leak issue with malloc #11196

fix(zigbee): memory leak issue with malloc #11196

Conversation

SuGlider commented Mar 28, 2025 • edited Loading

Description of Change

Tests scenarios

Related links

github-actions bot commented Mar 28, 2025 • edited Loading

github-actions bot commented Mar 28, 2025 • edited Loading

Memory usage test (comparing PR against master branch)

lucasssvaz left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2025 • edited Loading

Test Results

SuGlider commented Mar 28, 2025

SuGlider commented Mar 28, 2025 • edited Loading

SuGlider commented Mar 28, 2025

SuGlider commented Mar 28, 2025

SuGlider commented Mar 28, 2025

me-no-dev Mar 28, 2025

Choose a reason for hiding this comment

me-no-dev Mar 28, 2025

Choose a reason for hiding this comment

me-no-dev Mar 28, 2025

Choose a reason for hiding this comment

SuGlider commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

SuGlider commented Mar 28, 2025 •

edited

Loading