В этой статье я хочу рассказать о проверке проекта ReOpenLDAP. Этот проект был реализован для решения проблем, возникших при эксплуатации OpenLDAP в инфраструктуре ПАО МегаФон - крупнейшего в России оператора мобильной связи. Сейчас ReOpenLDAP успешно работает в филиалах МегаФон по всей России, поэтому было бы интересно проверить столь высоконагруженный проект при помощи статического анализатора PVS-Studio.
ReOpenLDAP, также известный как "TelcoLDAP" — это форк проекта OpenLDAP, созданный российскими разработчиками для применения в телекоммуникационной индустрии, с исправлением массы ошибок и работающей репликацией в мульти-мастер топологии. ReOpenLDAP является открытой реализацией сервера протокола LDAP на языке C.
ReOpenLDAP обеспечивает высокий уровень производительности:
Стоит отметить, что в наследство от OpenLDAP проект ReOpenLDAP получил 3185 операторов goto, которые в значительной мере затрудняют анализ, но даже несмотря на это было найдено некоторое количество ошибок.
Написание этой статьи стало возможным благодаря началу работ над созданием 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 до того, как его использовать.
Аналогичное предупреждение:
Предупреждение 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. Однако, это лишь предположение, но в любом случае, разработчикам следует проверить этот участок кода, и либо скорректировать одно из условий, либо удалить дублирующую проверку.
Аналогичное место:
Предупреждение 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" приведет к выводу неверных значений, при достаточно больших аргументах.
Аналогичные предупреждения:
Предупреждение 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')
....Аналогичные неразыменованные указатели:
Предупреждение 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 ) {
....
}Предупреждение 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 на своем собственном проекте.