Егор Бредихин

22 Авг 2016

Теги:

#Cpp

По просьбе читателей, проверяем код LDAP-сервера ReOpenLDAP

22 Авг 2016

Автор: Егор Бредихин

Введение
Прошу записываться на тестирование Beta-версии PVS-Studio под Linux
Результаты проверки
Заключение

В этой статье я хочу рассказать о проверке проекта ReOpenLDAP. Этот проект был реализован для решения проблем, возникших при эксплуатации OpenLDAP в инфраструктуре ПАО МегаФон - крупнейшего в России оператора мобильной связи. Сейчас ReOpenLDAP успешно работает в филиалах МегаФон по всей России, поэтому было бы интересно проверить столь высоконагруженный проект при помощи статического анализатора PVS-Studio.

Введение

ReOpenLDAP, также известный как "TelcoLDAP" — это форк проекта OpenLDAP, созданный российскими разработчиками для применения в телекоммуникационной индустрии, с исправлением массы ошибок и работающей репликацией в мульти-мастер топологии. ReOpenLDAP является открытой реализацией сервера протокола LDAP на языке C.

ReOpenLDAP обеспечивает высокий уровень производительности:

Более 50 тысяч LDAP-изменений в секунду
Более 100 тысяч LDAP-запросов в секунду

Стоит отметить, что в наследство от OpenLDAP проект ReOpenLDAP получил 3185 операторов goto, которые в значительной мере затрудняют анализ, но даже несмотря на это было найдено некоторое количество ошибок.

Прошу записываться на тестирование Beta-версии PVS-Studio под Linux

Написание этой статьи стало возможным благодаря началу работ над созданием PVS-Studio for Linux. Именно в Linux мы проверяли проект ReOpenLDAP. Однако есть угроза, что Linux-версия закончит своё существование ещё до своего выхода в свет. Причина - мало практического интереса. Когда идёт какое-то обсуждение на форуме, то кажется, что самая большая проблема продукта PVS-Studio – это отсутствие версии для Linux. Но когда дело дошло до сбора контактов людей, готовых поучаствовать в тестировании Beta-версии, то оказалось, что их совсем мало. Примечание: мы писали о поиске энтузиастов в статье "PVS-Studio признаётся в любви к Linux".

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

Просим вступить вас в ряды Beta-тестеров PVS-Studio для Linux. Это покажет, что к инструменту есть практический интерес. Ещё раз напишу, как можно вступить в ряды энтузиастов.

Если вы хотите помочь нам проверить работу PVS-Studio для Linux, прошу написать нам. Чтобы письма можно было проще обрабатывать, просим указать в теме письма строчку "PVS-Studio for Linux, Beta". Письма отправляйте по адресу support@viva64.com. Просим писать письма с корпоративных ящиков и кратко представиться. Мы будем благодарны всем, кто откликнется, но в первую очередь мы будем учитывать пожелания и рекомендации тех людей, которые потенциально со временем могут стать нашими клиентами.

Также прошу в письме дать ответы на следующие вопросы:

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

Когда появится версия, которую можно будет попробовать, мы напишем всем откликнувшимся письма. Заранее всем спасибо.

Результаты проверки

Ошибка в приоритете операций

Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. mdb_dump.c 150

static int dumpit(....)
{
  ....
  while ((rc = mdb_cursor_get(...) == MDB_SUCCESS)) {
    ....
  }
  ....
}

Автор ошибся с расположением закрывающейся круглой скобки в условии цикла while, что привело к ошибке в приоритете операций. В результате сначала выполняется сравнение, а затем результат этого сравнения записывается в переменную rc.

Данный код следует исправить так:

while ((rc = mdb_cursor_get(...)) == MDB_SUCCESS) {
  ....
}

Работа с нулевым указателем

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

char *
mdb_dkey(MDB_val *key, char *buf)
{
  ....
  unsigned char *c = key->mv_data; // <=
  ....
  if (!key)                        // <=
    return "";
  ....
}

В блоке if происходит проверка указателя key на NULL, следовательно, автором допускается возможность равенства этого указателя нулю, однако выше указатель уже используется без проверки. Чтобы избежать ошибки, следует проверить значение указателя key до того, как его использовать.

Аналогичное предупреждение:

V595 The 'key' pointer was utilized before it was verified against nullptr. Check lines: 7282, 7291. mdb.c 7282

Подозрительный тернарный оператор

Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: "vlvResult". common.c 2119

static int
print_vlv(....)
{
  ....
  tool_write_ldif( ldif ? LDIF_PUT_COMMENT : LDIF_PUT_VALUE,
      ldif ? "vlvResult" : "vlvResult", buf, rc ); // <=
  }
  ....
}

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

....
tool_write_ldif( ldif ? LDIF_PUT_COMMENT : LDIF_PUT_VALUE,
      ldif ? "vlvResult: " : "vlvResult", buf, rc );
....

Вероятная опечатка в имени поля

Предупреждение PVS-Studio: V571 Recurring check. The 'if (s->state.r == 0)' condition was already verified in line 147. rurwl.c 148

void rurw_r_unlock(....) {
  ....
  if (s->state.r == 0) {  // <=
    if (s->state.r == 0)  // <=
      s->thr = 0;
    p->rurw_readers -= 1;
  }
  ....
}

Здесь производится проверка одного и того же условия дважды. Если обратить внимание на аналогичные места в исходниках, например:

void rurw_w_unlock(....) {
  ....
  if (s->state.w == 0) {
    if (s->state.r == 0)
      s->thr = 0;
    p->rurw_writer = 0;
  }
  ....
}

то можно предположить, что в одном из условий имелась в виду проверка s->state.w == 0. Однако, это лишь предположение, но в любом случае, разработчикам следует проверить этот участок кода, и либо скорректировать одно из условий, либо удалить дублирующую проверку.

Аналогичное место:

V571 Recurring check. The 'def->mrd_usage & 0x0100U' condition was already verified in line 319. mr.c 322

Перезапись параметра

Предупреждение PVS-Studio: V763 Parameter 'rc' is always rewritten in function body before being used. tls_o.c 426

static char *
tlso_session_errmsg(...., int rc, ....)
{
  char err[256] = "";
  const char *certerr=NULL;
  tlso_session *s = (tlso_session *)sess;
  rc = ERR_peek_error(); // <=
  ....
}

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

Неверный спецификатор форматирования

Предупреждение PVS-Studio: V576 Incorrect format. Consider checking the fourth actual argument of the 'snprintf' function. The SIGNED argument of memsize type is expected. conn.c 309

struct Connection {
  ....
  unsigned long c_connid;
  ....
}
....
static int
conn_create(....)
{
  ....
  bv.bv_len = snprintf( buf, sizeof( buf ),
                        "cn=Connection %ld", // <=
                        c->c_connid );
  ....
}

Спецификатор форматирования "%ld" не соответствует передаваемому в snprintf аргументу c->c_connid. Правильным спецификатором для unsigned long является "%lu". Использование "%ld" вместо "%lu" приведет к выводу неверных значений, при достаточно больших аргументах.

Аналогичные предупреждения:

V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. The SIGNED integer type argument is expected. ure.c 1865
V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. The SIGNED argument of memsize type is expected. tools.c 211
V576 Incorrect format. Consider checking the fourth actual argument of the 'fprintf' function. The UNSIGNED integer type argument is expected. mdb.c 1253

Неразыменованный указатель

Предупреждение PVS-Studio: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *ludp->lud_filter != '\0'. backend.c 1525

int
fe_acl_group(....)
{
  ....
  if ( ludp->lud_filter != NULL &&
       ludp->lud_filter != '\0') // <=
  { 
    ....
  }
}

Автор кода хотел выявить ситуацию, когда указатель нулевой или строка пустая. Но забыл разыменовать указатель ludp->lud_filter, поэтому указатель просто дважды проверится на равенство NULL.

Следует разыменовать указатель:

  ....
  if ( ludp->lud_filter != NULL &&
       *ludp->lud_filter != '\0')
  ....

Аналогичные неразыменованные указатели:

V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *(* lsei)->lsei_values[0] == '\0'. syntax.c 240
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *(* lsei)->lsei_values[1] != '\0'. syntax.c 241

Лишняя проверка

Предупреждение PVS-Studio: V560 A part of conditional expression is always true: !saveit. syncprov.c 1510

static void
syncprov_matchops( Operation *op, opcookie *opc, int saveit )
{
  ....
  if ( saveit || op->o_tag == LDAP_REQ_ADD ) {
    ....
  } else if ( op->o_tag == LDAP_REQ_MODRDN && !saveit ) {
    ....
  }
  ....
}

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

Но скорее всего код достаточно упростить:

if ( saveit || op->o_tag == LDAP_REQ_ADD ) {
  ....
} else if ( op->o_tag == LDAP_REQ_MODRDN ) {
  ....
}

Опасное использование realloc

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

int
main( int argc, char *argv[])
{
  ....
  lud.lud_exts = (char **)realloc( lud.lud_exts,
    sizeof( char * ) * ( nexts + 2 ) );
  ....
}

Выражение вида foo = realloc(foo, ....) является потенциально опасным. При невозможности выделения памяти realloc вернет нулевой указатель, перезаписав предыдущее значение указателя. Чтобы предотвратить это, рекомендуется сохранять значение указателя в дополнительной переменной перед использованием realloc.

Перезапись значения

Предупреждение PVS-Studio: V519 The 'ca.argv' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 7774, 7776. bconfig.c 7776

int
config_back_initialize( BackendInfo *bi )
{
  ....
  ca.argv = argv;      // <=
  argv[ 0 ] = "slapd";
  ca.argv = argv;      // <=
  ca.argc = 3;
  ca.fname = argv[0];
  ....
}

Если данный код корректен, то следует удалить первое лишнее присваивание.

Заключение

ReOpenLDAP является проектом, призванным обеспечить стабильную работу под высокой нагрузкой. Поэтому разработчики ответственно подходят к тестированию и используют специализированные инструменты, такие как ThreadSanitizer и Valgrind. Однако, как мы видим, этого не всегда достаточно, так как при помощи PVS-Studio был выявлен ряд ошибок, хотя их и мало.

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

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

#Cpp