Код некоторых модулей Erlang/OTP старше, чем большинство современных junior-разработчиков. Эти файлы — настоящие цифровые патриархи, десятилетиями обеспечивающие работу банковских транзакций, телефонных сетей и систем обмена сообщениями. Мы решили заглянуть под "капот" этого языка-долгожителя, чтобы проверить, что именно скрывается в строках, на которые сегодня полагаются миллионы пользователей. А вот что мы нашли, узнаем в этой статье.

Введение

Перед тем как перейти к рассматриванию срабатываний, стоит сказать пару слов о нашем проекте.

История языка Erlang началась в далёком 1986 году в одной из лабораторий компании Ericsson. Джо Армстронг тестировал Prolog, чтобы написать программу для телефонии. В ходе таких экспериментов и появился Erlang.

Erlang — это язык программирования, созданный для высоконадёжных систем, в которых падение недопустимо. Представьте телефонную станцию, где обрыв вызова — это ЧП, или банковскую транзакцию, которая должна пройти в любом случае. Именно для таких задач он и используется. Благодаря Erlang система может легко разделяться на миллионы изолированных, независимых процессов, где сбой одного не приводит к падению всей системы.

Erlang неразрывно связан с OTP (Open Telecom Platform) — фреймворком, который представляет набор мощных инструментов, паттернов и поведений для построения распределённых, отказоустойчивых приложений. Весь этот "коробочный набор" позволяет системам, написанным на нём, работать без остановки годами.

Проект был собран по этой инструкции. Проверка производилась на ветке maint-28 с помощью статического анализатора PVS-Studio и плагина для Visual Studio Code. Со сборкой проекта проблем не возникло, за что отдельная благодарность разработчикам.

В статью вошли предупреждения только уровней High и Medium.

На этом наша вводная часть окончена, давайте перейдём к срабатываниям. Разделим их на три части:

• логические ошибки;

• критические ошибки;

• ошибки при работе с памятью.

Логические ошибки

Фрагмент N1

Предупреждение PVS-Studio: V501 There are identical sub-expressions '(!esock_is_integer((env), (argv[3])))' to the left and to the right of the '||' operator. prim_socket_nif.c 6035

static ERL_NIF_TERM
nif_sendfile(ErlNifEnv*         env,
             int                argc,
             const ERL_NIF_TERM argv[])
{
  ....
  BOOLEAN_T    a2ok;

  if ((! (a2ok = GET_INT64(env, argv[2], &offset64))) ||
      (! GET_UINT64(env, argv[3], &count64u)))
  {
    if ((! IS_INTEGER(env, argv[3])) ||
        (! IS_INTEGER(env, argv[3])))
      return enif_make_badarg(env);

    if (! a2ok)
      return esock_make_error_integer_range(env, argv[2]);
    else
      return esock_make_error_integer_range(env, argv[3]);
  }
  ....
}

В коде разработчик пытался конвертировать 3-й и 4-й аргументы в 64-битные типы (знаковый и беззнаковый соответственно). Ранний выход из функции происходит в том случае, если один из аргументов не является числом. Однако здесь проверяется один и тот же аргумент.

Исправленный код:

....
  if ((! IS_INTEGER(env, argv[2])) ||
      (! IS_INTEGER(env, argv[3]))) 
                return enif_make_badarg(env);
....

Фрагмент N2

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'n->type == ycf_node_type_on_save_yield_state_code' to the left and to the right of the '||' operator. ycf_node.c 218

static void uniqify_local_vars_in_node(ycf_node* n)
{
  if (n->type == ycf_node_type_code_scope)
  {
    uniqify_local_vars_in_scope(&n->u.code_scope);
  }
  else if(n->type == ycf_node_type_on_restore_yield_state_code)
  {
    uniqify_local_vars_in_scope(&n->u.special_code_block
                                     .code.if_statement->u.code_scope);
  }
  else if (n->type == ycf_node_type_on_save_yield_state_code ||
           n->type == ycf_node_type_on_save_yield_state_code ||
           n->type == ycf_node_type_on_destroy_state_code ||
           n->type == ycf_node_type_on_return_code ||
           n->type == ycf_node_type_on_destroy_state_or_return_code)
  {
    uniqify_local_vars_in_scope(&n->u.special_code_block
                                     .code.if_statement->u.code_scope);
  }
  ....
}

Ещё один интересный пример копипасты. Перед нами функция uniqify_local_vars_in_node, которая рекурсивно обрабатывает узлы дерева, находя области видимости с локальными переменными и вызывая для них функцию унификации имён переменных. Однако в последней продемонстрированной ветке else if условие n->type == ycf_node_type_on_save_yield_state_code дублируется после оператора ||.

К сожалению, я не могу предложить корректное исправление для этого фрагмента. Судя по цепочке сравнений, перебираются значения следующего перечисления в порядке их объявления:

typedef enum
{
  ....
  ycf_node_type_on_save_yield_state_code,
  ycf_node_type_on_restore_yield_state_code,
  ycf_node_type_on_destroy_state_code,
  ycf_node_type_on_return_code,
  ycf_node_type_on_destroy_state_or_return_code
} ycf_node_type;

Однако константа ycf_node_type_on_restore_yield_state_code проверяется в предыдущем else if. Поэтому предполагаю, что здесь всё же есть дублирующая проверка, и её можно убрать.

Фрагмент N3

Предупреждение PVS-Studio: V1040 Possible typo in the spelling of a pre-defined macro name. The '__WIN_32__' macro is similar to '__WIN32__'. inet_drv.c 13506

static int tcp_send_error(tcp_descriptor* desc, int err)
{
   /* EPIPE errors usually occur in one of three ways:
    * 1. We write to a socket when we've already shutdown() the write side.
    *    On Windows the error returned for this is ESHUTDOWN
    *    rather than EPIPE.
    * 2. The TCP peer sends us an RST through no fault of our own (perhaps
    *    by aborting the connection using SO_LINGER) and we then attempt
    *    to write to the socket. On Linux and Windows we would actually
    *    receive an ECONNRESET error for this, but on the BSDs, Darwin,
    *    Illumos and presumably Solaris, it's an EPIPE.
    * 3. We cause the TCP peer to send us an RST by writing to a socket
    *    after we receive a FIN from them. Our first write will be
    *    successful, but if the they have closed the connection (rather
    *    than just shutting down the write side of it) this will cause their
    *    OS to send us an RST. Then, when we attempt to write to the socket
    *    a second time, we will get an EPIPE error. On Windows we get an
    *    ECONNABORTED.
    *
    * What we are going to do here is to treat all EPIPE messages that aren't
    * of type 1 as ECONNRESET errors. This will allow users who have the
    * show_econnreset socket option enabled to receive {error, econnreset} on
    * both send and recv operations to indicate that an RST
    * has been received.
    */
#ifdef __WIN_32__
  if (err == ECONNABORTED)
    err = ECONNRESET;
#endif
  if (err == EPIPE && !(desc->tcp_add_flags & TCP_ADDF_SHUTDOWN_WR_DONE))
    err = ECONNRESET;
  return tcp_send_or_shutdown_error(desc, err);
}

Здесь достаточно нестандартный случай проверки того, компилируется ли проект под Windows. В Erlang присутствуют и другие проверки на Windows ([1], [2], [3] и пр.), однако только в этом месте она отличается от всех остальных. Это вызывает сильное подозрение на простую опечатку (скорее всего, должно быть __WIN32__).

Всё же возникает вопрос: почему именно так? Стало интересно проверить, что можно найти по поисковому запросу "__WIN_32__". Результаты прилагаю:

Как можете заметить, результатов почти нет. Забавно, что одна из ссылок — это рассматриваемый нами файл на Debian Sources. Поиск grep'ом не нашёл подобного макроопределения по всему проекту. Было также предположение, что он может быть вшит в какой-то компилятор, например, MinGW. К сожалению, эксперимент окончился неудачей. Думаю, после всех попыток понять суть этого макроса, могу сказать, что этот код — ошибка.

В качестве исправления предлагаю воспользоваться проверкой, которую я находила ранее в других местах проекта:

static int tcp_send_error(tcp_descriptor* desc, int err)
{
  ....
#if defined(__WIN32__) || defined(_WIN32) || defined(_WIN32_)
  if (err == ECONNABORTED)
    err = ECONNRESET;
#endif
  ....
}

Фрагмент N4

Предупреждение PVS-Studio: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. dialyzer.c 246

int main(int argc, char** argv)
{
  ....
  if (argc > 2 && strcmp(argv[1], "+P") == 0)
  {
    PUSH2("+P", argv[2]);
    argc--, argv++;
    argc--, argv++;
  } else PUSH2("+P", "1000000");
  ....
}

Интересный пример ошибки, которую легко допустить при кодировании и не увидеть после. Давайте посмотрим, что из себя представляет конструкция PUSH2:

#define QUOTE(s) s
#define PUSH(s) eargv[eargc++] = QUOTE(s)
#define PUSH2(s, t) PUSH(s); PUSH(t)

Ага, это макрос, который раскроется в две конструкции. Раскроем код так, как бы это сделал препроцессор:

int main(int argc, char** argv)
{
  ....
  if (argc > 2 && strcmp(argv[1], "+P") == 0)
  {
    eargv[eargc++] = "+P"; eargv[eargc++] = argv[2];
    argc--, argv++;
    argc--, argv++;
  } else eargv[eargc++] = "+P"; eargv[eargc++] = "1000000";
  ....
}

В итоге, если проверка вычислится как false, будет условно исполнено лишь первое присвоение, а второе будет всегда происходить после ветвления. Вряд ли это то, что хотел получить разработчик.

Написание и использование макросов притягивает к себе ошибки. В идеале было бы избавиться от них, но давайте придём к менее радикальному исправлению. Когда требуется объединить несколько действий в одно в пределах макроса, это обычно делают с помощью конструкции do { .... } while (0):

#define PUSH2(s, t) do { PUSH(s); PUSH(t); } while (0)

Фрагмент N5

Предупреждение PVS-Studio: V654 The condition 'i >= 0' of loop is always true. wxe_return.cpp 236

ERL_NIF_TERM wxeReturn::make_array_objs(wxAuiPaneInfoArray& arr,
                                        WxeApp *app,
                                        const char *cname)
{
  ERL_NIF_TERM head, tail;
  ERL_NIF_TERM class_name = enif_make_atom(env, cname);
  tail = enif_make_list(env, 0);
  for (unsigned int i = arr.GetCount() - 1; i >= 0; i--)
  {
    head = make_ref(app->getRef((void *) &arr.Item(i),memenv), class_name);
    tail = enif_make_list_cell(env, head, tail);
  }
  return tail;
}

Обратите внимание на цикл for, а именно на его условие. Из-за того, что тип индуктивной переменной i — unsigned int, диапазон значений которого [0; UINT_MAX], цикл становится бесконечным. После итерации цикла c i == 0 в результате декремента переменная станет равна UINT_MAX, и цикл продолжится.

Исправим код следующим образом:

for (int64_t i = arr.GetCount() - 1; i >= 0; i--)
{
  ....
}

Аналогичные срабатывания:

• V654 The condition 'i >= 0' of loop is always true. wxe_return.cpp 248

• V654 The condition 'i >= 0' of loop is always true. wxe_return.cpp 260

Фрагмент N6

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2903, 3053. erl_bif_info.c 2903

BIF_RETTYPE system_info_1(BIF_ALIST_1)
{
  ....
  if (is_tuple(BIF_ARG_1)) {                                    // L2778
    ....
  } else if (BIF_ARG_1 == am_scheduler_id) {                    // L2782
    ....
  }
  ....
  else if (BIF_ARG_1 == am_garbage_collection) {                // L2903
    ....
  } else if (BIF_ARG_1 == am_fullsweep_after) {                 // L2921
    ....
  }
  else if (BIF_ARG_1 == am_garbage_collection) {                // L3053
    ....
  } else if (BIF_ARG_1 == am_instruction_counts) {              // L3056
    ....
  }
  ....
  else if (ERTS_IS_ATOM_STR("halt_flush_timeout", BIF_ARG_1)) { // L3552
    ....
  }
}

Анализатор обнаружил в функции, содержащей огромное количество if-else if (~800 строк кода), несколько веток с одинаковыми проверками. При этом логика в них отличается: первая проверка, вторая проверка. С учётом числа веток в этой функции и дистанции между одинаковыми проверками (150 строк) неудивительно, что такое могло произойти. Предотвратить такие случаи как раз и помогает статический анализ. К сожалению, мне сложно предложить вариант исправления, поэтому просто оставлю это разработчикам.

Фрагмент N7

Предупреждение PVS-Studio: V560 A part of conditional expression is always false: (arity == 0). bif.c 3145

BIF_RETTYPE delete_element_2(BIF_ALIST_3)
{
  ....
  ptr   = tuple_val(BIF_ARG_2);
  arity = arityval(*ptr);
  ix    = signed_val(BIF_ARG_1);

  if ((ix < 1) || (ix > arity) || (arity == 0))
  {
    BIF_ERROR(BIF_P, BADARG);
  }
  ....
}

Здесь у нас пример с математикой. Анализатор говорит о том, что последнее подвыражение arity == 0 всегда ложное. Почему же?

Давайте копать глубже. Если поток управления дойдёт до вычисления этого подвыражения, то мы будем знать, что два предыдущих подвыражения уже имеют результат false. При этом мы можем понять следующие вещи:

ix >= 1
ix <= arity

Перепишем эти выражения в математическом стиле:

1 <= ix <= arity

Далее вспоминаем про правило транзитивности, по которому получается, что arity >= 1. Следовательно, arity никогда не может быть равен 0 в этом контексте.

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

Критические ошибки

Фрагмент N8

Предупреждения PVS-Studio:

V557 Array overrun is possible. The value of 'prefix_len' index could reach 262. erl_alloc_util.c 5375

V557 Array overrun is possible. The value of 'prefix_len' index could reach 262. erl_alloc_util.c 5378

V557 Array overrun is possible. The value of 'prefix_len' index could reach 262. erl_alloc_util.c 5381

#define MAX_ATOM_CHARACTERS 255

static void make_name_atoms(Allctr_t *allctr)
{
  char alloc[] = "alloc";
  char realloc[] = "realloc";
  char free[] = "free";
  char buf[MAX_ATOM_CHARACTERS];
  size_t prefix_len = sys_strlen(allctr->name_prefix);

  if (prefix_len > MAX_ATOM_CHARACTERS + sizeof(realloc) - 1)
    erts_exit(ERTS_ERROR_EXIT,
              "Too long allocator name: %salloc\n"
              allctr->name_prefix);

  sys_memcpy((void *) buf, (void *) allctr->name_prefix, prefix_len);

  sys_memcpy((void *) &buf[prefix_len], (void *) alloc, sizeof(alloc) - 1);
  allctr->name.alloc = am_atom_put(buf, prefix_len + sizeof(alloc) - 1);

  sys_memcpy((void *) &buf[prefix_len], (void *) realloc, sizeof(realloc) - 1);
  allctr->name.realloc = am_atom_put(buf, prefix_len + sizeof(realloc) - 1);

  sys_memcpy((void *) &buf[prefix_len], (void *) free, sizeof(free) - 1);
  allctr->name.free = am_atom_put(buf, prefix_len + sizeof(free) - 1);
}

Итак, перед нами функция, которая генерирует имена аллоцирующих/деаллоцирующих функций на основе переданного префикса. Для того, чтобы их сгенерировать, используется буфер на 255 байт. Судя по содержимому функции, в формируемом буфере не будет нуль-терминала, так как в am_atom_put передаётся итоговый размер сформированной строки.

Самое первое, что делает разработчик, — это считает размер префикса. Далее он ограничивает размер префикса: если он больше суммы MAX_ATOM_CHARACTERS и длины строки realloc (самый длинный постфикс), то произойдёт вызов функции noreturn.

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

Чувствуете, где подвох? Проверка с ранним выходом выполнена некорректно. В буфере должно оставаться достаточно места, чтобы записать самый длинный постфикс (7 символов). Если префиксная строка будет размером от 256 до 262 символов, то раннего возврата не произойдёт, следовательно, функция sys_memcpy переполнит буфер. Поведение такой операции не определено.

Предполагая, что функция am_atom_put работает со строками длиной не более MAX_ATOM_CHARACTERS, поправим код следующим образом:

if (prefix_len > MAX_ATOM_CHARACTERS - sizeof(realloc) + 1)

Теперь максимальная длина префикса будет составлять 248 символов. При записи максимального постфикса переполнения не произойдёт.

Аналогичные срабатывания:

• V557 Array overrun is possible. The value of 'i' index could reach 40. ycf_lexer.c 493

• V557 Array overrun is possible. The value of 'n' index could reach 16. erl_call.c 1663

Фрагмент N9

Этот пример начну нестандартно. Для начала рассмотрим одну функцию:

static const char* event_state_flag_to_str(EventStateFlags f,
                                           const char *buff,
                                           int len)
{
  switch ((int)f)
  {
  case ERTS_EV_FLAG_CLEAR:
    return "CLEAR";
  case ERTS_EV_FLAG_USED:
    return "USED";

  /* other cases that return string literals */

  default:
    snprintf((char *)buff, len, "ERROR(%d)", f);
    return buff;
  }
}

Функция event_state_flag_to_str преобразует перечисление EventStateFlags в строку. Для не-default веток switch возвращает строковый литерал, в ином случае функция формирует строку в переданном буфере и возвращает указатель на него. Заметьте, как именно она это делает: у буфера принудительно отбрасывают константность. Согласно стандарту C, делать такое можно только если исходный буфер не был объявлен как const (C11 6.7.3.6, C23 6.7.4.1.7).

А так ли это на самом деле? Давайте взглянем на точку вызова:

static ERTS_INLINE void
print_flags(erts_dsprintf_buf_t *dsbufp, EventStateFlags f)
{
  const char buff[64];
  if (f & ERTS_EV_FLAG_WANT_ERROR)
  {
    erts_dsprintf(dsbufp, "WANTERR|");
    f &= ~ERTS_EV_FLAG_WANT_ERROR;
  }

  erts_dsprintf(dsbufp, "%s",
                event_state_flag_to_str(f, buff, sizeof(buff)));
}

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

V614 Uninitialized buffer 'buff' used. Consider checking the second actual argument of the 'event_state_flag_to_str' function. erl_check_io.c 2833

Исправим код, убрав константность у буфера:

static ERTS_INLINE void
print_flags(erts_dsprintf_buf_t *dsbufp, EventStateFlags f)
{
  char buff[64];
  if (f & ERTS_EV_FLAG_WANT_ERROR)
  {
    erts_dsprintf(dsbufp, "WANTERR|");
    f &= ~ERTS_EV_FLAG_WANT_ERROR;
  }
  erts_dsprintf(dsbufp, "%s",
                event_state_flag_to_str(f, buff, sizeof(buff)));
}

Также рекомендую поменять сигнатуру функции event_state_flag_to_str, чтобы второй параметр принимал указатель типа char *. Так будет понятно, что функция может модифицировать переданный буфер. Много кода мы при этом не поломаем, т.к. функция находится только в компилируемом файле и помечена как static.

Фрагмент N10

Предупреждение PVS-Studio: V1050 The uninitialized class member 'a' is used when initializing the base class 'BeamAssemblerCommon'. beam_asm.hpp 87

class BeamAssemblerCommon : public ErrorHandler
{
  BaseAssembler &assembler;
protected:
  BeamAssemblerCommon(BaseAssembler &assembler);
  ....
};

struct BeamAssembler : public BeamAssemblerCommon
{
  BeamAssembler() : BeamAssemblerCommon(a) { /* .... */ }

  protected:
    a64::Assembler a;
  ....
};

В этом фрагменте у нас небольшая иерархия классов: в конструктор базового класса (BeamAssemblerCommon) передаётся ссылка на нестатическое поле производного класса (BeamAssembler). Такая операция не приводит к проблемам до тех пор, пока в конструкторе базового класса не работают с этим полем. Почему так? Время жизни объекта из производного класса начинается, когда отработает нужный инициализатор в списке инициализации или поток управления попадёт в тело конструктора, если инициализатор отсутствует.

Вооружившись этой информацией, смотрим на конструктор базового класса. Он же не использует этот объект? Правда ведь?..

BeamAssemblerCommon::BeamAssemblerCommon(BaseAssembler &assembler_)
  : assembler(assembler_), ....
{
  ....
#ifdef DEBUG
  assembler.addDiagnosticOptions(DiagnosticOptions::kValidateAssembler);
#endif
  assembler.addEncodingOptions(EncodingOptions::kOptimizeForSize
                             | EncodingOptions::kOptimizedAlign);
  ....
}

Неправда :) У объекта производного класса по ссылке вызывается нестатическая функция-член, а его время жизни ещё не началось. И снова поведение не определено.

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

Ошибки при работе с памятью

Фрагмент N11

Предупреждение PVS-Studio: V586 The 'free' function is called twice for deallocation of the same memory space. erl_call.c 668

int main(int argc, char *argv[])
{
  int fd;
  char *p = NULL;
  ei_cnode ec;
  ....
  int i = 0;
  ei_x_buff reply;
  ....
  if (ei_rpc(&ec, fd, "c", "c", p, i, &reply) < 0)
  {
    free(p);
    ei_x_free(&reply);
    fprintf(stderr,"erl_call: can't compile file %s\n", fname);
  }
  free(p);
  /* FIXME complete this code
     FIXME print out error message as term
  if (!erl_match(erl_format("{ok,_}"), reply)) {
      fprintf(stderr,"erl_call: compiler errors\n");
  }
  */
  ei_x_free(&reply);
  ....
}

Судя по названию, ei_rpc — это удалённый вызов процедуры (Remote Procedure Call). Отрицательное число возвращается в случае неуспеха операции. Если удалённый вызов не отработал, начинается очистка некоторых ресурсов. Указатель p передаётся в функцию free и происходит логирование ошибки в stderr. Дальше, вне зависимости от результата вызова функции, указатель снова передаётся во free. Получаем двойное освобождение памяти.

Решение проблемы довольно простое: нужно или прервать выполнение программы, или занулить освобождённые указатели. Я бы исправила проблему, прервав исполнение в этой ветке:

if (ei_rpc(&ec, fd, "c", "c", p, i, &reply) < 0)
{
  free(p);
  ei_x_free(&reply);
  fprintf(stderr,"erl_call: can't compile file %s\n", fname);
  return 1;
}

free(p);

Фрагмент N12

Предупреждение PVS-Studio: V595 The 'pkey' pointer was utilized before it was verified against nullptr. Check lines: 581, 582. pkey.c 581

static int get_pkey_public_key(ErlNifEnv *env,
                               const ERL_NIF_TERM argv[],
                               int algorithm_arg_num,
                               int key_arg_num,
                               EVP_PKEY **pkey,
                               ERL_NIF_TERM *err_return)
{
  ....
  if (enif_is_map(env, argv[key_arg_num]))
  {
    password = get_key_password(env, argv[key_arg_num]);
    *pkey = ENGINE_load_public_key(e, id, NULL, password);
    if (!pkey)
      assign_goto(*err_return, err,
                  EXCP_BADARG_N(env, key_arg_num,
                                "Couldn't get public key from engine"));
  }
  /* other branches */
  
  ret = 1;

 done:
  ....
  return ret;

 err:
  ....
  *pkey = NULL;
  ret = 0;
  goto done;
}

Перед нами достаточно занятный фрагмент. Функция пытается извлечь публичный ключ в 5-й выходной параметр и возвращает в случае успеха 1, а в случае неудачи — 0. В последнем случае публичный ключ также обнуляется.

Анализатор предупреждает, что в одной из веток функции указатель pkey сначала разыменовывается, а затем проверяется на валидность. При этом в других ветках такой проверки не наблюдается. Делаем вывод, что у функции есть контракт "параметр pkey никогда не должен быть равен нулевому указателю". Убедиться в этом можно, если поискать вызовы этой функции в этом же файле ([1], [2], [3]), — везде передаётся адрес переменной.

На самом деле, разработчик хотел обработать некорректный результат функции ENGINE_load_public_key, прыгнув на метку err, но в итоге опечатался, забыв написать звёздочку перед оператором !. Современный компилятор удалит такую проверку на релизе, что также приведёт к тому, что функция может вернуть 1 и нулевой указатель в пятом параметре.

Поправим проверку:

....
*pkey = ENGINE_load_public_key(e, id, NULL, password);
if (!*pkey)
  assign_goto(*err_return, err,
              EXCP_BADARG_N(env,
                            key_arg_num,
                            "Couldn't get public key from engine"));
....

Фрагмент N13

Довольно часто нам в поддержку пишут, что анализатор ругается на то, что во free толкается нулевой указатель. Ведь по стандарту ничего страшного не произойдёт. Мы даже писали про похожий случай статью.

Да, ничего страшного нет, но этот код, скорее всего, подозрителен. И это помогло найти интересную утечку памяти в коде. Давайте посмотрим на неё.

Предупреждение PVS-Studio: V575 The null pointer is passed into 'free' function. Inspect the first argument. erl_misc_utils.c 264

void erts_cpu_info_destroy(erts_cpu_info_t *cpuinfo)
{
  if (cpuinfo)
  {
    cpuinfo->configured = 0;
    cpuinfo->online = 0;
    cpuinfo->available = 0;
#ifdef HAVE_PSET_INFO
    if (cpuinfo->cpuids)
      free(cpuinfo->cpuids);
#endif
    cpuinfo->topology_size = 0;
    if (cpuinfo->topology)
    {
      cpuinfo->topology = NULL;
      free(cpuinfo->topology);
    }
    free(cpuinfo);
  }
}

В этом фрагменте указатель cpuinfo->topology явно устанавливается в NULL перед вызовом free. По итогу операция освобождения памяти бессмысленна, так как free(NULL) не выполняет никаких действий. Это приводит к утечке памяти, потому что оригинальный блок памяти, на который указывал cpuinfo->topology, никогда не освобождается.

Исправим проблему, поменяв операции местами:

if (cpuinfo->topology)
{
  free(cpuinfo->topology);
  cpuinfo->topology = NULL;
}

Фрагмент N14

Предупреждение PVS-Studio: V773 The function was exited without releasing the 'entry' pointer. A memory leak is possible. wxe_gl.cpp 88

typedef struct _wxe_glc
{
  wxGLCanvas *canvas;
  wxGLContext *context;
} wxe_glc;

void setActiveGL(wxeMemEnv *memenv,
                 ErlNifPid caller,
                 wxGLCanvas *canvas,
                 wxGLContext *context)
{
  ErlNifUInt64 callId = wxe_make_hash(memenv->tmp_env, &caller);
  wxe_glc * entry = glc[callId];
  gl_active_index = callId;
  gl_active_pid = caller;

  if (!entry)
  {
    entry = (wxe_glc *) malloc(sizeof(wxe_glc));
    entry->canvas = NULL;
    entry->context = NULL;
  }

  if (entry->canvas == canvas && entry->context == context)
    return;

  entry->canvas = canvas;
  entry->context = context;
  glc[gl_active_index] = entry;
  ....
}

В функции setActiveGL объявлен указатель entry, который инициализируют элементом из массива. Если этот элемент оказывается нулевым указателем, то происходит выделение памяти через malloc. Далее происходит сравнение указателей. Если оно вычислится как true, то произойдёт ранний возврат. При этом если динамическая аллокация произошла, то память не освободили.

Да, такая ситуация, скорее, редкость и никогда не произойдёт, ведь для этого нужен целый комплекс событий:

• при вызове функции 3-м и 4-м аргументами передали нулевые указатели;

• в glc[callId] находится нулевой указатель.

Но мы можем сделать так, чтобы даже теоретически утечка никогда не происходила. Немного модифицируем код:

ErlNifUInt64 callId = wxe_make_hash(memenv->tmp_env, &caller);
wxe_glc * entry = glc[callId];
gl_active_index = callId;
gl_active_pid = caller;

bool new_alloc = false;

if (!entry)
{
  entry = (wxe_glc *) malloc(sizeof(wxe_glc));
  entry->canvas = NULL;
  entry->context = NULL;
  new_alloc = true;
}

if (entry->canvas == canvas && entry->context == context)
{
  if (new_alloc)
  {
    free(entry);
  }

  return;
}
....

Аналогичные срабатывания

• V773 The function was exited without closing the file referenced by the 'f' handle. A resource leak is possible. erl_poll.c 2423

• V773 The 'erl_errno_p' pointer was assigned values twice without releasing the memory. A memory leak is possible. ei_pthreads.c 195

• V773 The function was exited without closing the file referenced by the 'fp' handle. A resource leak is possible. cpu_sup.c 433

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1300

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1330

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1663

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1690

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1715

• V773 The exception was thrown without releasing the 'alpha' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1718

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1747

• V773 The exception was thrown without releasing the 'alpha' pointer. A memory leak is possible. wxe_wrapper_4.cpp 1750

• V773 The exception was thrown without releasing the 'alpha' pointer. A memory leak is possible. wxe_wrapper_4.cpp 2680

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 2715

• V773 The exception was thrown without releasing the 'data' pointer. A memory leak is possible. wxe_wrapper_4.cpp 2733

Фрагмент N15

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'outbuf_base' is lost. Consider assigning realloc() to a temporary pointer. run_erl.c 1324

static void outbuf_append(const char* buf, int n)
{
  ....
  /*
   * Allocate a larger buffer if we still cannot fit the data.
   */
  if (outbuf_base+outbuf_total < outbuf_in+n)
  {
    int size = outbuf_in - outbuf_out;
    outbuf_total = size+n;
    outbuf_base = realloc(outbuf_base, outbuf_total);
    outbuf_out = outbuf_base;
    outbuf_in = outbuf_base + size;
  }
  ....
}

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

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

Получается, если сразу же перезаписывать исходный указатель вызовом realloc, то получим утечку памяти.

Исправим код следующим образом:

static void outbuf_append(const char* buf, int n)
{
  ....
  /*
   * Allocate a larger buffer if we still cannot fit the data.
   */
  if (outbuf_base + outbuf_total < outbuf_in + n)
  {
    int size = outbuf_in - outbuf_out;
    outbuf_total = size+n;
    char *tmp = realloc(outbuf_base, outbuf_total);
    if (!tmp)
    {
      /* somehow handle this scenario
       * the `outbuf_base` buffer here is still valid */
    }

    outbuf_base = tmp;
    outbuf_out = outbuf_base;
    outbuf_in = outbuf_base + size;
  }
  ....
}

В исправленном примере тоже нужно как-то обработать ситуацию, когда realloc возвращает нулевой указатель. Это может быть аварийное завершение работы, попытка выделить буфер другого размера и др.

Фрагмент N16

Предупреждение PVS-Studio: V769 The 'end' pointer in the 'end - mm->sua.top' expression equals nullptr. The resulting value is senseless and it should not be used. erl_mmap.c 2411

static void
add_free_desc_area(ErtsMemMapper* mm, char *start, char *end)
{
  ERTS_MMAP_ASSERT(end == (void *) 0 || end > start);
  if (sizeof(ErtsFreeSegDesc) <= ((UWord) end) - ((UWord) start))
  {
  ....
  }
  ....
}

void erts_mmap_init(ErtsMemMapper* mm, ErtsMMapInit *init)
{
  ....
  if (end == (void *) 0)
  {
    /*
     * Very unlikely, but we need a guarantee
     * that `mm->sua.top` always will
     * compare as larger than all segment pointers
     * into the super carrier...
     */
    mm->sua.top -= ERTS_PAGEALIGNED_SIZE;
    mm->size.supercarrier.used.total += ERTS_PAGEALIGNED_SIZE;
#ifdef ERTS_HAVE_OS_PHYSICAL_MEMORY_RESERVATION
    if (!virtual_map || os_reserve_physical(mm->sua.top, ERTS_PAGEALIGNED_SIZE))
#endif
      add_free_desc_area(mm, mm->sua.top, end);
    mm->desc.reserved += (end - mm->sua.top) / sizeof(ErtsFreeSegDesc);
  }
  ....
}

Анализатор обнаружил подозрительную адресную арифметику. Давайте посмотрим, что тут не так.

Мы попадаем в ветку кода, когда end — это действительно нулевой указатель. Поле mm->sua.top также указатель, но о его значении мы пока ничего сказать не можем. Стандарт C определяет поведение при разнице указателей только в случае, когда они оба указывают на элементы одного и того же массива (C11 6.5.6.8; C23 6.5.7.10). Нулевой указатель не указывает на какой-либо элемент массива. Следовательно, поведение такой операции не определено, что не есть хорошо.

Мне стало интересно, а что же происходит одной строкой выше, в функции add_free_desc_area?

static void
add_free_desc_area(ErtsMemMapper* mm, char *start, char *end)
{
  ERTS_MMAP_ASSERT(end == (void *) 0 || end > start);
  if (sizeof(ErtsFreeSegDesc) <= ((UWord) end) - ((UWord) start)) {
  ....
}

И тут тоже вычитание указателей, с небольшим "но"... перед этим указатели приводят к числам. А это уже поведение, определяемое реализацией (C11 6.3.2.3.6; C23 6.3.2.3.6), поэтому тут ситуация уже много лучше.

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

Заключение

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

Современные технологии не стоят на месте, и сегодня у разработчиков есть мощные инструменты для повышения надёжности кода. Например, статические анализаторы помогают выявлять скрытые ошибки и потенциальные уязвимости. Предлагаю вам самим в этом убедиться :) Получить пробную версию анализатора PVS-Studio можно здесь.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Elizaveta Zhegalova. Code legacy: Analyzing Erlang's C and C++ modules that have been running for decades.