Недавно мы рассказывали, что анализатор кода PVS-Studio начал интегрироваться с PlatformIO. Естественно, при этом команда разработчиков PVS-Studio общалась с командой PlatformIO и те предложили ради интереса проверить код операционной системы реального времени Zephyr. Почему бы и нет, подумали мы, и вот перед вами статья о таком исследовании.
PlatformIO
Прежде чем приступить к основной части статьи, хочется отрекомендовать разработчикам встраиваемых систем проект PlatformIO, который может облегчить им жизнь. Это кроссплатформенный инструмент для программирования микроконтроллеров. Ядром PlatformIO является инструмент с интерфейсом командной строки, однако рекомендуется использовать его в виде плагина для Visual Studio Code. Поддерживается большое количество современных чипов и плат на их основе. Умеет автоматически загружать подходящие системы сборки, а на сайте собрана большая коллекция библиотек для управления подключаемыми электронными компонентами.
PVS-Studio
PVS-Studio пока мало известен в мире встраиваемых систем, поэтому я на всякий случай сделаю вступление для новых читателей, пока ещё не знакомых с этим инструментом. Наши постоянные читатели могут сразу перейти к следующему разделу.
PVS-Studio — это статический анализатор кода, позволяющий выявлять ошибки и потенциальные уязвимости в коде программ, написанных на языках C, C++, C# и Java. Если говорить только о C и C++, то поддерживаются следующие компиляторы:
- Windows. Visual Studio 2010-2019 C, C++, C++/CLI, C++/CX (WinRT)
- Windows. IAR Embedded Workbench, C/C++ Compiler for ARM C, C++
- Windows. QNX Momentics, QCC C, C++
- Windows/Linux. Keil µVision, DS-MDK, ARM Compiler 5/6 C, C++
- Windows/Linux. Texas Instruments Code Composer Studio, ARM Code Generation Tools C, C++
- Windows/Linux/macOS. GNU Arm Embedded Toolchain, Arm Embedded GCC compiler, C, C++
- Windows/Linux/macOS. Clang C, C++
- Linux/macOS. GCC C, C++
- Windows. MinGW C, C++
Анализатор имеет свою собственную систему классификации предупреждений, но в случае необходимости вы можете включить отображение предупреждений согласно стандартам кодирования CWE, SEI CERT, MISRA.
Вы можете быстро начать регулярно использовать PVS-Studio даже в большом legacy-проекте. Для этого предусмотрен специальный механизм массового подавления предупреждений. Все текущие предупреждения считаются техническим долгом и прячутся, что позволяет сосредоточиться на предупреждениях, относящихся только к новому или модифицированному коду. Это позволяет команде сразу начать ежедневно использовать анализатор в своей работе, а к техническому долгу можно время от времени возвращаться и улучшать код.
Существует множество других сценариев использования PVS-Studio. Например, вы можете использовать его как плагин к SonarQube. Возможна интеграция с такими системами, как Travis CI, CircleCI, GitLab CI/CD и т.д. Более подробное описание PVS-Studio выходит за рамки этой статьи. Поэтому предлагаю ознакомиться со статьёй, в которой много полезных ссылок, и в которой даны ответы на многие вопросы: "Причины внедрить в процесс разработки статический анализатор кода PVS-Studio".
Zephyr
Работая над интеграцией PVS-Studio в PlatformIO, наши команды пообщались, и нам предложили проверить проект из мира embedded, а именно — Zephyr. Идея нам понравилась, что и послужило поводом к написанию этой статьи.
Zephyr — легковесная операционная система реального времени, предназначенная для работы на устройствах с ограниченными ресурсами различных архитектур. Код распространяется под открытой лицензией Apache 2.0. Работает на следующих платформах: ARM (Cortex-M0, Cortex-M3, Cortex-M4, Cortex-M23, Cortex-M33, Cortex-R4, Cortex-R5, Cortex-A53), x86, x86-64, ARC, RISC-V, Nios II, Xtensa.
Некоторые особенности:
- Единое адресное пространство. Специфичный код приложения в сочетании с кастомным ядром создают монолитный образ, исполняемый на устройстве.
- Большие возможности настройки. Приложение получает только те возможности, которые ему нужны и когда они ему нужны.
- Ресурсы определяются во время компиляции. Это уменьшает размер кода и увеличивает производительность.
- Минимальный контроль ошибок. Служит для того же самого. При этом во время тестирования есть возможность получать полную отладочную информацию.
- Богатый набор возможностей для разработчика: многопоточность, контроль прерываний, внутрипотоковая синхронизация, средства для работы с памятью, управление питанием и многое другое.
Из интересных для нас моментов, в разработке операционной системы принимает участие компания Synopsys. В 2014 году компания Synopsys приобрела компанию Coverity, выпускавшую одноименный статический анализатор кода.
Совершенно естественно, что с самого начала при разработке Zephyr используется анализатор Coverity. Анализатор является лидером рынка и это не могло не сказаться в лучшую сторону на качестве кода операционной системы.
Качество кода Zephyr
По моему мнению, код операционной системы Zephyr является качественным. Вот что даёт повод мне так думать:
- Анализатор PVS-Studio выдал 122 предупреждения общего назначения уровня High и 367 предупреждений уровня Medium. Это немного, учитывая, что было проверено 560 C/C++ файлов. Ядро проверяется путём проверки сэмплов. Всего же в проекте я насчитал 7810 C/C++ файлов и 10075 заголовочных файлов. Получается, что проверена только часть проекта. Впрочем, задача проверить всё и не ставилась, а полученные результаты всё равно говорят о небольшой плотности предупреждений.
- Многие из выданных предупреждений являются ложными или полуложными. Что подразумевается под «полуложными» предупреждениями, я поясню ниже.
- Утилита SourceMonitor, проанализировав исходный код, выдала статистику, что 48% кода является комментариями. Это много и по моему опыту свидетельствует о высокой заботе о качестве кода, его понятности для других разработчиков.
- При разработке проекта используется статический анализатор кода Coverity. Скорее всего, в силу именно этого факта, анализатор PVS-Studio хотя и нашёл ошибки в проекте, но не смог показать себя ярко, как это иногда бывает при анализе других проектов.
На основании этого я считаю, что авторы проекта заботятся о качестве и надёжности кода. Давайте теперь рассмотрим некоторые предупреждения, выданные анализатором PVS-Studio (версия 7.06).
«Полуложные» предупреждения
Код проекта в силу своей низкоуровневости написан достаточно специфично и с большим количеством условной компиляции (#ifdef). Это порождает большое количество предупреждений, которые не указывают на настоящую ошибку, но при этом их нельзя назвать просто ложными. Проще всего будет пояснить это, приведя несколько примеров.
Пример «полуложного» срабатывания N1
static struct char_framebuffer char_fb;
int cfb_framebuffer_invert(struct device *dev)
{
struct char_framebuffer *fb = &char_fb;
if (!fb || !fb->buf) {
return -1;
}
fb->inverted = !fb->inverted;
return 0;
}Предупреждение PVS-Studio: V560 A part of conditional expression is always false: !fb. cfb.c 188
При взятии адреса статической переменной всегда получается ненулевой указатель. Поэтому указатель fb всегда не равен нулю и его проверка не имеет смысла.
Однако видно, что это никакая не ошибка, а просто избыточная проверка, которая ничем не вредит. Более того, при построении Release версии компилятор её выбросит, так что это даже не повлечёт замедления работы.
Подобный случай как раз и попадает в моём понимании под понятие «полуложное» срабатывание анализатора. Формально, анализатор совершенно прав. И лучше удалить лишнюю бессмысленную проверку из кода. Однако всё это мелко и подобные предупреждения неинтересно даже рассматривать в рамках статьи.
Пример «полуложного» срабатывания N2
int hex2char(u8_t x, char *c)
{
if (x <= 9) {
*c = x + '0';
} else if (x >= 10 && x <= 15) {
*c = x - 10 + 'a';
} else {
return -EINVAL;
}
return 0;
}Предупреждение PVS-Studio: V560 A part of conditional expression is always true: x >= 10. hex.c 31
Анализатор вновь формально прав, утверждая, что часть условия всегда истинна. Если переменная x не меньше/равна 9, то получается, что она всегда больше/равна 10. И код можно упростить:
} else if (x <= 15) {Вновь понятно, что никакой настоящей вредной ошибки здесь нет, и лишнее сравнение написано просто для красоты кода.
Теперь давайте рассмотрим более сложный пример N3
Для начала посмотрим, как может быть реализован макрос CHECKIF.
#if defined(CONFIG_ASSERT_ON_ERRORS)
#define CHECKIF(expr) \
__ASSERT_NO_MSG(!(expr)); \
if (0)
#elif defined(CONFIG_NO_RUNTIME_CHECKS)
#define CHECKIF(...) \
if (0)
#else
#define CHECKIF(expr) \
if (expr)
#endifВ зависимости от режима компиляции проекта, проверка может как выполняться, так и пропускаться. В нашем случае при проверке кода с помощью PVS-Studio выбиралась эта реализация макроса:
#define CHECKIF(expr) \
if (expr)Посмотрим теперь, к чему это приводит.
int k_queue_append_list(struct k_queue *queue, void *head, void *tail)
{
CHECKIF(head == NULL || tail == NULL) {
return -EINVAL;
}
k_spinlock_key_t key = k_spin_lock(&queue->lock);
struct k_thread *thread = NULL;
if (head != NULL) {
thread = z_unpend_first_thread(&queue->wait_q);
}
....
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'head != NULL' is always true. queue.c 244
Анализатор считает, что проверка (head != NULL) всегда даёт истину. И это действительно так. Если указатель head был равен NULL, то функция бы прекратила свою работу благодаря проверке в начале функции:
CHECKIF(head == NULL || tail == NULL) {
return -EINVAL;
}Напомним, что здесь макрос раскрывается так:
if (head == NULL || tail == NULL) {
return -EINVAL;
}Итак, анализатор PVS-Studio прав со своей точки зрения и выдаёт корректное предупреждение. Однако удалить эту проверку нельзя. Она нужна. При другом сценарии макрос раскроется так:
if (0) {
return -EINVAL;
}И тогда повторная проверка указателя нужна. Конечно, анализатор не выдаст предупреждение в таком варианте компиляции кода. Однако он выдаёт предупреждение для отладочного варианта компиляции.
Надеюсь, теперь читателям понятно, откуда берутся «полуложные» предупреждения. Впрочем, ничего страшного в них нет. Анализатор PVS-Studio предоставляет различные механизмы подавления ложных предупреждений, с которыми можно ознакомиться в документации.
Предупреждения по делу
А удалось ли найти всё-таки что-то интересное? Удалось, и сейчас мы посмотрим на различные ошибки. При этом хочу сразу отметить два момента:
- Суть статического анализа не в подобных разовых проверках. Правильная методология: регулярный запуск анализатора, как, собственно, сейчас в проекте и используется Coverity. Поэтому, добавив в процесс разработки PVS-Studio или какой-либо другой анализатор, можно на раннем этапе найти ещё больше ошибок и тем самым снизить стоимость их исправления.
- При написании статьи у меня не было цели найти как можно больше ошибок. Поэтому, многие ошибки остались мной незамеченными или я зря отнёс их к категории «полуложных» срабатываний и не включил в статью. Если авторов проекта заинтересует эта публикация, я рекомендую им самостоятельно провести анализ и изучить полученный отчёт. Так как проект является открытым и находится на GitHub, то можно воспользоваться бесплатным вариантом лицензирования PVS-Studio.
Фрагмент N1, опечатка
static void gen_prov_ack(struct prov_rx *rx, struct net_buf_simple *buf)
{
....
if (link.tx.cb && link.tx.cb) {
link.tx.cb(0, link.tx.cb_data);
}
....
}Предупреждение PVS-Studio: V501 [CWE-571] There are identical sub-expressions to the left and to the right of the '&&' operator: link.tx.cb && link.tx.cb pb_adv.c 377
Дважды проверяется одна и та же переменная link.tx.cb. Видимо, это опечатка, и второй проверяемой переменной должна выступать link.tx.cb_data.
Фрагмент N2, выход за границу буфера
Рассмотрим функцию net_hostname_get, которая будет использоваться дальше.
#if defined(CONFIG_NET_HOSTNAME_ENABLE)
const char *net_hostname_get(void);
#else
static inline const char *net_hostname_get(void)
{
return "zephyr";
}
#endifВ моём случае при препроцессировании выбирался вариант, относящийся к ветке #else. То-есть в препроцессированном файле функция реализуется так:
static inline const char *net_hostname_get(void)
{
return "zephyr";
}Функция возвращает указатель на массив из 7 байт (учитываем терминальный ноль в конце строки).
Теперь рассмотрим код, приводящий к выходу за границу массива.
static int do_net_init(void)
{
....
(void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);
....
}Предупреждение PVS-Studio: V512 [CWE-119] A call of the 'memcpy' function will lead to the 'net_hostname_get()' buffer becoming out of range. log_backend_net.c 114
После препроцессирования MAX_HOSTNAME_LEN раскрывается следующим образом:
(void)memcpy(hostname, net_hostname_get(),
sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));Соответственно, при копировании данных возникает выход за границу строкового литерала. Как это скажется на выполнении программы — предсказать сложно, так как это приводит к неопределённому поведению.
Фрагмент N3, потенциальный выход за границу буфера
int do_write_op_json(struct lwm2m_message *msg)
{
u8_t value[TOKEN_BUF_LEN];
u8_t base_name[MAX_RESOURCE_LEN];
u8_t full_name[MAX_RESOURCE_LEN];
....
/* combine base_name + name */
snprintf(full_name, TOKEN_BUF_LEN, "%s%s", base_name, value);
....
}Предупреждение PVS-Studio: V512 [CWE-119] A call of the 'snprintf' function will lead to overflow of the buffer 'full_name'. lwm2m_rw_json.c 826
Если подставить значения макросов, то картина происходящего выглядит следующим образом:
u8_t value[64];
u8_t base_name[20];
u8_t full_name[20];
....
snprintf(full_name, 64, "%s%s", base_name, value);Под буфер full_name, в котором формируется строка, выделено только 20 байт. При этом части, из которых формируется строка, хранятся в буферах размером 20 и 64 байта. Более того, константа 64, передаваемая в функцию snprintf и призванная предотвратить выход за границу массива, явно великовата!
Этот код не обязательно приведёт к переполнению буфера. Возможно, всегда везёт, и подстроки всегда очень маленькие. Однако, в целом, этот код никак не защищён от переполнения и содержит классический дефект безопасности CWE-119.
Фрагмент N4, выражение всегда истинно
static int keys_set(const char *name, size_t len_rd, settings_read_cb read_cb,
void *cb_arg)
{
....
size_t len;
....
len = read_cb(cb_arg, val, sizeof(val));
if (len < 0) {
BT_ERR("Failed to read value (err %zu)", len);
return -EINVAL;
}
....
}Предупреждение PVS-Studio: V547 [CWE-570] Expression 'len < 0' is always false. Unsigned type value is never < 0. keys.c 312
Переменная len имеет беззнаковый тип и, значит, не может быть меньше 0. Соответственно, статус ошибки никак не обрабатывается. В других местах для хранения результата работы функции read_cb используется тип int или ssize_t.
Пример:
static inline int mesh_x_set(....)
{
ssize_t len;
len = read_cb(cb_arg, out, read_len);
if (len < 0) {
....
}Примечание. Кажется с функцией read_cb вообще всё плохо. Дело в том, что она объявлено так:
static u8_t read_cb(const struct bt_gatt_attr *attr, void *user_data)Тип u8_t это unsigned char.
Функция всегда возвращает только положительные числа типа unsigned char. Если поместить это значение в знаковую переменную типа int или ssize_t, всё равно значение всегда будет положительным. Следовательно, в других местах проверки на статус ошибки тоже не работают. Но я не углублялся в изучение этого вопроса.
Фрагмент N5, что-то очень странное
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
((u8_t *)mntpt)[strlen(mntpt)] = '\0';
memcpy(cpy_mntpt, mntpt, strlen(mntpt));
}
return cpy_mntpt;
}Предупреждение PVS-Studio: V575 [CWE-628] The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. shell.c 427
Кто-то пытался сделать аналог функции strdup, но у него это не получилось.
Начнём с предупреждения анализатора. Он сообщает, что функция memcpy копирует строчку, но не скопирует терминальный ноль, и это очень подозрительно.
Кажется, что этот терминальный 0 копируется здесь:
((u8_t *)mntpt)[strlen(mntpt)] = '\0';Но нет! Здесь опечатка, из-за которой терминальный ноль копируется сам в себя! Обратите внимание, что запись происходит в массив mntpt, а не в cpy_mntpt. В итоге функция mntpt_prepare возвращает строку, незавершенную терминальным нулём.
На самом деле программист хотел написать так:
((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';Однако всё равно не понятно, зачем сделано так сложно! Этот код можно упростить до следующего варианта:
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
strcpy(cpy_mntpt, mntpt);
}
return cpy_mntpt;
}Фрагмент N6, разыменование указателя до проверки
int bt_mesh_model_publish(struct bt_mesh_model *model)
{
....
struct bt_mesh_model_pub *pub = model->pub;
....
struct bt_mesh_msg_ctx ctx = {
.send_rel = pub->send_rel,
};
....
if (!pub) {
return -ENOTSUP;
}
....
}Предупреждение PVS-Studio: V595 [CWE-476] The 'pub' pointer was utilized before it was verified against nullptr. Check lines: 708, 719. access.c 708
Очень распространённый паттерн ошибки. Вначале указатель разыменовывается для инициализации члена структуры:
.send_rel = pub->send_rel,И только потом следует проверка на то, что этот указатель может быть нулевым.
Фрагмент N7-N9, разыменование указателя до проверки
int net_tcp_accept(struct net_context *context, net_tcp_accept_cb_t cb,
void *user_data)
{
....
struct tcp *conn = context->tcp;
....
conn->accept_cb = cb;
if (!conn || conn->state != TCP_LISTEN) {
return -EINVAL;
}
....
}Предупреждение PVS-Studio: V595 [CWE-476] The 'conn' pointer was utilized before it was verified against nullptr. Check lines: 1071, 1073. tcp2.c 1071
То же самое, что и в предыдущем случае. Пояснение здесь не требуется.
Ещё две такие ошибки можно увидеть здесь:
- V595 [CWE-476] The 'context->tcp' pointer was utilized before it was verified against nullptr. Check lines: 1512, 1518. tcp.c 1512
- V595 [CWE-476] The 'fsm' pointer was utilized before it was verified against nullptr. Check lines: 365, 382. fsm.c 365
Фрагмент N10, ошибочная проверка
static int x509_get_subject_alt_name( unsigned char **p,
const unsigned char *end,
mbedtls_x509_sequence *subject_alt_name)
{
....
while( *p < end )
{
if( ( end - *p ) < 1 )
return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS +
MBEDTLS_ERR_ASN1_OUT_OF_DATA );
....
}
....
}Предупреждение PVS-Studio: V547 [CWE-570] Expression '(end — * p) < 1' is always false. x509_crt.c 635
Внимательно посмотрите на условия:
- *p < end
- (end — *p) < 1
Они противоречат друг другу.
Если (*p < end), то (end — *p) всегда даст значение 1 или больше. В общем, здесь что-то не так, но как должно быть написано правильно, я не знаю.
Фрагмент N11, недостижимый код
uint32_t lv_disp_get_inactive_time(const lv_disp_t * disp)
{
if(!disp) disp = lv_disp_get_default();
if(!disp) {
LV_LOG_WARN("lv_disp_get_inactive_time: no display registered");
return 0;
}
if(disp) return lv_tick_elaps(disp->last_activity_time);
lv_disp_t * d;
uint32_t t = UINT32_MAX;
d = lv_disp_get_next(NULL);
while(d) {
t = LV_MATH_MIN(t, lv_tick_elaps(d->last_activity_time));
d = lv_disp_get_next(d);
}
return t;
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'disp' is always true. lv_disp.c 148
Функция прекращает свою работу, если disp является нулевым указателем. Далее, наоборот, проверяется, что указатель disp не нулевой (а это всегда так), и функция опять-таки заканчивает свою работу.
В результате часть кода в функции вообще никогда не получит управление.
Фрагмент N12, странное возвращаемое значение
static size_t put_end_tlv(struct lwm2m_output_context *out, u16_t mark_pos,
u8_t *writer_flags, u8_t writer_flag,
int tlv_type, int tlv_id)
{
struct tlv_out_formatter_data *fd;
struct oma_tlv tlv;
u32_t len = 0U;
fd = engine_get_out_user_data(out);
if (!fd) {
return 0;
}
*writer_flags &= ~writer_flag;
len = out->out_cpkt->offset - mark_pos;
/* use stored location */
fd->mark_pos = mark_pos;
/* set instance length */
tlv_setup(&tlv, tlv_type, tlv_id, len);
len = oma_tlv_put(&tlv, out, NULL, true) - tlv.length;
return 0;
}Предупреждение PVS-Studio: V1001 The 'len' variable is assigned but is not used by the end of the function. lwm2m_rw_oma_tlv.c 338
Функция содержит два оператора return, которые оба возвращают 0. Это странно, что функция всегда возвращает 0. Ещё странно, что переменная len после присваивания больше никак не используется. У меня есть большое подозрение, что на самом деле должно быть написано так:
len = oma_tlv_put(&tlv, out, NULL, true) - tlv.length;
return len;
}Фрагмент N13-N16, ошибка синхронизации
static int nvs_startup(struct nvs_fs *fs)
{
....
k_mutex_lock(&fs->nvs_lock, K_FOREVER);
....
if (fs->ate_wra == fs->data_wra && last_ate.len) {
return -ESPIPE;
}
....
end:
k_mutex_unlock(&fs->nvs_lock);
return rc;
}Предупреждение PVS-Studio: V1020 The function exited without calling the 'k_mutex_unlock' function. Check lines: 620, 549. nvs.c 620
Существует ситуация, когда функция завершит свою работу, не разлочив мьютекс. Как я понимаю, правильно было бы написать так:
static int nvs_startup(struct nvs_fs *fs)
{
....
k_mutex_lock(&fs->nvs_lock, K_FOREVER);
....
if (fs->ate_wra == fs->data_wra && last_ate.len) {
rc = -ESPIPE;
goto end;
}
....
end:
k_mutex_unlock(&fs->nvs_lock);
return rc;
}Ещё три таких ошибки:
- V1020 The function exited without calling the 'k_mutex_unlock' function. Check lines: 574, 549. nvs.c 574
- V1020 The function exited without calling the 'k_mutex_unlock' function. Check lines: 908, 890. net_context.c 908
- V1020 The function exited without calling the 'k_mutex_unlock' function. Check lines: 1194, 1189. shell.c 1194
Заключение
Надеюсь, вам понравилось. Приходите к нам в блог читать про проверки других проектов и иные интересные публикации.
Используйте статические анализаторы в своей работе, чтобы сократить количество ошибок и потенциальных уязвимостей ещё на этапе написания кода. Особенно раннее обнаружение ошибок актуально для встраиваемых систем, обновление программ в которых часто является трудоёмким и дорогим процессом.
Также предлагаю не откладывать и попробовать проверить ваши проекты с помощью анализатора PVS-Studio. См. статью: Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking the Code of Zephyr Operating System.
