Около года назад мы проверяли ядро Linux. В то время это была одна из самых обсуждаемых статей. Мы также получили довольно много запросов на проверку FreeBSD, поэтому, наконец, мы решили потратить время на это.
О проекте
FreeBSD — современная операционная система для серверов, настольных компьютеров и встраиваемых компьютерных платформ. Его код прошел более тридцати лет непрерывного развития, улучшения и оптимизации. Он зарекомендовал себя как система для построения интрасети, интернет-сетей и серверов. Он обеспечивает надежные сетевые службы и эффективное управление памятью.
Несмотря на то, что FreeBSD регулярно проверяется Coverity, мы отлично провели время, проверяя этот проект, потому что было обнаружено много подозрительных фрагментов. В этой статье мы приведем около 40 фрагментов, но разработчики этого проекта могут ознакомиться с полным списком, который содержит около 1000 предупреждений анализатора высокой серьезности.
По моему скромному мнению, многие из тех предупреждений, которые выдает анализатор, являются реальными ошибками, но мне сложно определить, насколько они критичны, так как я не являюсь разработчиком системы. Думаю, это могло бы стать хорошей почвой для дискуссии с авторами проекта.
Исходный код взят из ветки GitHub — мастер. В репозитории ~23000 файлов и два десятка конфигураций сборок для разных платформ, но я проверял только ядро, которое собрал таким образом:
# make buildkernel KERNCONF=MYKERNEL
Методология
Мы использовали статический анализатор кода PVS-Studio версии 6.01.
Для удобства я поставил PC-BSD и написал небольшую утилиту на C++, которая сохраняет рабочую среду запуска компиляторов при сборке ядра. Полученная информация использовалась для получения препроцессированных файлов и их анализа с помощью PVS-Studio. Этот метод позволил мне быстро проверить проект без изучения незнакомой системы сборки для интеграции анализатора. Кроме того, анализ предварительно обработанных файлов позволяет провести более глубокий анализ кода и найти более сложные и интересные ошибки, например, в макросах. В этой статье будет представлено несколько примеров такого рода.
Таким же образом было проанализировано ядро Linux; этот режим также доступен для пользователей Windows в утилите Standalone, входящей в состав дистрибутива PVS-studio. Обычно PVS-Studio без проблем интегрируется в проекты. Существует несколько способов интеграции анализатора, описанных в документации. Утилиты мониторинга имеют большое преимущество при попытке использовать анализатор, если в проекте используется необычная система построения.
Удивительная удача
Первая возможная ошибка была обнаружена до того, как я запустил анализатор проекта и даже до того, как собрал ядро; сборка была прервана ошибкой компоновки. Обратившись к файлу, указанному в ошибке, я увидел следующее:
Обратите внимание на выделенный фрагмент: для оформления отступов используется символ табуляции; два утверждения перемещаются под условием. Но последний оператор на самом деле не относится к условию и всегда будет выполняться. Возможно, здесь забыли фигурные скобки.
Однажды мы получили комментарий, что мы просто копируем предупреждения анализатора, но это не так. Перед анализом проекта мы должны убедиться, что он правильно скомпилирован; когда отчет готов, предупреждения должны быть отсортированы/проанализированы и прокомментированы. Ту же работу выполняет наша служба поддержки клиентов, отвечая на входящие письма. Бывают и случаи, когда заказчики присылают примеры ложных (по их мнению) срабатываний, которые на деле оказываются багами.
Капи-пост и опечатки
Анализатор PVS-Studio — мощный инструмент статического анализа кода, который находит ошибки разной степени серьезности. Первые диагностики были очень простыми и были созданы для обнаружения наиболее распространенных ошибок, связанных с опечатками и копированием-вставкой программирования. После просмотра анализа я сортирую их по коду ошибки. Итак, в этой статье мы начнем с этого типа диагностических правил.
V501 Слева и справа от оператора › одинаковые подвыражения (uintptr_t)b-›handler. ip_fw_sockopt.c 2893
static int
compare_sh(const void *_a, const void *_b)
{
const struct ipfw_sopt_handler *a, *b;
a = (const struct ipfw_sopt_handler *)_a;
b = (const struct ipfw_sopt_handler *)_b;
....
if ((uintptr_t)a->handler < (uintptr_t)b->handler)
return (-1);
else if ((uintptr_t)b->handler > (uintptr_t)b->handler) // <=
return (1);
return (0);
}
Вот яркий пример плохой практики — давать переменным короткие и малоинформативные имена. Теперь из-за опечатки в букве «б» а часть условия никогда не будет возвращать 1. Таким образом, функция не всегда корректно возвращает нулевой статус.
V501 Слева и справа от оператора ‘!=’ находятся одинаковые подвыражения: m-›m_pkthdr.len != m-›m_pkthdr.len key.c 7208
int
key_parse(struct mbuf *m, struct socket *so)
{
....
if ((m->m_flags & M_PKTHDR) == 0 ||
m->m_pkthdr.len != m->m_pkthdr.len) { // <=
....
goto senderror;
}
....
}
Одно из полей структуры сравнивается с самим собой; поэтому результатом логической операции всегда будет False.
V501 Слева и справа от оператора | одинаковые подвыражения: PIM_NOBUSRESET | PIM_NOBUSRESET sbp_targ.c 1327
typedef enum {
PIM_EXTLUNS = 0x100,
PIM_SCANHILO = 0x80,
PIM_NOREMOVE = 0x40,
PIM_NOINITIATOR = 0x20,
PIM_NOBUSRESET = 0x10, // <=
PIM_NO_6_BYTE = 0x08,
PIM_SEQSCAN = 0x04,
PIM_UNMAPPED = 0x02,
PIM_NOSCAN = 0x01
} pi_miscflag;
static void
sbp_targ_action1(struct cam_sim *sim, union ccb *ccb)
{
....
struct ccb_pathinq *cpi = &ccb->cpi;
cpi->version_num = 1; /* XXX??? */
cpi->hba_inquiry = PI_TAG_ABLE;
cpi->target_sprt = PIT_PROCESSOR
| PIT_DISCONNECT
| PIT_TERM_IO;
cpi->transport = XPORT_SPI;
cpi->hba_misc = PIM_NOBUSRESET | PIM_NOBUSRESET; // <=
....
}
В этом примере мы видим, что в побитовой операции используется одна и та же переменная «PIM_NOBUSRESET», что никак не влияет на результат. Скорее всего здесь предполагалось использовать константу с другим значением, но переменную оставили без изменений.
V523 Оператор тогда эквивалентен оператору еще. святой c 2023
GLOBAL void siSMPRespRcvd(....)
{
....
if (agNULL == frameHandle)
{
/* indirect mode */
/* call back with success */
(*(ossaSMPCompletedCB_t)(pRequest->completionCB))(agRoot,
pRequest->pIORequestContext, OSSA_IO_SUCCESS, payloadSize,
frameHandle);
}
else
{
/* direct mode */
/* call back with success */
(*(ossaSMPCompletedCB_t)(pRequest->completionCB))(agRoot,
pRequest->pIORequestContext, OSSA_IO_SUCCESS, payloadSize,
frameHandle);
}
....
}
Две ветки условий комментируются по-разному: /* непрямой режим */ и /* прямой режим */, но реализованы они одинаково, что очень подозрительно.
V523 Оператор тогда эквивалентен оператору еще. smsat.c 2848
osGLOBAL void
smsatInquiryPage89(....)
{
....
if (oneDeviceData->satDeviceType == SATA_ATA_DEVICE)
{
pInquiry[40] = 0x01; /* LBA Low */
pInquiry[41] = 0x00; /* LBA Mid */
pInquiry[42] = 0x00; /* LBA High */
pInquiry[43] = 0x00; /* Device */
pInquiry[44] = 0x00; /* LBA Low Exp */
pInquiry[45] = 0x00; /* LBA Mid Exp */
pInquiry[46] = 0x00; /* LBA High Exp */
pInquiry[47] = 0x00; /* Reserved */
pInquiry[48] = 0x01; /* Sector Count */
pInquiry[49] = 0x00; /* Sector Count Exp */
}
else
{
pInquiry[40] = 0x01; /* LBA Low */
pInquiry[41] = 0x00; /* LBA Mid */
pInquiry[42] = 0x00; /* LBA High */
pInquiry[43] = 0x00; /* Device */
pInquiry[44] = 0x00; /* LBA Low Exp */
pInquiry[45] = 0x00; /* LBA Mid Exp */
pInquiry[46] = 0x00; /* LBA High Exp */
pInquiry[47] = 0x00; /* Reserved */
pInquiry[48] = 0x01; /* Sector Count */
pInquiry[49] = 0x00; /* Sector Count Exp */
}
....
}
Этот пример еще более подозрительный, чем предыдущий. Был скопирован большой фрагмент кода, но в дальнейшем никаких изменений не производилось.
Выражение V547 всегда истинно. Вероятно, здесь следует использовать оператор &&. qla_hw.c 799
static int
qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....)
{
....
if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) ||
(*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 2) != 10)) { // <=
return -1;
}
....
}
Здесь анализатор обнаружил, что условие «(* (tcp_opt + 2)! = 0x08) | (*(tcp_opt+2)!=10) «всегда верно и это действительно так, если построить таблицу истинности. Но скорее всего «&&» тут не нужен, это просто опечатка в смещении адреса. Возможно, код функции должен быть таким:
static int
qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....)
{
....
if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) ||
(*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 3) != 10)) {
return -1;
}
....
}
V571 Периодическая проверка. Это условие уже было проверено в строке 1946 г. sahw.c 1949 г.
GLOBAL
bit32 siHDAMode_V(....)
{
....
if( saRoot->memoryAllocated.agMemory[i].totalLength > biggest)
{
if(biggest < saRoot->memoryAllocated.agMemory[i].totalLength)
{
save = i;
biggest = saRoot->memoryAllocated.agMemory[i].totalLength;
}
}
....
}
Этот код действительно странный, если его упростить, то мы увидим следующее:
if( A > B )
{
if (B < A)
{
....
}
}
Одно и то же условие проверяется дважды. Скорее всего, здесь должно было быть написано что-то другое.
Аналогичный фрагмент:
- V571 Периодическая проверка. Это условие уже проверено в строке 1940. if_rl.c 1941
Опасные макросы
V523 Оператор тогда эквивалентен оператору еще. agtiapi.c 829
if (osti_strncmp(buffer, "0x", 2) == 0)
{
maxTargets = osti_strtoul (buffer, &pLastUsedChar, 0);
AGTIAPI_PRINTK( ".... maxTargets = osti_strtoul 0 \n" );
}
else
{
maxTargets = osti_strtoul (buffer, &pLastUsedChar, 10);
AGTIAPI_PRINTK( ".... maxTargets = osti_strtoul 10\n" );
}
Сначала я пропустил это предупреждение анализатора, думая, что это ложное срабатывание. А вот предупреждения низкой важности тоже стоит пересматривать после проверки проекта (для улучшения анализатора). Итак, я наткнулся на такой макрос:
#define osti_strtoul(nptr, endptr, base) \
strtoul((char *)nptr, (char **)endptr, 0)
Параметр base вообще не используется, а значение 0 всегда передается в функцию strtoul последним параметром, хотя в макрос передаются значения 0 и 10. В препроцессированных файлах все макросы расширились, а код стал похожим. Этот макрос используется таким образом несколько десятков раз. Весь список таких фрагментов отправлен разработчикам.
V733 Возможно, раскрытие макроса привело к неправильному порядку вычисления. Проверочное выражение: чан — 1 * 20. исп.ц 2301
static void
isp_fibre_init_2400(ispsoftc_t *isp)
....
if (ISP_CAP_VP0(isp))
off += ICB2400_VPINFO_PORT_OFF(chan);
else
off += ICB2400_VPINFO_PORT_OFF(chan - 1); // <=
....
}
На первый взгляд в этом фрагменте кода нет ничего странного. Мы видим, что иногда используется значение chan, иногда меньше на один chan — 1, но давайте посмотрим на определение макроса:
#define ICB2400_VPOPT_WRITE_SIZE 20 #define ICB2400_VPINFO_PORT_OFF(chan) \ (ICB2400_VPINFO_OFF + \ sizeof (isp_icb_2400_vpinfo_t) + \ (chan * ICB2400_VPOPT_WRITE_SIZE)) // <=
При передаче бинарного выражения в макрос логика вычислений кардинально меняется. Выражение «(чан — 1) * 20» превращается в «чан — 1 * 20», т.е. в «чан — 20», а неправильно рассчитанный размер используется дальше в программе.
О приоритетах операций
В этом разделе я расскажу, насколько важно знать приоритеты операций, использовать лишние скобки, если вы не уверены, и иногда проверять себя, строя таблицы истинности логических выражений.
V502 Возможно, оператор ?: работает не так, как ожидалось. Оператор ‘?:’ имеет более низкий приоритет, чем оператор ‘|’. ata-serverworks.c 166
ata_serverworks_chipinit(device_t dev)
{
....
pci_write_config(dev, 0x5a,
(pci_read_config(dev, 0x5a, 1) & ~0x40) |
(ctlr->chip->cfg1 == SWKS_100) ? 0x03 : 0x02, 1);
}
....
}
Приоритет оператора ‘?:’ ниже, чем у побитового ИЛИ ‘|’. В результате в битовых операциях, помимо числовых констант, используется выражение result «(ctlr-›chip›cfg1=SWKS_100)», что резко меняет логику вычислений/вычислений. Возможно, эта ошибка не была замечена до сих пор, потому что результат казался очень близким к истине.
V502 Возможно, оператор ?: работает не так, как ожидалось. Оператор ‘?:’ имеет более низкий приоритет, чем оператор ‘|’. in6.c 1318
void
in6_purgeaddr(struct ifaddr *ifa)
{
....
error = rtinit(&(ia->ia_ifa), RTM_DELETE, ia->ia_flags |
(ia->ia_dstaddr.sin6_family == AF_INET6) ? RTF_HOST : 0);
....
}
В другом файле тоже был фрагмент с похожей ошибкой с тернарным оператором.
V547 Выражение ‘cdb[0] != 0x28 || cdb[0] != 0x2A’ всегда верно. Вероятно, здесь следует использовать оператор &&. mfi_tbolt.c 1110
int
mfi_tbolt_send_frame(struct mfi_softc *sc, struct mfi_command *cm)
{
....
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;
}
}
else
device_printf(sc->mfi_dev, "DJA NA XXX SYSPDIO\n");
....
}
Первое условное выражение всегда истинно, поэтому ветвь else никогда не получает управление. Я приведу таблицу истинности на случай спорных логических выражений в этом и следующих примерах. Пример для этого случая:
V590 Рассмотрите возможность проверки «ошибка == 0 || ошибка != — 1’ выражение. Выражение является избыточным или содержит опечатку. nd6.c 2119
int
nd6_output_ifp(....)
{
....
/* Use the SEND socket */
error = send_sendso_input_hook(m, ifp, SND_OUT,
ip6len);
/* -1 == no app on SEND socket */
if (error == 0 || error != -1) // <=
return (error);
....
}
Проблема с этим фрагментом в том, что условное выражение не зависит от результата «ошибка == 0». Возможно, здесь что-то не так.
Еще три случая:
- V590 Рассмотрите возможность проверки ошибки == 0 || ошибка != 35’ выражение. Выражение является избыточным или содержит опечатку. if_ipw.c 1855
- V590 Рассмотрите возможность проверки ошибки == 0 || ошибка != 27’ выражение. Выражение является избыточным или содержит опечатку. if_vmx.c 2747
- V547 Выражение всегда верно. Вероятно, здесь следует использовать оператор «&&». igmp.c 1939 г.
V590 Попробуйте проверить это выражение. Выражение является избыточным или содержит опечатку. sig_verify.c 94
enum uni_ieact {
UNI_IEACT_CLEAR = 0x00, /* clear call */
....
}
void
uni_mandate_epref(struct uni *uni, struct uni_ie_epref *epref)
{
....
maxact = -1;
FOREACH_ERR(e, uni) {
if (e->ie == UNI_IE_EPREF)
continue;
if (e->act == UNI_IEACT_CLEAR)
maxact = UNI_IEACT_CLEAR;
else if (e->act == UNI_IEACT_MSG_REPORT) {
if (maxact == -1 && maxact != UNI_IEACT_CLEAR) // <=
maxact = UNI_IEACT_MSG_REPORT;
} else if (e->act == UNI_IEACT_MSG_IGNORE) {
if (maxact == -1)
maxact = UNI_IEACT_MSG_IGNORE;
}
}
....
}
Результат всего условного выражения не зависит от вычисления значения «maxact != UNI_IEACT_CLEAR». Вот как это выглядит в таблице:
В этом разделе я даю три способа, как сделать ошибку в, казалось бы, простых формулах. Просто подумай об этом…
V593 Рассмотрим выражение вида A = B != C. Выражение рассчитывается следующим образом: A = (B != C). 2854
#define EINVAL 22 /* Invalid argument */
#define EFAULT 14 /* Bad address */
#define EPERM 1 /* Operation not permitted */
static int
aac_ioctl_send_raw_srb(struct aac_softc *sc, caddr_t arg)
{
....
int error, transfer_data = 0;
....
if ((error = copyin((void *)&user_srb->data_len, &fibsize,
sizeof (u_int32_t)) != 0))
goto out;
if (fibsize > (sc->aac_max_fib_size-sizeof(....))) {
error = EINVAL;
goto out;
}
if ((error = copyin((void *)user_srb, srbcmd, fibsize) != 0))
goto out;
....
out:
....
return(error);
}
В этой функции код ошибки искажается, когда присваивание выполняется в операторе «если». т.е. в выражении «error = copyin(…) != 0» сначала оценивается «copyin(…) != 0», а затем результат (0 или 1) записывается в переменную «ошибка».
В документации на функцию copyin указано, что в случае ошибки она возвращает EFAULT (значение 14), и после такой проверки результат логической операции «1» сохраняется в коде ошибки. На самом деле это EPERM, совершенно другой статус ошибки.
К сожалению, таких фрагментов довольно много.
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». 2861
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_age.c 591
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_alc.c 1535
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_ale.c 606
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_jme.c 807
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_msk.c 1626
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_stge.c 511
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». hunt_filter.c 973
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_smsc.c 1365
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». if_vte.c 431
- V593 Попробуйте просмотреть выражение типа «A = B != C». Выражение рассчитывается следующим образом: «A = (B != C)». zfs_vfsops.c 498
Струны
V541 Печатать строку буфер в себя опасно. ata-highpoint.c 102
static int
ata_highpoint_probe(device_t dev)
{
....
char buffer[64];
....
strcpy(buffer, "HighPoint ");
strcat(buffer, idx->text);
if (idx->cfg1 == HPT_374) {
if (pci_get_function(dev) == 0)
strcat(buffer, " (channel 0+1)");
if (pci_get_function(dev) == 1)
strcat(buffer, " (channel 2+3)");
}
sprintf(buffer, "%s %s controller",
buffer, ata_mode2str(idx->max_dma));
....
}
В буфере формируется некоторая строка. Затем программист хочет получить новую строку, сохранив значение предыдущей строки, и добавить еще два слова. Это кажется очень простым.
Чтобы объяснить, почему здесь будет получен неожиданный результат, приведу простой и понятный пример из документации по этой диагностике:
char s[100] = "test"; sprintf(s, "N = %d, S = %s", 123, s);
В результате работы мы хотели бы получить следующую строку:
N = 123, S = test
Но на практике будет так:
N = 123, S = N = 123, S =
В других ситуациях тот же код может привести не только к некорректному тексту, но и к прерыванию программы. Код можно исправить, если использовать новый буфер для хранения результата. Правильная версия:
char s1[100] = "test"; char s2[100]; sprintf(s2, "N = %d, S = %s", 123, s1);
V512 Вызов функции strcpy приведет к переполнению буфера p-›vendor. aacraid_cam.c 571
#define SID_VENDOR_SIZE 8
char vendor[SID_VENDOR_SIZE];
#define SID_PRODUCT_SIZE 16
char product[SID_PRODUCT_SIZE];
#define SID_REVISION_SIZE 4
char revision[SID_REVISION_SIZE];
static void
aac_container_special_command(struct cam_sim *sim, union ccb *ccb,
u_int8_t *cmdp)
{
....
/* OEM Vendor defines */
strcpy(p->vendor,"Adaptec "); // <=
strcpy(p->product,"Array "); // <=
strcpy(p->revision,"V1.0"); // <=
....
}
Все три строки здесь заполнены неправильно. В массивах нет места для нулевого терминального символа, что может вызвать серьезные проблемы с такими строками в будущем. В p-›vendor и p-›product можно убрать по одному пробелу. Тогда останется место для нулевого терминала, который функция strcpy() добавляет в конец строки. Но для символов конца строки для p-›revision свободного места вообще нет; поэтому значение SID_REVISION_SIZE нужно увеличить хотя бы на единицу.
Конечно, мне довольно сложно судить о коде. Возможно, терминальный ноль вообще не нужен и все рассчитано на конкретный размер буфера. Тогда функция strcpy() выбрана неправильно. В этом случае код должен быть написан так:
memcpy(p->vendor, "Adaptec ", SID_VENDOR_SIZE); memcpy(p->product, "Array ", SID_PRODUCT_SIZE); memcpy(p->revision, "V1.0", SID_REVISION_SIZE);
V583 Оператор ‘?:’, независимо от его условного выражения, всегда возвращает одно и то же значение: td-›td_name. subr_turnstile.c 1029
static void
print_thread(struct thread *td, const char *prefix)
{
db_printf("%s%p (tid %d, pid %d, ....", prefix, td, td->td_tid,
td->td_proc->p_pid, td->td_name[0] != '\0' ? td->td_name :
td->td_name);
}
Подозрительный фрагмент. Несмотря на проверку «td-›td_name[0] != ‘\0’», эта строка все равно печатается.
Вот такие фрагменты:
- V583 Оператор ‘?:’, независимо от его условного выражения, всегда возвращает одно и то же значение: td-›td_name. subr_turnstile.c 1112
- V583 Оператор ‘?:’, независимо от его условного выражения, всегда возвращает одно и то же значение: td-›td_name. subr_turnstile.c 1196
Операции с памятью
В этом разделе я расскажу о некорректном использовании следующих функций:
void bzero(void *b, size_t len); int copyout(const void *kaddr, void *uaddr, size_t len);
V579 Функция bzero получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте второй аргумент. osapi.c 316
/* Autosense storage */
struct scsi_sense_data sense_data;
void
ostiInitiatorIOCompleted(....)
{
....
bzero(&csio->sense_data, sizeof(&csio->sense_data));
....
}
Чтобы обнулить структуру, мы должны передать указатель структуры и размер обнуляемой памяти в байтах в функцию bzero(); но здесь в функцию передается размер указателя, а не размер структуры.
Правильный код должен быть таким:
bzero(&csio->sense_data, sizeof(csio->sense_data));
V579 Функция bzero получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте второй аргумент. acpi_package.c 83
int
acpi_PkgStr(...., void *dst, ....)
{
....
bzero(dst, sizeof(dst));
....
}
В этом примере мы видим аналогичную ситуацию: в функцию «bzero» передается размер указателя, а не объекта.
Правильная версия:
bzero(dst, sizeof(*dst));
V579 Функция копирования получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. if_nxge.c 1498
int
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
{
....
*data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) // <=
retValue = 0;
break;
....
}
В данном примере память копируется из data в ifreqp-›ifr_data, при этом размер копируемой памяти sizeof(data), т.е. 4 или 8 байт в зависимости от разрядности архитектуры.
указатели
V557 Возможен переполнение массива. Индекс 2 указывает за границу массива. if_sppsubr.c 4348
#define AUTHKEYLEN 16
struct sauth {
u_short proto; /* authentication protocol to use */
u_short flags;
#define AUTHFLAG_NOCALLOUT 1
/* callouts */
#define AUTHFLAG_NORECHALLENGE 2 /* do not re-challenge CHAP */
u_char name[AUTHNAMELEN]; /* system identification name */
u_char secret[AUTHKEYLEN]; /* secret password */
u_char challenge[AUTHKEYLEN]; /* random challenge */
};
static void
sppp_chap_scr(struct sppp *sp)
{
u_long *ch, seed;
u_char clen;
/* Compute random challenge. */
ch = (u_long *)sp->myauth.challenge;
read_random(&seed, sizeof seed);
ch[0] = seed ^ random();
ch[1] = seed ^ random();
ch[2] = seed ^ random(); // <=
ch[3] = seed ^ random(); // <=
clen = AUTHKEYLEN;
....
}
Размер типа u_char — 1 байт в 32- и 64-битных приложениях; но размер типа «u_long» составляет 4 байта в 32-битных приложениях и 8 байтов в 64-битных приложениях. Так в 32-битном приложении при выполнении операции «u_long* ch = (u_long *)sp-›myauth.challenge» массив «ch» будет состоять из 4 элементов по 4 байта. А в 64-битном приложении массив ch будет состоять из 2-х элементов по 8 байт каждый. Поэтому, если мы скомпилируем 64-битное ядро, то при обращении к ch[2] и ch[3] мы будем иметь индекс массива за пределами границ.
V503 Это бессмысленное сравнение: указатель ›= 0. geom_vinum_plex.c 173
gv_plex_offset(...., int *sdno, int growing)
{
....
*sdno = stripeno % sdcount;
....
KASSERT(sdno >= 0, ("gv_plex_offset: sdno < 0"));
....
}
Нам удалось обнаружить очень интересный фрагмент с помощью диагностики 503. Нет смысла проверять, что указатель больше или равен 0. Скорее всего, указатель «sdno» не был разыменован, чтобы сравнить хранимое значение .
Есть еще два сравнения с null.
- V503 Это бессмысленное сравнение: указатель ›= 0. geom_vinum_raid5.c 602
- V503 Это бессмысленное сравнение: указатель ›= 0. geom_vinum_raid5.c 610
V522 Может иметь место разыменование нулевого указателя sc. mrsas.c 4027
void
mrsas_aen_handler(struct mrsas_softc *sc)
{
....
if (!sc) {
device_printf(sc->mrsas_dev, "invalid instance!\n");
return;
}
if (sc->evt_detail_mem) {
....
}
Если указатель «sc» нулевой, то функция завершится. Однако не совсем понятно, зачем программист пытался разыменовать указатель «sc-›mrsas_dev».
Список странных фрагментов:
- V522 Может иметь место разыменование нулевого указателя ‘sc’. mrsas.c 1279
- V522 Может иметь место разыменование нулевого указателя ‘sc’. tws_cam.c 1066
- V522 Может иметь место разыменование нулевого указателя ‘sc’. blkfront.c 677
- V522 Может иметь место разыменование нулевого указателя dev_priv. radeon_cs.c 153
- V522 Может иметь место разыменование нулевого указателя «ha». ql_isr.c 728
V713 Указатель m использовался в логическом выражении до того, как он был проверен на соответствие nullptr в том же логическом выражении. ip_fastfwd.c 245
struct mbuf *
ip_tryforward(struct mbuf *m)
{
....
if (pfil_run_hooks(
&V_inet_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN, NULL) ||
m == NULL)
goto drop;
....
}
Чек «m == NULL» поставлен неправильно. Сначала нам нужно проверить указатель, и только потом вызывать функцию pfil_run_hooks().
Петли
V621 Рассмотрите возможность проверки оператора for. Возможно, что цикл будет выполняться некорректно или вообще не будет выполняться. if_ae.c 1663
#define AE_IDLE_TIMEOUT 100
static void
ae_stop_rxmac(ae_softc_t *sc)
{
int i;
....
/*
* Wait for IDLE state.
*/
for (i = 0; i < AE_IDLE_TIMEOUT; i--) { // <=
val = AE_READ_4(sc, AE_IDLE_REG);
if ((val & (AE_IDLE_RXMAC | AE_IDLE_DMAWRITE)) == 0)
break;
DELAY(100);
}
....
}
В исходном коде FreeBSD мы нашли вот такой интересный и неправильный цикл. По какой-то причине происходит уменьшение счетчика цикла вместо приращения. Получается, что цикл может выполняться больше раз, чем значение AE_IDLE_TIMEOUT, пока не выполнится оператор break.
Если цикл не остановить, то мы получим переполнение переменной со знаком ‘i’. Переполнение переменной со знаком — это не что иное, как неопределенное поведение. И это не какая-то абстрактная теоретическая опасность, это вполне реальная опасность. Недавно мой коллега написал статью на эту тему: Неопределенное поведение ближе, чем вы думаете
Еще один интересный момент. Такую же ошибку мы обнаружили в коде операционной системы Haiku (см. раздел Предупреждения №17, №18). Не знаю, кто позаимствовал файл if_ae.c, но эта ошибка появляется после Copy-Paste.
V535 Переменная i используется для этого цикла и для внешнего цикла. Контрольные строки: 182, 183. mfi_tbolt.c 183
mfi_tbolt_adp_reset(struct mfi_softc *sc)
{
....
for (i=0; i < 10; i++) {
for (i = 0; i < 10000; i++);
}
....
}
Возможно, этот небольшой кусок кода используется для создания задержки, но в сумме выполняется всего 10000 операций, а не 10*10000; зачем тогда тут 2 петли?
Я специально привел этот пример, потому что он наиболее наглядно показывает, что использование одной и той же переменной во внешнем и вложенном циклах приводит к неожиданным результатам.
V535 Переменная i используется для этого цикла и для внешнего цикла. Проверить строки: 197, 208. linux_vdso.c 208
void
__elfN(linux_vdso_reloc)(struct sysentvec *sv, long vdso_adjust)
{
....
for(i = 0; i < ehdr->e_shnum; i++) { // <=
if (!(shdr[i].sh_flags & SHF_ALLOC))
continue;
shdr[i].sh_addr += vdso_adjust;
if (shdr[i].sh_type != SHT_SYMTAB &&
shdr[i].sh_type != SHT_DYNSYM)
continue;
sym = (Elf_Sym *)((caddr_t)ehdr + shdr[i].sh_offset);
symcnt = shdr[i].sh_size / sizeof(*sym);
for(i = 0; i < symcnt; i++, sym++) { // <=
if (sym->st_shndx == SHN_UNDEF ||
sym->st_shndx == SHN_ABS)
continue;
sym->st_value += vdso_adjust;
}
}
....
}
Вероятно, это слишком сложный пример, чтобы понять, правильно ли выполняется код. Но глядя на предыдущий пример, можно сделать вывод, что и здесь выполняется неправильное количество итераций.
V547 Выражение j ›= 0 всегда истинно. Значение беззнакового типа всегда ›= 0. safe.c 1596
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;
}
dptr = mtod(dstm, caddr_t) + j;
dlen = dstm->m_len - j;
....
}
В этой функции есть два опасных цикла. Так как переменная ‘j’ (счетчики циклов) имеет беззнаковый тип, то проверка «j ›= 0» всегда истинна и эти циклы «бесконечны». Другая проблема заключается в том, что из этого счетчика постоянно вычитается какое-то значение; поэтому, если будет попытка доступа за пределы нулевого значения, тогда переменная j получит максимальное значение своего типа.
V711 Опасно создавать внутри цикла локальную переменную с тем же именем, что и у переменной, управляющей этим циклом. 73
static int
pn_decode_pst(device_t dev)
{
....
struct pst_header *pst; // <=
....
p = ((uint8_t *) psb) + sizeof(struct psb_header);
pst = (struct pst_header*) p;
maxpst = 200;
do {
struct pst_header *pst = (struct pst_header*) p; // <=
....
p += sizeof(struct pst_header) + (2 * pst->numpstates);
} while (cpuid_is_k7(pst->cpuid) && maxpst--); // <=
....
}
В теле цикла мы обнаружили, что объявление переменной соответствует переменной, используемой для управления циклом. Я подозреваю, что значение внешнего указателя с именем «pst» не меняется, потому что создается локальный указатель с тем же «pst». Возможно, в условии цикла do….while() всегда проверяется одно и то же значение «pst-›cupid». Разработчики должны просмотреть этот фрагмент и дать переменным разные имена.
Разное
V569 Усечение постоянного значения -96. Диапазон значений типа unsigned char: [0, 255]. if_rsu.c 1516
struct ieee80211_rx_stats {
....
uint8_t nf; /* global NF */
uint8_t rssi; /* global RSSI */
....
};
static void
rsu_event_survey(struct rsu_softc *sc, uint8_t *buf, int len)
{
....
rxs.rssi = le32toh(bss->rssi) / 2;
rxs.nf = -96;
....
}
Очень странно, что беззнаковой переменной «rxs.nf» присваивается отрицательное значение «-96». В результате переменная будет иметь значение «160».
Тело функции V729 содержит метку done, которая не используется никакими операторами goto. zfs_acl.c 2023
int
zfs_setacl(znode_t *zp, vsecattr_t *vsecp, ....)
{
....
top:
mutex_enter(&zp->z_acl_lock);
mutex_enter(&zp->z_lock);
....
if (error == ERESTART) {
dmu_tx_wait(tx);
dmu_tx_abort(tx);
goto top;
}
....
done: // <=
mutex_exit(&zp->z_lock);
mutex_exit(&zp->z_acl_lock);
return (error);
}
В этом коде есть функции содержащие метки, но при этом для этих меток отсутствует вызов оператора goto. Например, мы видим, что в этом фрагменте используется метка «top», а «done» нигде не используется. Возможно, программист забыл добавить к метке переход, либо ее со временем убрали, а метку оставили в коде.
V646 Попробуйте проверить логику приложения. Возможно, отсутствует ключевое слово else. mac_process.c 352
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;
....
}
Напоследок хочу рассказать о подозрительном форматировании, с которым я уже сталкивался в самом начале проверки проекта. Здесь код выровнен таким образом, что отсутствие ключевого слова else выглядит странно.
V705 Возможно, блок else был забыт или закомментирован, что изменило логику работы программы. scsi_da.c 3231
static void
dadone(struct cam_periph *periph, union ccb *done_ccb)
{
....
/*
* If we tried READ CAPACITY(16) and failed,
* fallback to READ CAPACITY(10).
*/
if ((state == DA_CCB_PROBE_RC16) &&
....
} else // <=
/*
* Attach to anything that claims to be a
* direct access or optical disk device,
* as long as it doesn't return a "Logical
* unit not supported" (0x25) error.
*/
if ((have_sense) && (asc != 0x25) // <=
....
} else {
....
}
....
}
Сейчас в этом коде нет ошибки, но однажды она обязательно появится. Оставляя такой большой комментарий перед else, вы можете случайно забыть, что это ключевое слово было где-то в коде, и внести ошибочные правки.
Вывод
Проект FreeBSD был протестирован специальной версией PVS-Studio, которая показала отличный результат! Весь материал невозможно уместить в одной статье. Тем не менее, команда разработчиков FreeBSD получила полный список предупреждений анализатора, которые необходимо проверить.
Предлагаю всем попробовать PVS-Studio на ваших проектах. Анализатор работает в среде Windows. Публичной версии для использования анализатора при разработке проектов под Linux/FreeBSD у нас нет. Мы также можем обсудить возможные варианты настройки PVS-Studio под ваши проекты и конкретные задачи.
