Chromium: утечки памяти / Habr

Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте Chromium. Это третья часть, которая будет посвящена утечкам памяти.

Я считаю, что код проекта Chromium и используемые в нём библиотеки очень высокого качества. Да, в вводной статье я писал про 250 ошибок, но на самом деле — это очень маленькое число. В силу законов вероятности в огромном проекте обязательно найдётся немало ошибок.

Тем не менее, если говорить об утечках памяти, их не так уж и мало. Думаю, разработчиков Chromium подводит то, что они отдают предпочтение динамическим анализаторам кода. Конечно, у этих инструментов есть ряд преимуществ. Например, они не дают ложных срабатываний, ведь раз динамический анализатор обнаружил ошибку, значит она действительно есть.

С другой стороны, у динамического анализа есть и слабости. Если какой-то код не выполнился, то и ошибка обнаружена не будет. А любой программист понимает, что крайне сложно покрыть тестами 100% кода, вернее, на практике это просто невозможно. В результате, ряд ошибок остаётся в коде и ждёт благоприятного стечения обстоятельств, чтобы проявить себя.

Здесь на помощь может прийти стат��ческий анализатор кода. Да, это намёк разработчикам из компании Google, что мы будем рады, если они станут нашими клиентами. Более того, мы готовы на дополнительные работы по адаптации и настройке PVS-Studio под особенности проекта Chromium. Наша команда также готова взять на себя правку найденных ошибок. У нас уже был подобный опыт (пример).

Но вернёмся к утечкам памяти. Как вы увидите, они прячутся в коде, который крайне редко получает управление. В основном, это различные обработчики ошибок. Статические анализаторы, в отличие от динамических, не всегда способны отследить «судьбу указателя» на выделенную память и не обнаружат многие утечки памяти. С другой стороны, статические анализаторы проверяют весь код, независимо от вероятности его выполнения, и замечают ошибки. Таким образом, статические и динамические анализаторы дополняют друг друга.

Давайте посмотрим, какие утечки памяти я заметил в процессе разбора отчета, выданного PVS-Studio. Как я писал в вводной статье, отчёт я смотрел достаточно бегло, поэтому могут быть и другие, незамеченные мной ошибки. Отмечу также, что утечки памяти крайне неприятны для такого проекта, как Chromium, поэтому будет интересно о них поговорить. Согласно CWE эти ошибки можно классифицировать как CWE-401.

Часть 1: забыли освободить память перед выходом из функции

Рассмотрим ошибку в коде Chromium. Для начала я покажу вспомогательную функцию BnNew, которая выделяет и возвращает обнулённый буфер памяти:

uint32_t* BnNew() {
  uint32_t* result = new uint32_t[kBigIntSize];
  memset(result, 0, kBigIntSize * sizeof(uint32_t));
  return result;
}

А теперь посмотрим на код, который может приводить к утечке памяти:

std::string AndroidRSAPublicKey(crypto::RSAPrivateKey* key) {
  ....
  uint32_t* n = BnNew();
  ....
  RSAPublicKey pkey;
  pkey.len = kRSANumWords;
  pkey.exponent = 65537; // Fixed public exponent
  pkey.n0inv = 0 - ModInverse(n0, 0x100000000LL);
  if (pkey.n0inv == 0)
    return kDummyRSAPublicKey;
  ....
}

Если выполнится условие (pkey.n0inv == 0), то происходит выход из функции без освобождения буфера, указатель на который хранится в переменной n.

Анализатор указывает на этот дефект, выдавая предупреждение: V773 CWE-401 The function was exited without releasing the 'n' pointer. A memory leak is possible. android_rsa.cc 248

Кстати, на этом утечки памяти, относящиеся именно к самому Chromium, заканчиваются. Зато их немало в используемых библиотеках. А пользователям все равно, будет течь память в библиотеках Chromium или в самом Chromium. Поэтому ошибки в библиотеках не менее важны.

Следующие ошибки относятся к движку WebKit. Начать нам вновь придётся со вспомогательной функции:

static CSSValueList* CreateSpaceSeparated() {
  return new CSSValueList(kSpaceSeparator);
}

Теперь код, содержащий ошибку:

const CSSValue* CSSTransformValue::ToCSSValue(....) const {
  CSSValueList* transform_css_value =
    CSSValueList::CreateSpaceSeparated();
  for (size_t i = 0; i < transform_components_.size(); i++) {
    const CSSValue* component =
        transform_components_[i]->ToCSSValue(secure_context_mode);
    if (!component)
      return nullptr;                              // <=
    transform_css_value->Append(*component);
  }
  return transform_css_value;
}

Если указатель component окажется нулевым, то функция завершит свою работу, и при этом произойдёт утечка памяти.

Анализатор PVS-Studio выдаёт предупреждение: V773 CWE-401 The function was exited without releasing the 'transform_css_value' pointer. A memory leak is possible. csstransformvalue.cpp 73

Давайте посмотрим ещё какую-нибудь ошибку, относящуюся к WebKit.

Request* Request::CreateRequestWithRequestOrString(....)
{
  ....
  BodyStreamBuffer* temporary_body = ....;
  ....
  temporary_body =
   new BodyStreamBuffer(script_state, std::move(init.GetBody()));
  ....
  if (exception_state.HadException())
    return nullptr;
  .... 
}

Если функция HadException() вернёт истину, то функция досрочно завершит свою работу. При этом никто не вызовет оператор delete для указателя, хранящегося в переменной temporary_body.

Предупреждение PVS-Studio: V773 CWE-401 The function was exited without releasing the 'temporary_body' pointer. A memory leak is possible. request.cpp 381

Остальные ошибки, которые я заметил в WebKit, ничем не отличаются от описанных, поэтому я не вижу смысла рассматривать их в статье и ограничусь просто списком сообщений анализатора.

V773 CWE-401 The function was exited without releasing the 'image_set' pointer. A memory leak is possible. csspropertyparserhelpers.cpp 1507
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. csspropertyparserhelpers.cpp 1619
V773 CWE-401 The function was exited without releasing the 'shape' pointer. A memory leak is possible. cssparsingutils.cpp 248
V773 CWE-401 The function was exited without releasing the 'shape' pointer. A memory leak is possible. cssparsingutils.cpp 272
V773 CWE-401 The function was exited without releasing the 'shape' pointer. A memory leak is possible. cssparsingutils.cpp 289
V773 CWE-401 The function was exited without releasing the 'shape' pointer. A memory leak is possible. cssparsingutils.cpp 315
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. cssparsingutils.cpp 1359
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. cssparsingutils.cpp 1406
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. cssparsingutils.cpp 1359
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. cssparsingutils.cpp 1406
V773 CWE-401 The function was exited without releasing the 'values' pointer. A memory leak is possible. cssparsingutils.cpp 1985
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. cssparsingutils.cpp 2474
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. cssparsingutils.cpp 2494
V773 CWE-401 The function was exited without releasing the 'values' pointer. A memory leak is possible. atruledescriptorparser.cpp 30
V773 CWE-401 The function was exited without releasing the 'values' pointer. A memory leak is possible. atruledescriptorparser.cpp 57
V773 CWE-401 The function was exited without releasing the 'values' pointer. A memory leak is possible. atruledescriptorparser.cpp 128
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. csssyntaxdescriptor.cpp 193
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. computedstylecssvaluemapping.cpp 1232
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. computedstylecssvaluemapping.cpp 1678
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. computedstylecssvaluemapping.cpp 1727
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. computedstylecssvaluemapping.cpp 2036
V773 CWE-401 The function was exited without releasing the 'size_and_line_height' pointer. A memory leak is possible. computedstylecssvaluemapping.cpp 2070
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. computedstylecssvaluemapping.cpp 2070
V773 CWE-401 The function was exited without releasing the 'file_list' pointer. A memory leak is possible. v8scriptvaluedeserializer.cpp 249
V773 CWE-401 The function was exited without releasing the 'file_list' pointer. A memory leak is possible. v8scriptvaluedeserializer.cpp 264
V773 CWE-401 The function was exited without releasing the 'computed_style_info' pointer. A memory leak is possible. inspectordomsnapshotagent.cpp 367
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. cursor.cpp 42
V773 CWE-401 The function was exited without releasing the 'values' pointer. A memory leak is possible. content.cpp 103
V773 CWE-401 The function was exited without releasing the 'variation_settings' pointer. A memory leak is possible. fontvariationsettings.cpp 56
V773 CWE-401 Visibility scope of the 'font_variation_value' pointer was exited without releasing the memory. A memory leak is possible. fontvariationsettings.cpp 58
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. rotate.cpp 32
V773 CWE-401 The function was exited without releasing the 'values' pointer. A memory leak is possible. quotes.cpp 25
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. textindent.cpp 52
V773 CWE-401 The function was exited without releasing the 'list' pointer. A memory leak is possible. shapeoutside.cpp 35
V773 CWE-401 The function was exited without releasing the 'port_array' pointer. A memory leak is possible. v8messageeventcustom.cpp 127

Много? Много. При этом здесь выписаны только те сообщения, на которые мне хватило сил. Я быстро заскучал и просматривал подобные предупреждения очень поверхностно. Скорее всего, при более внимательном анализе отчета, ошибок в WebKit будет найдено гораздо больше.

Что это значит? Это значит, что у проекта WebKit проблемы с утечками памяти, с чем я этот проект и «поздравляю».

Теперь перейдём к проекту ICU и рассмотрим ошибку, найденную в нём.

UVector*
RuleBasedTimeZone::copyRules(UVector* source) {
    if (source == NULL) {
        return NULL;
    }
    UErrorCode ec = U_ZERO_ERROR;
    int32_t size = source->size();
    UVector *rules = new UVector(size, ec);
    if (U_FAILURE(ec)) {
        return NULL;
    }
  ....
}

Если при инициализации объекта типа UVector произойдёт некая ошибка, то это отразится на статусе, который помещается в переменную ec. Например, конструктор вернёт статус U_MEMORY_ALLOCATION_ERROR если не удастся выделить буфер памяти для хранения нужного количества элементов. Однако, независимо от того, удастся выделить память для хранения элементов или нет, сам объект типа UVector будет создан и указатель на этот объект будет помещён в переменную rules.

Если конструктор возвратит статус U_MEMORY_ALLOCATION_ERROR, то произойдёт выход из функции. При этом сам объект типа UVector удалён не будет, и возникнет утечка памяти.

Предупреждение PVS-Studio: V773 CWE-401 The function was exited without releasing the 'rules' pointer. A memory leak is possible. rbtz.cpp 668

Другие ошибки в библиотеке ICU также приведу просто списком.

V773 CWE-401 The function was exited without releasing the 'tmpSet' pointer. A memory leak is possible. uspoof_impl.cpp 184
V773 CWE-401 The function was exited without releasing the 'result' pointer. A memory leak is possible. stsearch.cpp 301
V773 CWE-401 The function was exited without releasing the 'values' pointer. A memory leak is possible. tznames_impl.cpp 154
V773 CWE-401 The function was exited without releasing the 'filter' pointer. A memory leak is possible. tridpars.cpp 298
V773 CWE-401 The function was exited without releasing the 'targets' pointer. A memory leak is possible. transreg.cpp 984
V773 CWE-401 The function was exited without releasing the 'instance' pointer. A memory leak is possible. tzgnames.cpp 1216
V773 CWE-401 The function was exited without releasing the 'uset' pointer. A memory leak is possible. rbbiscan.cpp 1276

Что ещё я заметил?

Библиотека Libwebm.

V773 CWE-401 The function was exited without releasing the 'new_frame' pointer. A memory leak is possible. mkvmuxer.cc 3513
V773 CWE-401 The function was exited without releasing the 'new_frame' pointer. A memory leak is possible. mkvmuxer.cc 3539

Библиотека SwiftShader.

V773 CWE-401 The function was exited without releasing the 'node' pointer. A memory leak is possible. intermediate.cpp 405
V773 CWE-401 The function was exited without releasing the 'node' pointer. A memory leak is possible. intermediate.cpp 443
V773 CWE-401 The function was exited without releasing the 'node' pointer. A memory leak is possible. intermediate.cpp 514
V773 CWE-401 The function was exited without releasing the 'rightUnionArray' pointer. A memory leak is possible. intermediate.cpp 1457
V773 CWE-401 The function was exited without releasing the 'unionArray' pointer. A memory leak is possible. intermediate.cpp 1457
V773 CWE-401 The function was exited without releasing the 'aggregateArguments' pointer. A memory leak is possible. parsehelper.cpp 2109

Наверное, это далеко не все ошибки, но их мне достаточно для демонстрации возможностей PVS-Studio и написания этой статьи.

Часть 1: рекомендация

Что объединяет все рассмотренные выше случаи? То, что ошибки стали возможны из-за ручного управления памятью!

Друзья, мы уже используем C++17. Хватит вызывать оператор new, помещать результат в обыкновенный указатель, а потом забывать его освободить. Стыдно же.

Больше никаких обыкновенных указателей и последующего ручного управления выделенным ресурсом! Давайте всегда использовать умные указатели.

Современный стандарт C++ предлагает такие умные указатели, как unique_ptr, shared_ptr и weak_ptr. В большинстве случаев будет достаточно одного unique_ptr.

Вернёмся, например, к этому некорректному коду:

const CSSValue* CSSTransformValue::ToCSSValue(....) const {
  CSSValueList* transform_css_value =
    CSSValueList::CreateSpaceSeparated();
  for (size_t i = 0; i < transform_components_.size(); i++) {
    const CSSValue* component =
        transform_components_[i]->ToCSSValue(secure_context_mode);
    if (!component)
      return nullptr;
    transform_css_value->Append(*component);
  }
  return transform_css_value;
}

Давайте перепишем его с использованием unique_ptr. Для этого нам, во-первых, нужно изменить тип самого указателя. Во-вторых, в самом конце требуется вызвать функцию release, чтобы вернуть указатель на управляемый объект и более его не контролировать.

Корректный код:

const CSSValue* CSSTransformValue::ToCSSValue(....) const {
  unique_ptr<CSSValueList> transform_css_value(
    CSSValueList::CreateSpaceSeparated());
  for (size_t i = 0; i < transform_components_.size(); i++) {
    const CSSValue* component =
        transform_components_[i]->ToCSSValue(secure_context_mode);
    if (!component)
      return nullptr;
    transform_css_value->Append(*component);
  }
  return transform_css_value.release();
}

Я не планирую учить в этой статье как использовать умные указатели. Этой теме посвящено множество хороших статей и разделов в книгах. Я лишь хотел показать, что код из-за изменений не стал более сложным и громоздким. Зато теперь сделать ошибку будет намного сложнее.

Не думайте, что именно вы справитесь с new/delete или с malloc/free и не сделаете ошибок. Разработчики Chromium делают такие ошибки. Другие разработчики делают. Вы делаете и будете делать такие ошибки. И не надо лишних мечтаний, что ваша команда особенная :). Пользуясь случаем, прошу менеджеров прочитать сейчас вот эту заметку.

Используйте умные указатели.

Часть 2: realloc

По моим наблюдениям, программисты иногда неправильно используют функцию realloc. Вот как выглядит классический паттерн ошибки, связанный с использованием этой функции:

p = realloc(p, n);
if (!p)
  return ERROR;

Обратим внимание на следующее свойство функции: If there is not enough memory, the old memory block is not freed and null pointer is returned.

Поскольку NULL будет записан в переменную p, которая хранила указатель на буфер, то теряется возможность освободить этот буфер. Возникает утечка памяти.

Правильным вариантом будет переписать код, например, так:

void *old_p = p;
p = realloc(p, n);
if (!p)
{
  free(old_p);
  return ERROR;
}

Не обошлось без подобных ошибок и в библиотеках, используемых в проекте Chromium.

Например, рассмотрим фрагмент кода в кодеке FLAC.

FLAC__bool FLAC__format_entropy_codi.....ce_contents_ensure_size(
  FLAC__EntropyCodingMethod_PartitionedRiceContents *object,
  unsigned max_partition_order)
{
  ....
  if(object->capacity_by_order < max_partition_order) {
    if(0 == (object->parameters =
               realloc(object->parameters, ....)))
      return false;
    if(0 == (object->raw_bits = realloc(object->raw_bits, ....)))
      return false;
    ....
}

Функция увеличивает размер двух буферов:

object->parameters
object->raw_bits

Если происходит ошибка выделения памяти, то функция досрочно завершает работу и возвращает значение false. При этом теряется предыдущее значение указателя, и произойдёт утечка памяти.

Анализатор PVS-Studio выдаёт здесь два соответствующих предупреждения:

V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'object->parameters' is lost. Consider assigning realloc() to a temporary pointer. format.c 576
V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'object->raw_bits' is lost. Consider assigning realloc() to a temporary pointer. format.c 578

Аналогичные недоработки в проекте WebRTC.

V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'self->binary_far_history' is lost. Consider assigning realloc() to a temporary pointer. delay_estimator.cc 303
V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'self->far_bit_counts' is lost. Consider assigning realloc() to a temporary pointer. delay_estimator.cc 306
V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'self->mean_bit_counts' is lost. Consider assigning realloc() to a temporary pointer. delay_estimator.cc 453
V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'self->bit_counts' is lost. Consider assigning realloc() to a temporary pointer. delay_estimator.cc 456
V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'self->histogram' is lost. Consider assigning realloc() to a temporary pointer. delay_estimator.cc 458

Ошибок этого типа в Chromium, к счастью, очень мало. По крайней мере, намного меньше, чем я обычно встречаю в других проектах.

Часть 2: рекомендация

Не всегда можно отказаться от функции realloc, так как она позволяет писать эффективный код, когда требуется часто менять размер буфера.

Поэтому я не буду торопиться рекомендовать совсем отказаться от неё. Иногда это будет неоправданно. Прошу только быть аккуратным с этой функцией и не забывать паттерн ошибки, который я описал выше.

Впрочем, очень часто в C++ вполне можно обойтись без этой функции и использовать контейнеры, такие как std::vector или std::string. Эффективность контейнеров существенно выросла в последние годы. Я, например, был приятно удивлён, когда увидел, что больше нет отличия по производительности в ядре PVS-Studio между самодельным классом строки и std::string. А ведь много лет назад самодельный класс строки давал около 10% прироста производительности анализатору. Больше такой эффект не наблюдается и стало возможным удалить свой собственный класс. Сейчас класс std::string совсем не тот, каким он был 10 лет назад. Эффективность существенно повысилась благодаря современным возможностям компиляторов по оптимизации и благодаря таким новшествам языка, как, например, конструктор перемещения.

В общем, не спешите засучить рукава и вручную управлять памятью с помощью функций malloc, realloc, free. Почти наверняка std::vector окажется для ваших задач не менее эффективен. При этом, использовать std::vector намного проще. Сделать ошибку также станет сложнее. К низкоуровневым функциям есть смысл вернуться только тогда, когда профилировщик покажет, что это действительно одно из узких мест в работе программы.

Спасибо всем за внимание. Приглашаю скачать и попробовать анализатор PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Chromium: Memory Leaks.

Chromium: утечки памяти

Часть 1: забыли освободить память перед выходом из функции

Часть 1: рекомендация

Часть 2: realloc

Часть 2: рекомендация

Articles

Information