ThreadSafety issue in NimBLEAttValue::append(), manifesting in setValue()

We hit an issue on a device in production that was causing heap corruption. We tracked the issue down to a race condition in NimBLEAttValue::append()

NimBLEAttValue::setValue() / append() are not safe when the NimBLE host task and an application task update the same characteristic value concurrently. append() computes the new length from m_attr_len outside the critical section, then copies to res + m_attr_len using m_attr_len re-read inside it. If the other task's setValue() completes in between, the copy uses the wrong offset and overruns the buffer.

Environment

ESP32-S3, ESP-IDF v5.5.0
esp-nimble-cpp 2.3.4. The code is unchanged in 2.5.0
NimBLE host task on core 0, application task on core 1

Code path

NimBLEServer::handleGattEvent -> pAtt->writeEvent(buf, len, ...) (NimBLEServer.cpp#L791)
NimBLECharacteristic::writeEvent -> setValue(val, len) on the host task (NimBLECharacteristic.cpp#L435-L438)
The application calls setValue(frame, len) + notify() on the same characteristic from its own task.

NimBLEAttValue::setValue / append:

bool NimBLEAttValue::setValue(const uint8_t* value, uint16_t len) {
    m_attr_len      = 0;            // outside any critical section
    m_attr_value[0] = '\0';
    append(value, len);
    ...
}

NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) {
    ...
    uint16_t new_len = m_attr_len + len;   // L126: read OUTSIDE the critical section
    bool     grow    = new_len > m_capacity;
    if (grow) {
        res = realloc(m_attr_value, new_len + 1);   // also outside
    }
    ...
    ble_npl_hw_enter_critical();
    memcpy(res + m_attr_len, value, len);  // L148: m_attr_len re-read INSIDE
    m_attr_value             = res;
    m_attr_len               = new_len;
    m_attr_value[m_attr_len] = '\0';
    ble_npl_hw_exit_critical(0);
}

Same pattern in deepCopy()

deepCopy(), used by the copy that notify() takes (auto value{m_value};), sizes its buffer from source.m_capacity outside the critical section. It then copies source.m_attr_len + 1 bytes from source.m_attr_value inside it. If the source grows in between, the copy overruns the new buffer. If the source's realloc has already freed the old buffer, the copy reads freed memory.

@OzerOzdemir

Thank you @OzerOzdemir, should be resolved by #451, please test it and let me know.

@h2zero

Thanks for the quick turnaround, @h2zero . I reviewed #451 against the crash we had and this fixes the specific issue we hit. We have made some changes in how we use attributes to minimize concurrency issues.

For this PR, some observations:

I'd recommend locking readers that make a copy. getValue<T>() , the conversion operators, operator==, operator[], and the source side of operator+=
data(), c_str() and begin()/end() can't be fixed with a lock, because the pointer outlives it. I'd wish the library didn't hand out a pointer to the data. So maybe document this and tell users to treat the pointer getters as safe only on a copy.

Multi Step updates like in

esp-nimble-cpp/src/NimBLEClient.cpp

Lines 1183 to 1197 in 9c094d9

    
           if (pChr->m_value.setValue(event->notify_rx.om->om_data, len)) { 
        
               os_mbuf* next; 
        
               next = SLIST_NEXT(event->notify_rx.om, om_next); 
        
               while (next != NULL) { 
        
                   pChr->m_value.append(next->om_data, next->om_len); 
        
                   if (pChr->m_value.length() != len + next->om_len) { 
        
                       rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; 
        
                       break; 
        
                   } 
        
                   len  += next->om_len; 
        
                   next  = SLIST_NEXT(next, om_next); 
        
               } 
        
           } else { 
        
               rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; 
        
           }

are not atomic. A write from another task can land in between and leave a mixed value. Since the mutex is recursive, holding one Lock across the whole build would fix it. Another option is to build into a local NimBLEAttValue and move it in, then give the callback that local copy instead of a pointer into m_value. I'd avoid holding the lock across the user callback, since user code would then run under a lock that every attribute shares.

Nitpick: Lock::init() (NimBLEDevice.cpp#L1020) runs after the host task has started and synced. You might consider creating the mutex before nimble_port_freertos_init() and failing init().

@OzerOzdemir

@OzerOzdemir agreed, thank you, changes implemented, please let me know if there are any further issues.

Looks good to me

Navigation Menu

Repository navigation

ThreadSafety issue in NimBLEAttValue::append(), manifesting in setValue() #446

Environment

Code path

Same pattern in deepCopy()

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	if (pChr->m_value.setValue(event->notify_rx.om->om_data, len)) {
	os_mbuf* next;
	next = SLIST_NEXT(event->notify_rx.om, om_next);
	while (next != NULL) {
	pChr->m_value.append(next->om_data, next->om_len);
	if (pChr->m_value.length() != len + next->om_len) {
	rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN;
	break;
	}
	len += next->om_len;
	next = SLIST_NEXT(next, om_next);
	}
	} else {
	rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN;
	}

ThreadSafety issue in NimBLEAttValue::append(), manifesting in setValue() #446

Description

Environment

Code path

Same pattern in deepCopy()

Activity

h2zero commented on Oct 18, 2026

OzerOzdemir commented on Oct 18, 2026

h2zero commented on Oct 19, 2026

OzerOzdemir commented on Oct 20, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions