Недавно мы рассказывали, что анализатор кода PVS-Studio начал интегрироваться с PlatformIO. Естественно, при этом команда разработчиков PVS-Studio общалась с командой PlatformIO и те предложили ради интереса проверить код операционной системы реального времени Zephyr. Почему бы и нет, подумали мы, и вот перед вами статья о таком исследовании.
Прежде чем приступить к основной части статьи, хочется отрекомендовать разработчикам встраиваемых систем проект PlatformIO, который может облегчить им жизнь. Это кроссплатформенный инструмент для программирования микроконтроллеров. Ядром PlatformIO является инструмент с интерфейсом командной строки, однако рекомендуется использовать его в виде плагина для Visual Studio Code. Поддерживается большое количество современных чипов и плат на их основе. Умеет автоматически загружать подходящие системы сборки, а на сайте собрана большая коллекция библиотек для управления подключаемыми электронными компонентами. Есть поддержка нескольких статических анализаторов кода, в том числе и PVS-Studio.
PVS-Studio пока мало известен в мире встраиваемых систем, поэтому я на всякий случай сделаю вступление для новых читателей, пока ещё не знакомых с этим инструментом. Наши постоянные читатели могут сразу перейти к следующему разделу.
PVS-Studio - это статический анализатор кода, позволяющий выявлять ошибки и потенциальные уязвимости в коде программ, написанных на языках C, C++, C# и Java. Если говорить только о C и C++, то поддерживаются следующие компиляторы:
Анализатор имеет свою собственную систему классификации предупреждений, но в случае необходимости вы можете включить отображение предупреждений согласно стандартам кодирования CWE, SEI CERT, MISRA.
Вы можете быстро начать регулярно использовать PVS-Studio даже в большом legacy-проекте. Для этого предусмотрен специальный механизм массового подавления предупреждений. Все текущие предупреждения считаются техническим долгом и прячутся, что позволяет сосредоточиться на предупреждениях, относящихся только к новому или модифицированному коду. Это позволяет команде сразу начать ежедневно использовать анализатор в своей работе, а к техническому долгу можно время от времени возвращаться и улучшать код.
Существует множество других сценариев использования PVS-Studio. Например, вы можете использовать его как плагин к SonarQube. Возможна интеграция с такими системами, как Travis CI, CircleCI, GitLab CI/CD и т.д. Более подробное описание PVS-Studio выходит за рамки этой статьи. Поэтому предлагаю ознакомиться со статьёй, в которой много полезных ссылок, и в которой даны ответы на многие вопросы: "Причины внедрить в процесс разработки статический анализатор кода PVS-Studio".
Работая над интеграцией 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 является качественным. Вот что даёт повод мне так думать:
На основании этого я считаю, что авторы проекта заботятся о качестве и надёжности кода. Давайте теперь рассмотрим некоторые предупреждения, выданные анализатором 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 предоставляет различные механизмы подавления ложных предупреждений, с которыми можно ознакомиться в документации.
А удалось ли найти всё-таки что-то интересное? Удалось, и сейчас мы посмотрим на различные ошибки. При этом хочу сразу отметить два момента:
Фрагмент 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
То же самое, что и в предыдущем случае. Пояснение здесь не требуется.
Ещё две такие ошибки можно увидеть здесь:
Фрагмент 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 или больше. В общем, здесь что-то не так, но как должно быть написано правильно, я не знаю.
Фрагмент 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;
}Ещё три таких ошибки:
Надеюсь, вам понравилось. Приходите к нам в блог читать про проверки других проектов и иные интересные публикации.
Используйте статические анализаторы в своей работе, чтобы сократить количество ошибок и потенциальных уязвимостей ещё на этапе написания кода. Особенно раннее обнаружение ошибок актуально для встраиваемых систем, обновление программ в которых часто является трудоёмким и дорогим процессом.
Также предлагаю не откладывать и попробовать проверить ваши проекты с помощью анализатора PVS-Studio. См. статью: Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?