espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.73k stars 7.3k forks source link

[TW#15764] BT: btc_gatts_arg_deep_copy 8 no mem (IDFGH-6214) #1077

Closed nkolban closed 7 years ago

nkolban commented 7 years ago

I am running a BLE Server application with the latest master of ESP-IDF as of the date of this post. Each time I create a new BLE Characteristic with a call to:

esp_ble_gatts_add_char(...)

an error message is logged to the console which reads:

E (<time>) BT: btc_gatts_arg_deep_copy 8 no mem

I am at a loss to explain. My Bluetooth event stack is 16000.

negativekelvin commented 7 years ago

Deep copy is using heap not stack. Could be related to unexpected loss of 20k in other issue.

nkolban commented 7 years ago

I tried compiling with the previous (to current/new) toolchain and found that the unexplained error messages persist. The good news is that I haven't yet seen any adverse operational issues as a result of the messages.

negativekelvin commented 7 years ago

Does your characteristic have a value and value length set?

nkolban commented 7 years ago

Doing some more study, I was supplying an esp_attr_value when the control.auto_rsp was equal to ESP_GATT_RSP_BY_APP. My understanding now is that when one is declaring that the response will be provided by application logic, one need not provide a value. So my logic was likely confused ... I was declaring that my app will be responsible for sending a response while at the same time providing a response value that was available to be sent back by ESP-IDF without my involvement. However, I'm not seeing why this causes an error log message. I would have assured that if I had specified ESP_GATT_RSP_BY_APP then any value I may or may not have provided for the value would be ignored. This however does not appear to be the case. When I set the value to be "nullptr", the error messages stopped being produced (my control type was ESP_GATT_RSP_BY_APP).

Yulong-espressif commented 7 years ago

@nkolban I tried it in my own environment, did not find this error, how do I need to be able to reproduce your issue? My MR is b013f5d

Yulong-espressif commented 7 years ago

@nkolban I tried it in my own environment, did not find this error, how do I need to be able to reproduce your issue? My MR is b013f5d

FayeY commented 7 years ago

Any update? Is this issue solved now?

PhilColbert commented 6 years ago

Hi, how was this solved please ?

I am getting btc_gatts_arg_deep_copy 9 no mem

Thanks

dhrishi commented 6 years ago

@PhilColbert When do you see this error? Are you adding a characteristic descriptor with zero length?

PhilColbert commented 6 years ago

Thanks for the reply,

Its when its iterating through

while(pCharacteristic != nullptr) { m_lastCreatedCharacteristic = pCharacteristic; pCharacteristic->executeCreate(this);

    pCharacteristic = m_characteristicMap.getNext();
}

In BLEService:start

D (4977) BLEUtils: [status: ESP_GATT_OK, attr_handle: 42 0x2a, service_handle: 40 0x28, char_uuid: 0000ffe1-0000-1000-8000-00805f9b3 4fb] D (4989) BLEServer: >> handleGATTServerEvent: ESP_GATTS_ADD_CHAR_EVT D (4996) BLECharacteristic: >> setHandle: handle=0x2a, characteristic uuid=0000ffe1-0000-1000-8000-00805f9b34fb D (5006) BLECharacteristic: << setHandle D (5010) BLECharacteristic: >> handleGATTServerEvent: ESP_GATTS_ADD_CHAR_EVT V (5017) FreeRTOS: Semaphore giving: name: CreateEvt (0x3fffd0e0), owner: executeCreate D (5025) BLECharacteristic: << handleGATTServerEvent V (5025) FreeRTOS: << wait: Semaphore released: name: CreateEvt (0x3fffd0e0), owner: executeCreate D (5030) BLECharacteristic: >> handleGATTServerEvent: ESP_GATTS_ADD_CHAR_EVT D (5039) BLEDescriptor: >> executeCreate(): UUID: 00002902-0000-1000-8000-00805f9b34fb, handle: 0xffff D (5056) FreeRTOS: Semaphore taking: name: CreateEvt (0x3fffd410), owner: <N/A> for executeCreate D (5046) BLECharacteristic: << handleGATTServerEvent D (5065) FreeRTOS: Semaphore taken: name: CreateEvt (0x3fffd410), owner: executeCreate D (5070) BLEServer: << handleGATTServerEvent E (5078) BT: btc_gatts_arg_deep_copy 9 no mem

V (5087) FreeRTOS: >> wait: Semaphore waiting: name: CreateEvt (0x3fffd410), owner: executeCreate for executeCreate D (5088) BLEDevice: gattServerEventHandler [esp_gatt_if: 4] ... ESP_GATTS_ADD_CHAR_DESCR_EVT

Thanks

mws-rmain commented 6 years ago

I was getting: btc_gatts_arg_deep_copy 10 no mem

In my case, I was trying to send 'notify' on a characteristic. The value of the characteristic was still un-defined, so I had set 'length' to zero, and in my case the pointer was null. The 'length' is not range-checked, nor is the pointer checked for null. When btc_gatts_arg_deep_copy() requests a block of memory size 0 (ZERO) from osi_malloc, the error above results. It looks like the other clauses within btc_gatts_arg_deep_copy() are similar, so possibly look for osi_malloc of 0 bytes.

In particular: btc_gatts_arg_deep_copy 8 no mem appears associated with BTC_GATTS_ACT_ADD_CHAR. btc_gatts_arg_deep_copy 9 no mem appears associated with BTC_GATTS_ACT_ADD_CHAR_DESCR. (note in these two cases, the pointer to value IS checked for not equal to NULL in btc_gatts_arg_deep_copy()).

I really think this should be addressed in ESP IDF. In my case, the parameters of esp_ble_gatts_send_indicate() should be sanity checked (is value_len>0?) BEFORE invoking btc_transfer context(), resulting in cryptic deeply-embedded errors.

dhrishi commented 6 years ago

Hi @mws-rmain Yes. I think this should be addressed. Although, can you please let me know if this error is causing any functionality issue going ahead?

mws-rmain commented 6 years ago

I resolved my issue by only requesting notify once the value associated with the characteristic became valid (and therefore could be 'non-null'). I had thought to use the characteristic notify with NULL value as a kind of 'heartbeat', however once I determined the cause of the 'no mem' error, I investigated the BT spec, and could find no mention that notify/indicate with null/zero-length value was valid (with the possible exception of string characteristics, where this might make sense). It seemed the appropriate action was to ONLY notify on valid values. I assume you are attempting to determine priority to assign fixing this. The only argument I'd supply to a higher priority is the difficulty in determining the root cause when one encounters the problem, and that is not helped by the cryptic error message.

dhrishi commented 6 years ago

There can be cases, when the peripheral has to just send a zero length (no value) indication to the central so that it then requests for the updated characteristic value. So, I don't totally rule out the possibility for this use case. I completely agree about the error message which makes the user go clueless about the actual root cause. My purpose about asking this was just to understand if it breaks any functionality so that could be tested while fixing this. Anyway, fix for this will be made available soon. I will update here once it is merged so that you can give it a try. :-)

bliutrakeng commented 3 years ago

void gatts_comm_write_event_env(esp_gatt_if_t gatts_if, prepare_type_env_t prepare_write_env, esp_ble_gatts_cb_param_t param){ esp_gatt_status_t status = ESP_GATT_OK; if (param->write.need_rsp){ if (param->write.is_prep){ if (prepare_write_env->prepare_buf == NULL) { prepare_write_env->prepare_buf = (uint8_t )malloc(PREPARE_BUF_MAX_SIZEsizeof(uint8_t)); prepare_write_env->prepare_len = 0; if (prepare_write_env->prepare_buf == NULL) { ESP_LOGE(GATTS_TAG, "Gatt_server prep no mem\n"); status = ESP_GATT_NO_RESOURCES; } } else { if(param->write.offset > PREPARE_BUF_MAX_SIZE) { status = ESP_GATT_INVALID_OFFSET; } else if ((param->write.offset + param->write.len) > PREPARE_BUF_MAX_SIZE) { status = ESP_GATT_INVALID_ATTR_LEN; } }

        esp_gatt_rsp_t *gatt_rsp = (esp_gatt_rsp_t *)malloc(sizeof(esp_gatt_rsp_t));
        gatt_rsp->attr_value.len = param->write.len;
        gatt_rsp->attr_value.handle = param->write.handle;
        gatt_rsp->attr_value.offset = param->write.offset;
        gatt_rsp->attr_value.auth_req = ESP_GATT_AUTH_REQ_NONE;
        memcpy(gatt_rsp->attr_value.value, param->write.value, param->write.len);
        esp_err_t response_err = esp_ble_gatts_send_response(gatts_if, param->write.conn_id, param->write.trans_id, status, gatt_rsp);
        if (response_err != ESP_OK){
           ESP_LOGE(GATTS_TAG, "Send response error\n");
        }
        free(gatt_rsp);
        if (status != ESP_GATT_OK){
            return;
        }
        memcpy(prepare_write_env->prepare_buf + param->write.offset,
               param->write.value,
               param->write.len);
        prepare_write_env->prepare_len += param->write.len;

    }else{
        esp_gatt_rsp_t *gatt_rsp = (esp_gatt_rsp_t *)malloc(sizeof(esp_gatt_rsp_t));
        gatt_rsp->attr_value.len = param->write.len;
        gatt_rsp->attr_value.handle = param->write.handle;
        gatt_rsp->attr_value.offset = param->write.offset;
        gatt_rsp->attr_value.auth_req = ESP_GATT_AUTH_REQ_NONE;
        memcpy(gatt_rsp->attr_value.value, "....", 4);
        esp_ble_gatts_send_response(gatts_if, param->write.conn_id, param->write.trans_id, status, gatt_rsp);
        **free(gatt_rsp);**
    }
}

} The high light free() will fix the problem.

rahult-github commented 2 years ago

Hi @bliutrakeng , If the last parameter passed to esp_ble_gatts_send_response API is a dynamically allocated buffer, then it is correct to free it, once this API returns.

 }else{
        esp_gatt_rsp_t *gatt_rsp = (esp_gatt_rsp_t *)malloc(sizeof(esp_gatt_rsp_t));
        gatt_rsp->attr_value.len = param->write.len;
        gatt_rsp->attr_value.handle = param->write.handle;
        gatt_rsp->attr_value.offset = param->write.offset;
        gatt_rsp->attr_value.auth_req = ESP_GATT_AUTH_REQ_NONE;
        memcpy(gatt_rsp->attr_value.value, "....", 4);
        esp_ble_gatts_send_response(gatts_if, param->write.conn_id, param->write.trans_id, status, gatt_rsp);
        **free(gatt_rsp);**
    }
}

I couldn't find the above code in IDF . Can you please help share the ESP-IDF version being used that has this code ? .. In case a "free" is missing, then this can be a potential memory leak and we will work to address this issue.

Thanks, Rahul

bliutrakeng commented 2 years ago

Hi @bliutrakeng , If the last parameter passed to esp_ble_gatts_send_response API is a dynamically allocated buffer, then it is correct to free it, once this API returns.

}else{
      esp_gatt_rsp_t *gatt_rsp = (esp_gatt_rsp_t *)malloc(sizeof(esp_gatt_rsp_t));
      gatt_rsp->attr_value.len = param->write.len;
      gatt_rsp->attr_value.handle = param->write.handle;
      gatt_rsp->attr_value.offset = param->write.offset;
      gatt_rsp->attr_value.auth_req = ESP_GATT_AUTH_REQ_NONE;
      memcpy(gatt_rsp->attr_value.value, "....", 4);
      esp_ble_gatts_send_response(gatts_if, param->write.conn_id, param->write.trans_id, status, gatt_rsp);
      **free(gatt_rsp);**
  }
}

I couldn't find the above code in IDF . Can you please help share the ESP-IDF version being used that has this code ? .. In case a "free" is missing, then this can be a potential memory leak and we will work to address this issue.

Thanks, Rahul

This is the BLE example code in the ESP-IDF.

rahult-github commented 2 years ago

Hi @bliutrakeng

This is the BLE example code in the ESP-IDF.

I checked the existing examples in examples/bluetooth/bluedroid path and do not see an instance of esp_ble_gatts_send_response without a "free" if dynamically allocated buffer. The latest code doesn't have such issue.

Can you please let me know the commit / IDF version you are on ?

Thanks, Rahul

mws-rmain commented 2 years ago

Just to be clear, adding a 'missing' (?) free in example code shown IS NOT A FIX for the original issue I posted about (Handling of Notify/Indicate on Characteristics with null pointer or zero length).

As far as I know, the thread (up to the point of @bliutrakeng's post) still remains to be addressed - though I have not recently reviewed this in ESP-IDF.

rahult-github commented 2 years ago

Hi @mws-rmain ,

Indeed, the original issue you mentioned and the information @bliutrakeng posted are, in perspective, two different issues.

For the issue you have mentioned, changes were done in IDF previously to consider zero length / null pointer. Please feel free to refer to latest esp-idf for same .

Thanks, Rahul