Skip to content

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

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.

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

Closed
wants to merge 11 commits into from
53 changes: 28 additions & 25 deletions libraries/Zigbee/src/ZigbeeEP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ ZigbeeEP::ZigbeeEP(uint8_t endpoint) {
_ep_config.endpoint = 0;
_cluster_list = nullptr;
_on_identify = nullptr;
_read_model = nullptr; // Explicitly initialize here
_read_manufacturer = nullptr; // Explicitly initialize here
_time_status = 0;
if (!lock) {
lock = xSemaphoreCreateBinary();
Expand All @@ -33,38 +35,41 @@ void ZigbeeEP::setVersion(uint8_t version) {
}

bool ZigbeeEP::setManufacturerAndModel(const char *name, const char *model) {
constexpr size_t ZB_MAX_NAME_LENGTH = 32;
// Convert manufacturer to ZCL string
size_t name_length = strlen(name);
size_t model_length = strlen(model);
if (name_length > 32 || model_length > 32) {
if (name_length > ZB_MAX_NAME_LENGTH || model_length > ZB_MAX_NAME_LENGTH) {
log_e("Manufacturer or model name is too long");
return false;
}
// Allocate a new array of size length + 2 (1 for the length, 1 for null terminator)
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);
Copy link
Member

Choose a reason for hiding this comment

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

what will happen if there was not enough memory?

// Store the length as the first element
zb_name[0] = static_cast<char>(name_length); // Cast size_t to char
zb_model[0] = static_cast<char>(model_length);
// Use memcpy to copy the characters to the result array
memcpy(zb_name + 1, name, name_length);
memcpy(zb_model + 1, model, model_length);
memcpy(zb_name.data() + 1, name, name_length);
memcpy(zb_model.data() + 1, model, model_length);
// Null-terminate the array
zb_name[name_length + 1] = '\0';
zb_model[model_length + 1] = '\0';

// Get the basic cluster and update the manufacturer and model attributes
esp_zb_attribute_list_t *basic_cluster = esp_zb_cluster_list_get_cluster(_cluster_list, ESP_ZB_ZCL_CLUSTER_ID_BASIC, ESP_ZB_ZCL_CLUSTER_SERVER_ROLE);
esp_err_t ret_name = esp_zb_basic_cluster_add_attr(basic_cluster, ESP_ZB_ZCL_ATTR_BASIC_MANUFACTURER_NAME_ID, (void *)zb_name);
if (!basic_cluster) {
log_e("Failed to get basic cluster");
return false;
}
esp_err_t ret_name = esp_zb_basic_cluster_add_attr(basic_cluster, ESP_ZB_ZCL_ATTR_BASIC_MANUFACTURER_NAME_ID, (void *)zb_name.data());
if (ret_name != ESP_OK) {
log_e("Failed to set manufacturer: 0x%x: %s", ret_name, esp_err_to_name(ret_name));
}
esp_err_t ret_model = esp_zb_basic_cluster_add_attr(basic_cluster, ESP_ZB_ZCL_ATTR_BASIC_MODEL_IDENTIFIER_ID, (void *)zb_model);
esp_err_t ret_model = esp_zb_basic_cluster_add_attr(basic_cluster, ESP_ZB_ZCL_ATTR_BASIC_MODEL_IDENTIFIER_ID, (void *)zb_model.data());
if (ret_model != ESP_OK) {
log_e("Failed to set model: 0x%x: %s", ret_model, esp_err_to_name(ret_model));
}
delete[] zb_name;
delete[] zb_model;
return ret_name == ESP_OK && ret_model == ESP_OK;
}

Expand Down Expand Up @@ -163,9 +168,7 @@ char *ZigbeeEP::readManufacturer(uint8_t endpoint, uint16_t short_addr, esp_zb_i
read_req.attr_number = ZB_ARRAY_LENTH(attributes);
read_req.attr_field = attributes;

if (_read_manufacturer != nullptr) {
free(_read_manufacturer);
}
free(_read_manufacturer); // CPP tests it for nullptr
Copy link
Member

Choose a reason for hiding this comment

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

what does the comment mean?

_read_manufacturer = nullptr;

esp_zb_lock_acquire(portMAX_DELAY);
Expand Down Expand Up @@ -201,9 +204,7 @@ char *ZigbeeEP::readModel(uint8_t endpoint, uint16_t short_addr, esp_zb_ieee_add
read_req.attr_number = ZB_ARRAY_LENTH(attributes);
read_req.attr_field = attributes;

if (_read_model != nullptr) {
free(_read_model);
}
free(_read_model); // CPP tests it for nullptr
_read_model = nullptr;

esp_zb_lock_acquire(portMAX_DELAY);
Expand Down Expand Up @@ -245,20 +246,22 @@ void ZigbeeEP::zbReadBasicCluster(const esp_zb_zcl_attribute_t *attribute) {
/* Basic cluster attributes */
if (attribute->id == ESP_ZB_ZCL_ATTR_BASIC_MANUFACTURER_NAME_ID && attribute->data.type == ESP_ZB_ZCL_ATTR_TYPE_CHAR_STRING && attribute->data.value) {
zbstring_t *zbstr = (zbstring_t *)attribute->data.value;
char *string = (char *)malloc(zbstr->len + 1);
memcpy(string, zbstr->data, zbstr->len);
string[zbstr->len] = '\0';
log_i("Peer Manufacturer is \"%s\"", string);
_read_manufacturer = string;
std::vector<char> zb_manufacturer(zbstr->len + 1);
memcpy(zb_manufacturer.data(), zbstr->data, zbstr->len);
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
Copy link
Member

Choose a reason for hiding this comment

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

strdup will return NULL if there was not enough memory

xSemaphoreGive(lock);
}
if (attribute->id == ESP_ZB_ZCL_ATTR_BASIC_MODEL_IDENTIFIER_ID && attribute->data.type == ESP_ZB_ZCL_ATTR_TYPE_CHAR_STRING && attribute->data.value) {
zbstring_t *zbstr = (zbstring_t *)attribute->data.value;
char *string = (char *)malloc(zbstr->len + 1);
memcpy(string, zbstr->data, zbstr->len);
string[zbstr->len] = '\0';
log_i("Peer Model is \"%s\"", string);
_read_model = string;
std::vector<char> zb_model(zbstr->len + 1);
memcpy(zb_model.data(), zbstr->data, zbstr->len);
zb_model[zbstr->len] = '\0';
log_i("Peer Model is \"%s\"", zb_model.data());
free(_read_model); // Free any previously allocated memory
_read_model = strdup(zb_model.data()); // Duplicate the information for persistent storage
xSemaphoreGive(lock);
}
}
Expand Down
Loading