Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте 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 будет найдено гораздо больше.

Что это значит? Это значит, что у проекта 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


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



Наверное, это далеко не все ошибки, но их мне достаточно для демонстрации возможностей 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

Ошибок этого типа в 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.