Команде однажды завели баг-репорт: "Сервис упал c segmentation fault, в core dump стэк-трейс указывает как последнюю функцию перед падением что-то из вашей библиотеки. Разберитесь!". Упал сервис ровно один раз за полгода.
Этим чем-то был вызов free где-то глубоко-глубоко внутри библиотери Protobuf. И несколько последующих стэк-фреймов указывали на вызов деструктора уже в нашей библиотеке. Потратив некоторое время на анализ кода деструктора, дежурный инженер не нашел ничего подозрительного и предположил, что это похоже на какую-то ранее встреченную проблему в Protobuf. И как воспроизвести никто не представлял. Тупик...
Я заинтересовался этой загадочной историей и залез в core dump поглубже.
На пару десятков стэк-фреймов выше, уже принадлежащих чужому сервису, засветилась функция lru_insert. Это интересно. Это оказалась фукнция вставки в LRU-кэш. И уже можно было заподозрить, что, возможно, вызов деструктора как-то связан с вытеснением объекта из кэша.
Я решил найти конкретный код того сервиса и что же там происходит при вставке. Обнаруженный код привел меня сначала в замешательство, а потом в восхищение:
auto metadata = new Metadata(...);
metadata->cached = true;
lru_insert(cache, key, metadata);
// check if insert successfull!
if (auto item_handle = lru_get(cache, key)) {
...
} else {
// not found -> it's not cached
metadata->cached = false;
}Если есть голый new, то где-то должен быть и delete... И я нашел его. Целых два
Один при создании кэша
auto cache = lru_create(n, [](void* data){ delete static_cast<Metadata*>(data); });А второй где-то в другом месте
if (!metadata->cached) {
delete metadata;
}Дело пахнет повторным удалением. Но что же пошло не так здесь?
lru_insert(cache, key, metadata);
// check if insert successfull!
if (auto item_handle = lru_get(cache, key)) {
...
} else {
// not found -> it's not cached
metadata->cached = false;
}Код восхтитителен тем, что явно выполняет целых два обращения к кэшу: на вставку и на проверку. Но ведь можно было бы ограничиться только одной вставкой, если lru_insert может предоставить необходимую информацию об успехе... Может ли дело быть в этом? Нет ли в этом сервисе случайно гонок, которые могут вклиниться между вставкой и проверкой? Но меня уверили, что процесс однопоточный.
Наверное, стоит углубиться в функцию lru_insert. Ее написали 10 лет назад и больше не трогали. Ее протестировали. Она надежна. Как я могу в ней сомневаться?
void lru_insert(Cache* c, const char* key, void* data)
{
try {
c->cache.insert(std::string(key),
boost::intrusive_pointer(new LRUItem(data,
c->deleter)));
} catch (...) {}
}От увиденного мне стало дурно. Ведь здесь опять голый new, который может привести к утечке в самом редком и очень часто игнорируемом случае: если конструктор std::string бросит исключение (bad_alloc). И, как мы видим, эта функция полностью игнорирует пойманные исключения.
Но, как утверждает Rust, утечка это совершенно безопасно! И к use-after-free или double-free привести не может. А мы получили segfault с очень большой вероятностью именно из-за double-free.
Есть, конечно, еще вариант, что cache.insert может бросить исключение, не выполнить вставку и заставить intrusive_pointer удалить объект. Но этот сценарий не согласуется с увиденным core dump: повторное удаление (и падение) какого-то старого объекта произошло внутри insert. Если бы новый объект был удален, падение произошло бы в другом месте... Или бы не произошло вовсе. Неопределенное поведение!
У нас есть еще один подозреваемый в этом фрагменте кода. Давайте-ка глянем функцию lru_get. Ее тоже написали 10 лет назад, протестировали, и нет ни малейшего повода в ней сомневаться!
// LRUItemHandle protects data from deletion, if it's evicted from the cache
LRUItemHandle* lru_get(Cache* c, const char* key) {
try {
auto item_ptr = c->cache.get(std::string(key));
if (!item_ptr) {
return nullptr;
}
return new LRUItemHandle(item_ptr);
} catch (...) {
return nullptr;
}
}Я был в ужасе. Надеюсь, вы тоже. Вернемся обратно к злосчастному фрагменту
lru_insert(cache, key, metadata);
// допустим, что insert отработал успешно.
if (auto item_handle = lru_get(cache, key)) {
...
} else {
// У lru_get есть как минимум две возможности соврать нам
// о наличии элемента в кэше. И похоже что нас обманули
metadata->cached = false;
}Как выяснилось, в экстремально редком случае, раз в полгода, в зависимости от нагрузки, у сервиса заканчивалась память. Но вместо падения по out-of-memory он продолжал пытаться работать. Таковы требования. И часто ему это удавалось делать успешно. Пока однажды bad_alloc не был выброшен в этой надежной и протестированной библиотеке c LRU кэшем.
Теперь, когда все улики собраны и преступление реконструировано, надо сделать шаг назад и обдумать произошедшее.
-
Разработчики сервиса проиграли в игру с ручным разделением владения данными между разными компонентами и динамическим определением, кто эти данные будет освобождать. Это сложная игра.
-
У библиотеки с LRU-кэшем, с которым и хотели разделить владение, оказалось чудовищное API. Почему это C-API — обоснование тому есть и оно не касается нашей истории. Но это чудовищное C-API
// Эта фунция не сообщает о возможно ошибке вставки
// Эта фукнция пытается завладеть data, но в случае ошибки
// Data может быть удалена или нет — зависит от типа ошибки:
// - Не удалена при ошибке аллокации до входа в c->cache.insert
// - Удалена при любой ошибке внутри c->cache.insert
void lru_insert(Cache* c, const char* key, void* data)
// Эта функция может упасть с ошибкой, либо не найти элемент
// Но различить эти два исхода пользователю не предлагается возможности
LRUItemHandle* lru_get(Cache* c, const char* key)Передача и разделение владения через границы C-API с помощью сырых указателей и с учетом ошибок и исключений — непростая задача на внимательность, которая может стать кошмаром, если писать в стиле C, игнорируя возможности C++.
API библиотеки с «надежным и протестированным» LRU кэшем я расширил новыми функциями, более устойчивыми к ошибкам. И постарался исправить старые насколько это возможно: например, проблему с владением в lru_insert нельзя было исправлять полностью... потому что нашелся пользователь, который полагался на неправильное поведение.
А функцию стоило написать так с самого начала:
// Эта функция принимает владение data и передает его кэшу
// В случае любой ошибки data НЕ будет освобождена
// В случае успеха контролировать время жизни data будет кэш
ErrorCode lru_try_insert(Cache* c, const char* key, void* data) try {
// Подготавливаем слот для сырого указателя.
auto slot = boost::intrusive_pointer(new LRUItem(nullptr, c->deleter));
// Вставляем пустой слот в кэш.
// Слот должен быть пуст, чтоб не удалить данные при ошибке вставки
c->cache.insert(std::string(key), slot);
// Передаем владение в слот. На этом этапе никакой больше ошибки произойти не может.
// деструктор LRUItem гарантирует вызов deleter(data)
slot->data = data
return ErrorCode::LRU_OK;
} catch (...) {
return ErrorCode::LRU_ERROR;
}Но лучше бы, конечно, пересмотреть зависимости зависимостей и использовать C++ библиотеку для LRU кэша с более безопасными RAII-типами
Исключения (или паники, как в Rust) всегда осложняют ручное управление ресурсами. Это специфично не только для низкоуровневых языков. Например, программы на Go, Java, С#, Python и других также страдают от не закрытых файлов или соединений с базами данных, если программист забыл использовать try-with-resources, finally или defer блоки.
Ручное управление ресурсами крайне рекомендуется сводить к минимуму:
- С помощью RAII оберток
- С помощью умных указателей
- С помощью умных аллокаторов и arena/region based управления множеством объектов
- С помощью любого другого способа, который больше подходит вашей задаче и вашим ресурсам
Также крайне рекомендую C++ разработчикам пробовать Rust как минимум как тьюториал по владению и его передаче и разделению. Строгий borrow checker раскроет вам много интересных паттернов, в которых при полностью ручном контроле можно легко получить double-free.