Пришло время вновь проверить проект FreeBSD и продемонстрировать, что даже в таких серьезных и качественных проектах анализатор PVS-Studio легко находит ошибки. В этот раз я решил взглянуть на поиск ошибок с точки зрения обнаружения потенциальных уязвимостей. Анализатор PVS-Studio всегда умел выявлять дефекты, которые потенциально можно использовать для атаки. Но мы никогда не акцентировали на этом внимание и описывали ошибки как опечатки, последствия неудачного Copy-Paste и так далее, и не классифицировали их, например, согласно CWE. Сейчас очень популярно говорить и о безопасности, и об уязвимостях, поэтому попробую немного расширить ваше восприятие нашего анализатора. PVS-Studio - это не только поиск багов, но ещё и инструмент, повышающий безопасность кода.
Отчет о нашей предыдущей проверке FreeBSD в 2016 году можно посмотреть здесь.
Как следует из названия, то, что будет описано в статье, я нашел за один вечер. Т.е. на поиск потенциальных уязвимостей я потратил всего 2-3 часа. Это говорит о всей мощи статического анализатора PVS-Studio. Я рекомендую использовать наш анализатор всем, кого заботит качество кода, а тем более его надёжность и устойчивость к возможным атакам.
Выписал код с ошибками я быстро, а вот найти время оформить свои изыскания в виде этой статьи я не мог три недели. За это время мы даже поправили некоторые из ошибок, которые будут описаны в статье, в рамках нового направления "Дефекты безопасности, которые устранила команда PVS-Studio на этой неделе": выпуск N2, выпуск N3.
Правили мы то, что попроще и где понятно, как поправить, не вдаваясь в суть алгоритмов. Поэтому авторам FreeBSD не стоит ограничиваться нашими правками и даже этой статьей, а имеет смысл провести анализ кода самостоятельно и более подробно. Я готов выдать им временный ключ для полноценной проверки, а также помочь в борьбе с ложными срабатываниями, которые будут им мешать. Кстати, о ложных срабатываниях...
Проверив проект с помощью PVS-Studio, можно получить очень большой разброс по количеству ложных срабатываний. Например, мы недавно проверяли проект FAR, и количество ложных срабатываний составило 50%. Это отличный результат, означающий, что каждое второе сообщение указывает на ошибку или крайне плохой код. А при проверке Media Portal 2 результат был ещё лучше: 27% ложных срабатываний.
С проектом FreeBSD всё сложнее. Дело в том, что анализатор выдал огромное количество предупреждений общего назначения:
Большинство сообщений, естественно, являются ложными срабатываниями. Посчитать сложно, но думаю, ложных срабатываний будет около 95%.
О чем это говорит? Это говорит, что нет смысла рассуждать о количестве ложных срабатываний на больших проектах, не проведя предварительную настройку анализатора. Большинство ложных предупреждений возникает из-за разных макросов и их легко убрать, используя различные механизмы, предусмотренные в PVS-Studio. Поясню это на примере.
В коде FreeBSD можно встретить вот такой массив:
#ifdef Q
#undef Q
#endif
#define Q(_r) \
(((_r) == 1.5) ? 0 : (((_r) ==2.25) ? 1 : (((_r) == 3) ? 2 : \
(((_r) == 4.5) ? 3 : (((_r) == 6) ? 4 : (((_r) == 9) ? 5 : \
(((_r) == 12) ? 6 : (((_r) == 13.5)? 7 : 0))))))))
static const struct txschedule series_quarter[] = {
{ 3,Q( 1.5),3,Q(1.5), 0,Q(1.5), 0,Q(1.5) }, /* 1.5Mb/s */
{ 4,Q(2.25),3,Q(1.5), 4,Q(1.5), 0,Q(1.5) }, /*2.25Mb/s */
{ 4,Q( 3),3,Q(1.5), 4,Q(1.5), 0,Q(1.5) }, /* 3Mb/s */
{ 4,Q( 4.5),3,Q( 3), 4,Q(1.5), 2,Q(1.5) }, /* 4.5Mb/s */
{ 4,Q( 6),3,Q(4.5), 4,Q( 3), 2,Q(1.5) }, /* 6Mb/s */
{ 4,Q( 9),3,Q( 6), 4,Q(4.5), 2,Q(1.5) }, /* 9Mb/s */
{ 4,Q( 12),3,Q( 9), 4,Q( 6), 2,Q( 3) }, /* 12Mb/s */
{ 4,Q(13.5),3,Q( 12), 4,Q( 9), 2,Q( 6) } /*13.5Mb/s */
};
#undef QМакрос Q(1.5) раскрывается в:
(((1.5) == 1.5) ? 0 : (((1.5) ==2.25) ? 1 : (((1.5) == 3) ? 2 : \
(((1.5) == 4.5) ? 3 : (((1.5) == 6) ? 4 : (((1.5) == 9) ? 5 : \
(((1.5) == 12) ? 6 : (((1.5) == 13.5)? 7 : 0))))))))Некоторые из сравнений анализатор PVS-Studio считает подозрительными. Например, на выражение (((1.5) == 3) он выдаёт предупреждение:
V674 The '1.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the '(1.5) == 3' expression. tx_schedules.h 228
Всего на этот массив анализатор выдаёт 96 сообщений.
В коде FreeBSD можно встретить ещё несколько таких массивов. Суммарно анализатор выдаёт на них 692 предупреждения уровня High. Напомню, что всего предупреждений уровня High насчитывается 3577. Это значит, что такие макросы приводят к возникновению 1/5 этих предупреждений.
Другими словами, чуть-чуть настроив анализатор, можно устранить 20% ложных сообщений уровня High. Сделать это можно по-разному, но, пожалуй, проще всего будет выключить предупреждение V674 для тех файлов, в которых используются подобные массивы. Для этого нужно написать где-то в файле комментарий //-V::674.
Я уже озвучивал важную мысль, но повторю её, так как нас вновь и вновь спрашивают о среднем проценте ложных срабатываний. Даже если мы посчитаем этот средний процент на основании проверки многих проектов, он не будет иметь никакой практической ценности. Это тоже самое, как интересоваться средней температурой по больнице.
Все очень сильно зависит от проекта. Кому-то повезёт и можно работать со списком предупреждений, практически ничего не настраивая. Другим не повезёт, как в случае с проектом FreeBSD. Придётся заниматься настройкой и разметкой макросов. Но это не так страшно, как может показаться на первый взгляд. Выше я как раз показал, каким образом сразу можно устранить много ложных предупреждений. Аналогичная ситуация будет и с другими предупреждениями, возникающими из-за дурацких макросов.
Да и вообще, если бы с шумом было сложно бороться, я бы не смог найти все эти ошибки для статьи за один вечер.
Мы решили смотреть на мир более широко. Там, где раньше мы видели ошибки и код с запахом, теперь мы стараемся видеть ещё и потенциальные уязвимости. Для этого мы решили начать с того, что будем классифицировать предупреждения, выдаваемые с помощью PVS-Studio согласно Common Weakness Enumeration (CWE). Подробнее про это можно почитать здесь: PVS-Studio: поиск дефектов безопасности.
Конечно, только малую часть ошибок можно эксплуатировать. Другими словами, только немногие из найденных CWE ошибок могут превратиться в CVE. Однако, чем больше ошибок, попадающих под классификацию CWE, будет найдено с помощью статического анализа, тем лучше.
Используйте PVS-Studio для профилактики уязвимостей. Эта статья продемонстрирует, что анализатор хорошо справляется с этой задачей.
Всего я заметил 22 ошибки этого типа. И, наверное, ещё столько же пропустил.
Давайте начнем с простого случая.
void
ql_mbx_isr(void *arg)
{
....
ha = arg;
if (ha == NULL) {
device_printf(ha->pci_dev, "%s: arg == NULL\n", __func__);
return;
}
....
}Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'ha' might take place. ql_isr.c 750
Здесь ошибка видна сразу. Если указатель ha равен NULL, то он разыменовывается в выражении ha->pci_dev.
Такая же ситуация обнаруживается ещё в трёх файлах:
Теперь рассмотрим более сложную ситуацию:
static int ecore_ilt_client_mem_op(struct bxe_softc *sc,
int cli_num, uint8_t memop)
{
int i, rc;
struct ecore_ilt *ilt = SC_ILT(sc);
struct ilt_client_info *ilt_cli = &ilt->clients[cli_num];
if (!ilt || !ilt->lines)
return -1;
....
}Предупреждение PVS-Studio: V595 The 'ilt' pointer was utilized before it was verified against nullptr. Check lines: 667, 669. ecore_init_ops.h 667
На этом случае остановимся поподробнее, так как далеко не все понимают всю опасность, которая заключена в этом коде.
В начале указатель ilt разыменовывается:
struct ilt_client_info *ilt_cli = &ilt->clients[cli_num];Далее он проверяется на равенство NULL:
if (!ilt || !ilt->lines)Следовательно, возможно разыменование нулевого указателя. Это неизбежно приводит к неопределённому поведению программы.
Здесь некоторые возразят, что никакой беды нет, так как указатель разыменовывается "не по-настоящему". Они скажут, что просто вычисляется адрес ячейки массива. Да, скажут они, этот адрес некорректен и его использовать нельзя. Однако, ниже есть проверка, и произойдёт выход из функции, если указатель ilt будет равен нулю. Следовательно, невалидный указатель ilt_cli не будет нигде использоваться и никакой ошибки в программе нет.
Они не правы. Нельзя так размышлять. Разыменование нулевого указателя приводит к неопределенному поведению. Следовательно, код некорректен и не следует рассуждать, как он будет работать. Он может работать как угодно.
Однако, такое объяснение часто не устраивает некоторых читателей, поэтому я попробую развить мысль. Компилятор знает, что разыменование нулевого указателя — это неопределённое поведение. Следовательно, если указатель разыменовывают, то он не равен NULL. Раз он не равен NULL, компилятор имеет полное право удалить избыточную проверку if (!ilt). В результате, если указатель будет равен NULL, то из функции не произойдёт выход. Поэтому функция начнет обрабатывать невалидные указатели, что может привести к чему угодно.
Некоторые возражают, говоря, что макрос offsetof иногда реализуют следующим образом:
#define offsetof(st, m) ((size_t)(&((st *)0)->m))Здесь имеет место разыменование нулевого указателя, но код успешно работает. Следовательно, это доказывает, что подобные конструкции вполне допустимы.
И вновь они не правы. Ничего это не доказывает.
При рассмотрении идиоматической реализации offsetof следует учитывать, что компилятору разрешено использовать непереносимые приемы для реализации этой функциональности. Тот факт, что в реализации библиотеки в компиляторе используется константа нулевого указателя при реализации offsetof, вовсе не означает, что в пользовательском коде можно без опаски выполнять &ilt->clients[cli_num] в случае, когда ilt является нулевым указателем.
Более подробно с этой темой можно познакомиться в моей статье "Разыменовывание нулевого указателя приводит к неопределённому поведению".
В итоге, рассмотренный выше код представляет собой самую настоящую ошибку и должен быть исправлен.
Теперь, когда мы разобрались с нюансами разыменования нулевого указателя, становится понятно, что неправильной является и следующая функция:
static struct iscsi_outstanding *
iscsi_outstanding_add(struct iscsi_session *is,
struct icl_pdu *request,
union ccb *ccb,
uint32_t *initiator_task_tagp)
{
struct iscsi_outstanding *io;
int error;
ISCSI_SESSION_LOCK_ASSERT(is);
io = uma_zalloc(iscsi_outstanding_zone, M_NOWAIT | M_ZERO);
if (io == NULL) {
ISCSI_SESSION_WARN(is, "failed to allocate %zd bytes",
sizeof(*io));
return (NULL);
}
error = icl_conn_task_setup(is->is_conn, request, &ccb->csio,
initiator_task_tagp, &io->io_icl_prv);
....
}Предупреждение анализатора PVS-Studio: V522 Dereferencing of the null pointer 'ccb' might take place. The null pointer is passed into 'iscsi_outstanding_add' function. Inspect the third argument. Check lines: 'iscsi.c:2157'. iscsi.c 2091
В начале может быть не понятно, почему анализатор решил, что указатель ccb будет нулевым. Поэтому обратим внимание, что анализатор указывает в предупреждении ещё и вот сюда: iscsi.c:2157.
Здесь находится вызов функции iscsi_outstanding_add, в которую в качестве фактического аргумента передаётся NULL:
static void
iscsi_action_abort(struct iscsi_session *is, union ccb *ccb)
{
....
io = iscsi_outstanding_add(is, request, NULL,
&initiator_task_tag);
....
}Здесь анализатору понадобилось выполнить межпроцедурный анализ, чтобы найти дефект.
Немного отдохнем от сложных ошибок и рассмотрим совсем простой случай неправильного обработчика ошибок.
int radeon_cs_ioctl(struct drm_device *dev, void *data,
struct drm_file *fpriv)
{
....
struct drm_radeon_private *dev_priv = dev->dev_private;
....
if (dev_priv == NULL) {
DRM_ERROR("called with no initialization\n");
mtx_unlock(&dev_priv->cs.cs_mutex);
return -EINVAL;
}
....
}Предупреждение анализатора PVS-Studio: V522 Dereferencing of the null pointer 'dev_priv' might take place. radeon_cs.c 153
Тело оператора if выполняется только когда указатель dev_priv равен нулю. Таким образом, здесь вычисляется непонятно какой адрес: &dev_priv->cs.cs_mutex. Да и вообще это UB.
Ох. Что-то проблемы с нулевыми указателями никак не заканчиваются. Но что делать, раз это головная боль многих языков программирования. Так что заварите кофе и продолжайте чтение.
static void
bwn_txpwr(void *arg, int npending)
{
struct bwn_mac *mac = arg;
struct bwn_softc *sc = mac->mac_sc;
BWN_LOCK(sc);
if (mac && mac->mac_status >= BWN_MAC_STATUS_STARTED &&
mac->mac_phy.set_txpwr != NULL)
mac->mac_phy.set_txpwr(mac);
BWN_UNLOCK(sc);
}Предупреждение анализатора PVS-Studio: V595 The 'mac' pointer was utilized before it was verified against nullptr. Check lines: 6757, 6760. if_bwn.c 6757
Указатель mac в начале разыменовывается, а далее проверяется на равенство NULL. Здесь всё просто, так что подробнее комментировать не буду.
struct opcode_obj_rewrite *ctl3_rewriters;
void
ipfw_add_obj_rewriter(struct opcode_obj_rewrite *rw,
size_t count)
{
....
memcpy(tmp, ctl3_rewriters, ctl3_rsize * sizeof(*rw)); // <=
memcpy(&tmp[ctl3_rsize], rw, count * sizeof(*rw));
qsort(tmp, sz, sizeof(*rw), compare_opcodes);
/* Switch new and free old */
if (ctl3_rewriters != NULL) // <=
free(ctl3_rewriters, M_IPFW);
ctl3_rewriters = tmp;
ctl3_rsize = sz;
CTL3_UNLOCK();
}Предупреждение анализатора PVS-Studio: V595 The 'ctl3_rewriters' pointer was utilized before it was verified against nullptr. Check lines: 3206, 3210. ip_fw_sockopt.c 3206
Обратите внимание, что вначале указатель ctl3_rewriters используется как фактический аргумент функции memcpy:
memcpy(tmp, ctl3_rewriters, ctl3_rsize * sizeof(*rw));А затем вдруг вспоминают, что его следует проверять на равенство NULL:
if (ctl3_rewriters != NULL)Рассмотрим ещё один случай неправильного кода, предназначенного для освобождения ресурсов:
static int
mly_user_command(struct mly_softc *sc, struct mly_user_command *uc)
{
struct mly_command *mc;
....
if (mc->mc_data != NULL) // <=
free(mc->mc_data, M_DEVBUF); // <=
if (mc != NULL) { // <=
MLY_LOCK(sc);
mly_release_command(mc);
MLY_UNLOCK(sc);
}
return(error);
}Предупреждение анализатора PVS-Studio: V595 The 'mc' pointer was utilized before it was verified against nullptr. Check lines: 2954, 2955. mly.c 2954
Стоит закругляться с нулевыми указателями, так как, думаю, описание таких ошибок уже наскучило не только мне, но и читателю. Я вижу CWE-476 (NULL Pointer Dereference) ещё в следующих участках кода:
Но и это ещё не всё! Мне просто наскучило изучать эту разновидность ошибок, и я перешел к изучению предупреждений другого типа. Анализатор PVS-Studio ждёт героев, которые изучат все предупреждения, которые касаются нулевых указателей.
Рассмотрим определение структуры pfloghdr:
struct pfloghdr {
u_int8_t length;
sa_family_t af;
u_int8_t action;
u_int8_t reason;
char ifname[IFNAMSIZ];
char ruleset[PFLOG_RULESET_NAME_SIZE];
u_int32_t rulenr;
u_int32_t subrulenr;
uid_t uid;
pid_t pid;
uid_t rule_uid;
pid_t rule_pid;
u_int8_t dir;
u_int8_t pad[3];
};Как видите, такая структура достаточно большая. Для инициализации подобных структур распространена практика, когда вся структура заполняется нулями, а потом устанавливаются значения для отдельных полей.
Однако, в функции nat64lsn_log не удалось корректно произвести инициализацию структуры. Рассмотрим код этой функции:
static void
nat64lsn_log(struct pfloghdr *plog, ....)
{
memset(plog, 0, sizeof(plog)); // <=
plog->length = PFLOG_REAL_HDRLEN;
plog->af = family;
plog->action = PF_NAT;
plog->dir = PF_IN;
plog->rulenr = htonl(n);
plog->subrulenr = htonl(sn);
plog->ruleset[0] = '\0';
strlcpy(plog->ifname, "NAT64LSN", sizeof(plog->ifname));
ipfw_bpf_mtap2(plog, PFLOG_HDRLEN, m);
}Предупреждение анализатора PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'plog'. nat64lsn.c 218
Обратите внимание, что sizeof(plog) вычисляет не размер структуры, а размер указателя. В результате обнуляется не вся структура, а только несколько первых байт, все остальные поля структуры остаются неинициализированными. Конечно, в некоторые поля затем явно записываются правильные значения. Однако, ряд членов в структуре останется неинициализированными.
Точно такую же ошибку можно наблюдать в файле nat64stl.c: V512 A call of the 'memset' function will lead to underflow of the buffer 'plog'. nat64stl.c 72
Рассмотрим ещё одну ошибку, из-за которой переменная может быть не инициализирована.
osGLOBAL bit32
tdsaSendTMFIoctl(
tiRoot_t *tiRoot,
tiIOCTLPayload_t *agIOCTLPayload,
void *agParam1,
void *agParam2,
unsigned long resetType
)
{
bit32 status;
tmf_pass_through_req_t *tmf_req = ....;
#if !(defined(__FreeBSD__))
status = ostiSendResetDeviceIoctl(tiRoot, agParam2,
tmf_req->pathId, tmf_req->targetId, tmf_req->lun, resetType);
#endif
TI_DBG3((
"Status returned from ostiSendResetDeviceIoctl is %d\n",
status));
if(status != IOCTL_CALL_SUCCESS)
{
agIOCTLPayload->Status = status;
return status;
}
status = IOCTL_CALL_SUCCESS;
return status;
}Предупреждение анализатора PVS-Studio: V614 Uninitialized variable 'status' used. tdioctl.c 3396
Если объявлен макрос __FreeBSD__ (а он объявлен), то условие
#if !(defined(__FreeBSD__))не выполняется. Как результат, код внутри конструкции #if...#endif не компилируется, и переменная status остаётся неинициализированной.
typedef struct qls_mpid_glbl_hdr
{
uint32_t cookie;
uint8_t id[16];
uint32_t time_lo;
....
} qls_mpid_glbl_hdr_t;
struct qls_mpi_coredump {
qls_mpid_glbl_hdr_t mpi_global_header;
....
};
typedef struct qls_mpi_coredump qls_mpi_coredump_t;
int
qls_mpi_core_dump(qla_host_t *ha)
{
....
qls_mpi_coredump_t *mpi_dump = &ql_mpi_coredump;
....
memcpy(mpi_dump->mpi_global_header.id, "MPI Coredump",
sizeof(mpi_dump->mpi_global_header.id));
....
}Предупреждение анализатора PVS-Studio: V512 A call of the 'memcpy' function will lead to the '"MPI Coredump"' buffer becoming out of range. qls_dump.c 1615
Чтобы показать, как объявлены интересующие нас типы и члены структуры, пришлось привести достаточно много кода. Но не спешите зевать, сейчас я выделю самое существенное:
uint8_t id[16];
memcpy(id, "MPI Coredump", sizeof(id));Для нас важно:
Будет скопировано 13 байт строки и ещё 3 байта, расположенные после строки. Такой код на практике может даже работать. Просто скопируется 3 байта с мусором или фрагмент другой строки. Однако, формально, это выход за границу массива, а, следовательно, этот код приводит к неопределённому поведению программы.
Вот и представился случай продемонстрировать вам одну из новых диагностик, реализованных в PVS-Studio. Суть диагностики V781:
В начале значение переменной используется в качестве размера или индекса массива. А уже затем это значение сравнивается с 0 или с размером массива. Это может указывать на наличие логической ошибки в коде или опечатку в одном из сравнений.
По своей сути эта диагностика схожа с V595, которая хорошо знакома моим постоянным читателям.
Посмотрим, где сработала эта диагностика при проверке кода FreeBSD.
static void
sbp_targ_mgm_handler(struct fw_xfer *xfer)
{
....
int exclusive = 0, lun;
....
lun = orb4->id;
lstate = orbi->sc->lstate[lun];
if (lun >= MAX_LUN || lstate == NULL ||
(exclusive &&
STAILQ_FIRST(&lstate->logins) != NULL &&
STAILQ_FIRST(&lstate->logins)->fwdev != orbi->fwdev)
) {
/* error */
orbi->status.dead = 1;
orbi->status.status = STATUS_ACCESS_DENY;
orbi->status.len = 1;
break;
}
....
}Предупреждение анализатора PVS-Studio: V781 The value of the 'lun' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 1617, 1619. sbp_targ.c 1617
В начале программист смело использовал индекс lun для доступа к массиву lstate. И только затем осуществляется проверка: не превышает ли значение индекса предельно допустимое значение, равное MAX_LUN? Если превышает, то эта ситуация обрабатывается как ошибочная. Но уже поздно, так как мы уже могли обратиться далеко за пределы массива.
Формально, это приведёт к неопределённому поведению программы. На практике, вместо корректной обработки неправильного значения индекса может возникнуть Access Violation.
Теперь рассмотрим более интересный случай неправильной индексации массива.
#define R88E_GROUP_2G 6
#define RTWN_RIDX_OFDM6 4
#define RTWN_RIDX_COUNT 28
struct rtwn_r88e_txagc {
uint8_t pwr[R88E_GROUP_2G][20]; /* RTWN_RIDX_MCS(7) + 1 */
};
void
r88e_get_txpower(struct rtwn_softc *sc, int chain,
struct ieee80211_channel *c, uint16_t power[RTWN_RIDX_COUNT])
{
const struct rtwn_r88e_txagc *base = rs->rs_txagc;
....
for (ridx = RTWN_RIDX_OFDM6; ridx < RTWN_RIDX_COUNT; ridx++) {
if (rs->regulatory == 3)
power[ridx] = base->pwr[0][ridx];
else if (rs->regulatory == 1) {
if (!IEEE80211_IS_CHAN_HT40(c))
power[ridx] = base->pwr[group][ridx];
} else if (rs->regulatory != 2)
power[ridx] = base->pwr[0][ridx];
}
....
}Анализатор выдал здесь три предупреждения на три выражения, где осуществляется доступ к массиву pwr:
В цикле значение индекса ridx изменяется от значения RTWN_RIDX_OFDM6 до RTWN_RIDX_COUNT. То есть переменная ridx принимает значения в диапазоне [4..27]. На первый взгляд, всё хорошо.
Чтобы найти ошибку, обратим внимание на член pwr, представляющий собой двухмерный массив:
uint8_t pwr[R88E_GROUP_2G][20]; // R88E_GROUP_2G == 6И давайте ещё раз посмотрим, как происходит доступ к этому массиву в цикле:
base->pwr[0][ridx] // ridx=[4..27]
base->pwr[group][ridx] // ridx=[4..27]
base->pwr[0][ridx] // ridx=[4..27]Здесь явно что-то не так. Происходит выход за границу массива. Однако, я затрудняюсь предположить, как должен работать этот код и что здесь следует изменить.
static int
smbfs_getattr(ap)
struct vop_getattr_args *ap;
{
....
if (np->n_flag & NOPEN)
np->n_size = oldsize;
smbfs_free_scred(scred);
return 0;
}Предупреждение анализатора PVS-Studio: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. smbfs_vnops.c 283
Оформление кода не соответствует логике его работы. Визуально кажется, что строчка smbfs_free_scred(scred); выполняется только, если условие истинно. Но на самом деле, эта сточка выполняется всегда.
Возможно, настоящей ошибки здесь нет, и достаточно отформатировать код, но, в любом случае, этот фрагмент кода заслуживает самого пристального внимания.
Анализатор выявил ещё 4 четыре аналогичных подозрительных фрагмента кода, но не буду их здесь приводить, так как они все однотипны. Ограничусь предупреждениями:
int
ipf_p_ftp_port(softf, fin, ip, nat, ftp, dlen)
ipf_ftp_softc_t *softf;
fr_info_t *fin;
ip_t *ip;
nat_t *nat;
ftpinfo_t *ftp;
int dlen;
{
....
if (nat->nat_dir == NAT_INBOUND)
a1 = ntohl(nat->nat_ndstaddr); // <=
else
a1 = ntohl(ip->ip_src.s_addr); // <=
a1 = ntohl(ip->ip_src.s_addr); // <=
a2 = (a1 >> 16) & 0xff;
a3 = (a1 >> 8) & 0xff;
a4 = a1 & 0xff;
....
}Предупреждение анализатора PVS-Studio: V519 The 'a1' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 397, 400. ip_ftp_pxy.c 400
Независимо от условия, переменной a1 будет присвоено значение ntohl(ip->ip_src.s_addr).
Мне кажется, что последнее присваивание является лишним. Возможно, такой код получился в результате неудачного рефакторинга.
Продолжим рассмотрение ошибок этой разновидности:
static inline int ecore_func_send_switch_update(
struct bxe_softc *sc,
struct ecore_func_state_params *params)
{
....
if (ECORE_TEST_BIT(ECORE_F_UPDATE_VLAN_FORCE_PRIO_FLAG,
&switch_update_params->changes))
rdata->sd_vlan_force_pri_flg = 1;
rdata->sd_vlan_force_pri_flg =
switch_update_params->vlan_force_prio;
....
}Предупреждение анализатора PVS-Studio: V519 The 'rdata->sd_vlan_force_pri_flg' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 6327, 6328. ecore_sp.c 6328
Ситуация аналогичная, так что не будем на ней останавливаться. Идём дальше.
static int
ixgbe_add_vf(device_t dev, u16 vfnum, const nvlist_t *config)
{
....
if (nvlist_exists_binary(config, "mac-addr")) {
mac = nvlist_get_binary(config, "mac-addr", NULL);
bcopy(mac, vf->ether_addr, ETHER_ADDR_LEN);
if (nvlist_get_bool(config, "allow-set-mac"))
vf->flags |= IXGBE_VF_CAP_MAC;
} else
/*
* If the administrator has not specified a MAC address then
* we must allow the VF to choose one.
*/
vf->flags |= IXGBE_VF_CAP_MAC;
vf->flags = IXGBE_VF_ACTIVE;
....
}Предупреждение анализатора PVS-Studio: V519 The 'vf->flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 5992, 5994. if_ix.c 5994
Скорее всего пропустили "|" и корректный код должен был быть таким:
vf->flags |= IXGBE_VF_ACTIVE;Вообще, все эти ошибки, которые я рассматриваю, пугают. Точнее, их количество. Ведь я вижу, сколько ошибок выписал, и что конец статьи не близок.
typedef struct {
uint64_t __mask;
} l_sigset_t;
int
linux_sigreturn(struct thread *td,
struct linux_sigreturn_args *args)
{
l_sigset_t lmask;
....
lmask.__mask = frame.sf_sc.sc_mask;
lmask.__mask = frame.sf_extramask[0];
....
}Предупреждение анализатора PVS-Studio: V519 The 'lmask.__mask' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 594, 595. linux32_sysvec.c 595
static u_int sysctl_log_level = 0;
....
int sysctl_chg_loglevel(SYSCTL_HANDLER_ARGS)
{
u_int level = *(u_int *)arg1;
int error;
error = sysctl_handle_int(oidp, &level, 0, req);
if (error) return (error);
sysctl_log_level =
(level > SN_LOG_DEBUG_MAX)?(SN_LOG_DEBUG_MAX):(level);
sysctl_log_level =
(level < SN_LOG_LOW)?(SN_LOG_LOW):(level);
return (0);
}Предупреждение анализатора PVS-Studio: V519 The 'sysctl_log_level' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 423, 424. alias_sctp.c 424
Видимо код писался методом Copy-Paste, и, как всегда, имя самой последней переменной забыли поменять. Следовало написать:
sysctl_log_level =
(level < SN_LOG_LOW)?(SN_LOG_LOW):(sysctl_log_level);См. философско-исследовательскую статью на эту тему: "Объяснение эффекта последней строки".
Продолжим изучать, как глубока кроличья нора.
static int
uath_tx_start(struct uath_softc *sc, struct mbuf *m0,
struct ieee80211_node *ni, struct uath_data *data)
{
....
chunk->flags = (m0->m_flags & M_FRAG) ? 0 : UATH_CFLAGS_FINAL;
if (m0->m_flags & M_LASTFRAG)
chunk->flags |= UATH_CFLAGS_FINAL;
chunk->flags = UATH_CFLAGS_FINAL;
....
}Предупреждение анализатора PVS-Studio: V519 The 'chunk->flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1566, 1567. if_uath.c 1567
Пора вставить какую-то картинку для расслабления и отдыха. Думаю, эта как раз подойдёт.
static void ch7017_mode_set(....)
{
uint8_t lvds_pll_feedback_div, lvds_pll_vco_control;
....
lvds_pll_feedback_div =
CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
(2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) |
(3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT);
lvds_pll_feedback_div = 35;
....
}Предупреждение анализатора PVS-Studio: V519 The 'lvds_pll_feedback_div' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 287, 290. dvo_ch7017.c 290
Перетирать значение переменной магическим числом 35 ну очень странно и подозрительно. Похоже, что кто-то написал эту строчку в отладочных целях, а потом забыл её удалить.
static void
bhnd_pmu1_pllinit0(struct bhnd_pmu_softc *sc, uint32_t xtal)
{
uint32_t pmuctrl;
....
/* Write XtalFreq. Set the divisor also. */
pmuctrl = BHND_PMU_READ_4(sc, BHND_PMU_CTRL);
pmuctrl = ~(BHND_PMU_CTRL_ILP_DIV_MASK |
BHND_PMU_CTRL_XTALFREQ_MASK);
pmuctrl |= BHND_PMU_SET_BITS(((xt->fref + 127) / 128) - 1,
BHND_PMU_CTRL_ILP_DIV);
pmuctrl |= BHND_PMU_SET_BITS(xt->xf, BHND_PMU_CTRL_XTALFREQ);
....
}Предупреждение анализатора PVS-Studio: V519 The 'pmuctrl' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2025, 2026. bhnd_pmu_subr.c 2026
Вместо оператора |= случайно написали =.
И последний, на сегодня, дефект этого типа:
void e1000_update_mc_addr_list_vf(struct e1000_hw *hw,
u8 *mc_addr_list, u32 mc_addr_count)
{
....
if (mc_addr_count > 30) {
msgbuf[0] |= E1000_VF_SET_MULTICAST_OVERFLOW;
mc_addr_count = 30;
}
msgbuf[0] = E1000_VF_SET_MULTICAST;
msgbuf[0] |= mc_addr_count << E1000_VT_MSGINFO_SHIFT;
....
}Предупреждение анализатора PVS-Studio: V519 The 'msgbuf[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 422, 426. e1000_vf.c 426
....
U16 max_ncq_depth;
....
SCI_STATUS scif_user_parameters_set(
SCI_CONTROLLER_HANDLE_T controller,
SCIF_USER_PARAMETERS_T * scif_parms
)
{
....
if (scif_parms->sas.max_ncq_depth < 1 &&
scif_parms->sas.max_ncq_depth > 32)
return SCI_FAILURE_INVALID_PARAMETER_VALUE;
....
}Предупреждение анализатора PVS-Studio: V547 Expression is always false. scif_sas_controller.c 531
Переменная не может быть одновременно меньше 1 и больше 32. Чтобы корректно проверить диапазон, нужно заменить оператор && на оператор ||.
Из-за ошибки функция не проверяет входные значения и может работать с некорректными данными.
Теперь более интересный случай. Для начала рассмотрим прототип функции LibAliasSetMode:
unsigned int LibAliasSetMode(.....);Функция возвращает значение беззнакового типа. Функция, в случае возникновения внутренней ошибки, возвращается значение -1. Соответственно -1 превращается в UINT_MAX.
Теперь посмотрим, как используется эта функция.
static int
ng_nat_rcvmsg(node_p node, item_p item, hook_p lasthook)
{
....
if (LibAliasSetMode(priv->lib,
ng_nat_translate_flags(mode->flags),
ng_nat_translate_flags(mode->mask)) < 0) {
error = ENOMEM;
break;
}
....
}Предупреждение анализатора PVS-Studio: V547 Expression is always false. Unsigned type value is never < 0. ng_nat.c 374
Естественно, условие всегда ложно. Значение беззнакового типа не может быть меньше нуля.
Для людей, делающих обзор кода, такая ошибка будет плохо заметна. В случае ошибки функция возвращает -1. Проверка вида if (foo() < 0) есть. Создаётся впечатление, что всё хорошо.
Но неправильный тип функции всё портит. Вариантов исправления 2:
Как поступить лучше - решать разработчикам.
В следующем фрагменте, скорее всего, настоящей ошибки нет, и код просто избыточен. Хотя кто знает, я не разработчик и не знаю его замысел.
HAL_BOOL
ar9300_reset_tx_queue(struct ath_hal *ah, u_int q)
{
u_int32_t cw_min, chan_cw_min, value;
....
value = (ahp->ah_beaconInterval * 50 / 100)
- ah->ah_config.ah_additional_swba_backoff
- ah->ah_config.ah_sw_beacon_response_time
+ ah->ah_config.ah_dma_beacon_response_time;
if (value < 10)
value = 10;
if (value < 0)
value = 10;
....
}Предупреждение анализатора PVS-Studio: V547 Expression 'value < 0' is always false. Unsigned type value is never < 0. ar9300_xmit.c 450
Попробуйте самостоятельно заметить ошибку вот здесь:
static void
dtrace_debug_output(void)
{
....
if (d->first < d->next) {
char *p1 = dtrace_debug_bufr;
count = (uintptr_t) d->next - (uintptr_t) d->first;
for (p = d->first; p < d->next; p++)
*p1++ = *p;
} else if (d->next > d->first) {
char *p1 = dtrace_debug_bufr;
count = (uintptr_t) d->last - (uintptr_t) d->first;
for (p = d->first; p < d->last; p++)
*p1++ = *p;
count += (uintptr_t) d->next - (uintptr_t) d->bufr;
for (p = d->bufr; p < d->next; p++)
*p1++ = *p;
}
....
}Предупреждение анализатора PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 102, 109. dtrace_debug.c 102
Следует обратить внимание вот на эти две строчки:
if (d->first < d->next) {
} else if (d->next > d->first) {Хотели написать другое условие, но не получилось. Поэтому второе условие всегда будет ложным.
int
mfi_tbolt_send_frame(struct mfi_softc *sc, struct mfi_command *cm)
{
....
uint8_t *cdb;
....
/* check for inquiry commands coming from CLI */
if (cdb[0] != 0x28 || cdb[0] != 0x2A) {
if ((req_desc = mfi_tbolt_build_mpt_cmd(sc, cm)) == NULL) {
device_printf(sc->mfi_dev, "Mapping from MFI "
"to MPT Failed \n");
return 1;
}
}
....
}Предупреждение анализатора PVS-Studio: V547 Expression 'cdb[0] != 0x28 || cdb[0] != 0x2A' is always true. Probably the '&&' operator should be used here. mfi_tbolt.c 1110
Условие (cdb[0] != 0x28 || cdb[0] != 0x2A) написано неправильно. Если байт равен значению 0x28, то он не может быть равен 0x2A. И наоборот. В результате условие всегда истинно.
Теперь рассмотрим два цикла, реализованных сложным и опасным образом.
static void
safe_mcopy(struct mbuf *srcm, struct mbuf *dstm, u_int offset)
{
u_int j, dlen, slen;
caddr_t dptr, sptr;
/*
* Advance src and dst to offset.
*/
j = offset;
while (j >= 0) {
if (srcm->m_len > j)
break;
j -= srcm->m_len;
srcm = srcm->m_next;
if (srcm == NULL)
return;
}
sptr = mtod(srcm, caddr_t) + j;
slen = srcm->m_len - j;
j = offset;
while (j >= 0) {
if (dstm->m_len > j)
break;
j -= dstm->m_len;
dstm = dstm->m_next;
if (dstm == NULL)
return;
}
....
}Предупреждения анализатора PVS-Studio:
Обратите внимание, что переменная j имеет беззнаковый тип. Следовательно, проверка (j >= 0) не имеет смысла. С тем же успехом можно было написать while (true).
Я не знаю, может ли эта неправильная проверка вызвать сбой или циклы в любом случае будут корректно прерваны, благодаря наличию в их телах операторов break и return. Мне кажется, это настоящий баг и стоит изменить тип переменной j с u_int на int.
Если даже ошибки здесь нет, то код стоит переписать, чтобы он не сбивал с толку других разработчиков и не стал в дальнейшем причиной проблемы, когда кто-то будет изменять этот код.
Лично мне нравится ошибка, описанная ниже. Это красивая опечатка. Хотя нет, стоп, я же решил в этот раз говорить про CWE. Итак, перед нами красивая потенциальная уязвимость:
#define OSSA_MPI_ENC_ERR_ILLEGAL_DEK_PARAM 0x2001
#define OSSA_MPI_ERR_DEK_MANAGEMENT_DEK_UNWRAP_FAIL 0x2002
GLOBAL bit32 mpiDekManagementRsp(
agsaRoot_t *agRoot,
agsaDekManagementRsp_t *pIomb
)
{
....
if (status == OSSA_MPI_ENC_ERR_ILLEGAL_DEK_PARAM ||
OSSA_MPI_ERR_DEK_MANAGEMENT_DEK_UNWRAP_FAIL)
{
agEvent.eq = errorQualifier;
}
....
}Предупреждение анализатора PVS-Studio: V560 A part of conditional expression is always true: 0x2002. sampirsp.c 7224
В условии один раз пропущена переменная status. Таким образом, значение статуса на самом деле не проверяется, и условие всегда истинно.
Рассмотрим ещё один аналогичный случай. Попробуйте, не читая описание, найти ошибку в функции ugidfw_rule_valid самостоятельно.
static int
ugidfw_rule_valid(struct mac_bsdextended_rule *rule)
{
if ((rule->mbr_subject.mbs_flags | MBS_ALL_FLAGS) != MBS_ALL_FLAGS)
return (EINVAL);
if ((rule->mbr_subject.mbs_neg | MBS_ALL_FLAGS) != MBS_ALL_FLAGS)
return (EINVAL);
if ((rule->mbr_object.mbo_flags | MBO_ALL_FLAGS) != MBO_ALL_FLAGS)
return (EINVAL);
if ((rule->mbr_object.mbo_neg | MBO_ALL_FLAGS) != MBO_ALL_FLAGS)
return (EINVAL);
if ((rule->mbr_object.mbo_neg | MBO_TYPE_DEFINED) &&
(rule->mbr_object.mbo_type | MBO_ALL_TYPE) != MBO_ALL_TYPE)
return (EINVAL);
if ((rule->mbr_mode | MBI_ALLPERM) != MBI_ALLPERM)
return (EINVAL);
return (0);
}Тяжело?
Думаю, да. Вот поэтому статические анализаторы так важны. Они не зевают и не устают при просмотре подобных функций.
Предупреждение анализатора PVS-Studio: V617 Consider inspecting the condition. The '0x00000080' argument of the '|' bitwise operation contains a non-zero value. mac_bsdextended.c 128
Для начала, взглянем на макрос MBO_TYPE_DEFINED:
#define MBO_TYPE_DEFINED 0x00000080А теперь, смотрим вот сюда:
(rule->mbr_object.mbo_neg | MBO_TYPE_DEFINED)Часть условия всегда истина. Если посмотреть код, расположенный рядом, то становится понятным, что на самом деле, хотели написать вот так:
(rule->mbr_object.mbo_neg | MBO_TYPE_DEFINED) != MBO_TYPE_DEFINEDЧто-то статья всё не хочет заканчиваться. Придётся немного ускориться и сократить количество фрагментов кода. Так что просто знайте, что вижу ещё четыре CWE-571:
int mlx5_core_create_qp(struct mlx5_core_dev *dev,
struct mlx5_core_qp *qp,
struct mlx5_create_qp_mbox_in *in,
int inlen)
{
....
struct mlx5_destroy_qp_mbox_out dout;
....
err_cmd:
memset(&din, 0, sizeof(din));
memset(&dout, 0, sizeof(dout));
din.hdr.opcode = cpu_to_be16(MLX5_CMD_OP_DESTROY_QP);
din.qpn = cpu_to_be32(qp->qpn);
mlx5_cmd_exec(dev, &din, sizeof(din), &out, sizeof(dout));
return err;
}Предупреждение анализатора PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'dout' object. The memset_s() function should be used to erase the private data. mlx5_qp.c 159
Хотели обнулить структуру dout, содержащую приватные данные. Ошибка в том, что эта структура в дальнейшем не используется. Точнее, она используется вот здесь sizeof(dout), но это не считается. Поэтому компилятор удалит вызов функции memset.
Ещё одно неудачное обнуление структуры можно найти здесь: V597 The compiler could delete the 'memset' function call, which is used to flush 'dout' object. The memset_s() function should be used to erase the private data. mlx5_qp.c 323
Каждый раз, когда я описываю ошибку обнуления приватных данных, находится кто-то, кто пишет мне одно из двух сообщений:
Поэтому сразу поясню. Современные компиляторы действительно удаляют вызовы функции memset для оптимизации. И это не ошибка компилятора. Подробно эта тема рассматривается в описании диагностики V597.
static int
wi_pci_resume(device_t dev)
{
struct wi_softc *sc = device_get_softc(dev);
struct ieee80211com *ic = &sc->sc_ic;
WI_LOCK(sc);
if (sc->wi_bus_type != WI_BUS_PCI_NATIVE) {
return (0); // <=
WI_UNLOCK(sc); // <=
}
if (ic->ic_nrunning > 0)
wi_init(sc);
WI_UNLOCK(sc);
return (0);
}Предупреждение анализатора PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. if_wi_pci.c 258
В начале в тексте программы расположен оператор return, а затем происходит попытка разлочить некий ресурс.
Я нашел ещё десяток занятных багов в коде. Я не знаю, как их классифицировать согласно CWE, поэтому не буду считать их за потенциальные уязвимости, но все равно опишу. Ведь независимо от того, могу я их классифицировать или нет, ошибки всё равно остаются ошибками.
static void
mac_proc_vm_revoke_recurse(struct thread *td, struct ucred *cred,
struct vm_map *map)
{
....
if (!mac_mmap_revocation_via_cow) {
vme->max_protection &= ~VM_PROT_WRITE;
vme->protection &= ~VM_PROT_WRITE;
} if ((revokeperms & VM_PROT_READ) == 0)
vme->eflags |= MAP_ENTRY_COW | MAP_ENTRY_NEEDS_COPY;
....
}Предупреждение анализатора PVS-Studio: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. mac_process.c 352
Мне, как и анализатору, кажется, что здесь забыли ключевое слово else.
Аналогично:
Теперь красивый случай неудачного Copy-Paste. Видите ошибку?
static int
cyapa_raw_input(struct cyapa_softc *sc,
struct cyapa_regs *regs, int freq)
{
....
if (sc->delta_x > sc->cap_resx)
sc->delta_x = sc->cap_resx;
if (sc->delta_x < -sc->cap_resx)
sc->delta_x = -sc->cap_resx;
if (sc->delta_y > sc->cap_resx)
sc->delta_y = sc->cap_resy;
if (sc->delta_y < -sc->cap_resy)
sc->delta_y = -sc->cap_resy;
....
}Предупреждение анализатора PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'cap_resy' variable should be used instead of 'cap_resx'. cyapa.c 1458
Вот она:
if (sc->delta_y > sc->cap_resx)Здесь забыли поменять cap_resx на cap_resy.
static int
linux_msqid_pushdown(l_int ver, struct l_msqid64_ds *linux_msqid64,
caddr_t uaddr)
{
....
if (linux_msqid64->msg_qnum > USHRT_MAX)
linux_msqid.msg_qnum = linux_msqid64->msg_qnum;
else
linux_msqid.msg_qnum = linux_msqid64->msg_qnum;
....
}Предупреждение анализатора PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. linux_ipc.c 353
Аналогично:
Напоследок, подозрительные фактические аргументы при вызове функции strncmp:
int
ipf_p_irc_complete(ircp, buf, len)
ircinfo_t *ircp;
char *buf;
size_t len;
{
....
if (strncmp(s, "PRIVMSG ", 8))
return 0;
....
if (strncmp(s, "\001DCC ", 4)) // <=
return 0;
....
}Предупреждение анализатора PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. ip_irc_pxy.c 140
Обратите внимание, что в начале проверяется, начинается ли строка с символов "PRIVMSG ". При этом учитывается пробел.
Далее идёт проверка, что строка начинается с "\001DCC ". Но в этой строке, если считать пробел, не 4, а 5 символов. Примечание: \001 - это один символ.
Код FreeBSD регулярно проверяется с помощью Coverity (который теперь стал частью Synopsys). Это не помешало мне сесть вечером на стул, запустить PVS-Studio и найти за вечер 56 потенциальных уязвимостей и ещё 10 багов. Причем я совершенно не ставил цель найти как можно больше ошибок. О чем это говорит? О том, что PVS-Studio является серьёзным конкурентом Coverity в диагностических возможностях. Да ещё PVS-Studio и стоит при этом дешевле.
Традиционно повторю, что статический анализатор должен использоваться не от случая к случаю, а регулярно. Единичная проверка, подобная той, что описана в статье, может служить хорошей рекламой анализатора, но не принесёт проверенному проекту существенной пользы. Вся суть статического анализа, что многие ошибки можно исправить на самом раннем этапе. Дополнительно, так легче поддерживать вывод анализатора чистым и не искать ошибки среди сотен ложных срабатываний. Здесь полная аналогия с предупреждениями компилятора.
Поэтому хватит читать статьи, пора начинать использовать PVS-Studio на практике. Поэтому предлагаю, не откладывая скачать PVS-Studio и попробовать проверить свои проекты. По вопросам лицензирования, пишите на почту support[@]viva64.com или воспользуйтесь формой обратной связи.