В этой статье я хотел бы рассказать об анализе проекта ReOpenLDAP. Он был разработан для решения проблем, с которыми столкнулось ПАО (ПАО) «МегаФон», крупнейший российский оператор мобильной связи, при использовании OpenLDAP в своей инфраструктуре. Сейчас ReOpenLDAP успешно используется в филиалах МегаФона по всей России, поэтому мы подумали, что было бы интересно проверить такой высоконагруженный проект, как этот, с помощью нашего статического анализатора PVS-Studio.
Введение
ReOpenLDAP, также известный как TelcoLDAP, является ответвлением проекта OpenLDAP, созданным российскими разработчиками для использования в телекоммуникационной отрасли, с множеством исправлений ошибок и добавлением мультимастерной кластеризации с горячей репликацией. ReOpenLDAP — это реализация C-сервера с протоколом LDAP с открытым исходным кодом.
ReOpenLDAP показывает высокий уровень производительности:
- До 50 тысяч LDAP-изменений в секунду
- До 100 тысяч LDAP-запросов в секунду
Следует отметить, что ReOpenLDAP унаследовал 3185 операторов goto от OpenLDAP, которые довольно сильно усложняют процесс анализа. Несмотря на это, PVS-Studio все же удалось найти некоторое количество ошибок.
Пожалуйста, зарегистрируйтесь для бета-тестирования PVS-Studio для Linux
Эта статья стала возможной благодаря разработке версии PVS-Studio для Linux, которую мы начали недавно: именно на Linux была сделана проверка проекта ReOpenLDAP. Однако существует угроза того, что версия для Linux может прекратить свое существование до того, как она выйдет, поскольку мы не видим большого интереса со стороны потенциальных пользователей. Если вы посмотрите на некоторые обсуждения на форуме, вы можете подумать, что самая большая проблема PVS-Studio — это отсутствие поддержки Linux, но когда мы начали искать бета-тестеров, очень немногие откликнулись. Примечание: история нашего поиска энтузиастов была рассказана в статье «PVS-Studio признается в любви к Linux».
Должен отметить, что нас не слишком беспокоит бета-тестирование. Почему-то некоторые относятся ко всему происходящему так, как будто мы специально затеяли эту кампанию, чтобы привлечь программистов, которые будут выполнять за нас работу бесплатных тестировщиков. Конечно, это далеко не так: мы могли бы протестировать наш инструмент самостоятельно. Просто небольшое количество ответов говорит о том, что нам, вероятно, следует замедлить или даже приостановить работу над этой версией. К сожалению, желающих принять участие действительно очень мало. В свете всего этого Unicorn обращается ко всем Linux-программистам.
Пожалуйста, запишитесь на бета-тестирование Linux-версии PVS-Studio: так мы увидим, что люди действительно заинтересованы в нашем инструменте. Вот напоминание о том, как подать заявку.
Если вы хотите помочь нам в тестировании PVS-Studio на Linux, напишите нам по адресу [email protected]. В теме сообщения укажите «PVS-Studio for Linux, Beta», чтобы мы могли быстрее обрабатывать письма. Пожалуйста, отправьте сообщение с вашего корпоративного адреса электронной почты и обязательно напишите несколько слов о себе. Мы будем признательны за помощь от каждого, но пожелания и предложения наших потенциальных клиентов будут рассмотрены в первую очередь.
Также, пожалуйста, ответьте на следующие вопросы в своем электронном письме:
- С какой операционной системой вы собираетесь использовать анализатор?
- Какую IDE вы используете?
- Какой компилятор вы используете для сборки своих проектов?
- Какую систему сборки вы используете?
Как только рабочая версия будет готова, мы отправим электронное письмо всем, кто подал заявку. Спасибо всем заранее!
Результаты анализа
Ошибка приоритета операции
Диагностическое сообщение PVS-Studio: V593 Рассмотрим выражение вида A = B == C. Выражение рассчитывается следующим образом: 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 Указатель ‘key’ был использован до того, как он был проверен на nullptr. Проверить строки: 1324, 1327. mdb.c 1324
char *
mdb_dkey(MDB_val *key, char *buf)
{
....
unsigned char *c = key->mv_data; // <=
....
if (!key) // <=
return "";
....
}
Указатель key проверяется на NULL в блоке if, что означает, что программист предполагает, что этот указатель может быть нулевым. Однако несколькими строками ранее он уже использовался без какой-либо проверки. Чтобы избежать этой ошибки, необходимо проверить указатель key перед его использованием.
Аналогичная ошибка:
- V595 Указатель «ключ» использовался до того, как он был проверен на соответствие nullptr. Контрольные строки: 7282, 7291. mdb.c 7282
Подозрительный тернарный оператор
Диагностическое сообщение PVS-Studio: V583 Оператор ‘?:’, независимо от его условного выражения, всегда возвращает одно и то же значение: vlvResult. общий.с 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 Периодическая проверка. Условие if (s-›state.r == 0) уже проверено в строке 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 Периодическая проверка. Условие «def-›mrd_usage & 0x0100U» уже проверено в строке 319. mr.c 322
Перезапись параметра
Диагностическое сообщение PVS-Studio: V763 Параметр rc всегда перезаписывается в теле функции перед использованием. 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 Неверный формат. Рассмотрите возможность проверки четвертого фактического аргумента функции snprintf. Ожидается аргумент SIGNED типа memsize. конн.с 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 не соответствует аргументу c-›c_connid, переданному в snprintf. Вместо этого следует использовать %lu , который является подходящим спецификатором для unsigned long . Использование %ldвместо %lu приведет к печати неверных значений, если аргументы достаточно велики.
Другие подобные ошибки:
- V576 Неверный формат. Рассмотрите возможность проверки третьего фактического аргумента функции fprintf. Ожидается аргумент целочисленного типа SIGNED. ure.c 1865
- V576 Неверный формат. Рассмотрите возможность проверки третьего фактического аргумента функции fprintf. Ожидается аргумент SIGNED типа memsize. инструменты.c 211
- V576 Неверный формат. Рассмотрите возможность проверки четвертого фактического аргумента функции fprintf. Ожидается аргумент целочисленного типа UNSIGNED. mdb.c 1253
Недостаточный указатель
Сообщение диагностики PVS-Studio: V528 Странно, что указатель на тип char сравнивается со значением \0. Вероятно имелось в виду: *ludp-›lud_filter != ‘\0’. бэкенд.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 Странно, что указатель на тип char сравнивается со значением '\0'. Вероятно имелось в виду: *(* lsei)-›lsei_values[0] == ‘\0’. синтаксис.c 240
- V528 Странно, что указатель на тип char сравнивается со значением '\0'. Вероятно имелось в виду: *(* lsei)-›lsei_values[1] != ‘\0’. синтаксис.с 241
Избыточная проверка
Диагностическое сообщение PVS-Studio: V560 Часть условного выражения всегда истинна: !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 ) {
....
}
....
}
saveit проверяется на null в ветке else , что не имеет смысла, поскольку оно уже было проверено в первом условии. Такая избыточная проверка только усложняет код. Возможно, это даже не ошибка, а вместо этого программист хотел проверить что-то другое.
Однако первый вариант более вероятен, поэтому код следует упростить:
if ( saveit || op->o_tag == LDAP_REQ_ADD ) {
....
} else if ( op->o_tag == LDAP_REQ_MODRDN ) {
....
}
Опасное использование realloc
Диагностическое сообщение PVS-Studio: V701 возможная утечка realloc(): когда realloc() не удается выделить память, исходный указатель lud.lud_exts теряется. Рассмотрите возможность назначения realloc() временному указателю. 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 Переменной ca.argv дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 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 со своими проектами.
