НОВОСТИ Исследуем качество кода операционной системы Zephyr

BDFpromo · 02.04.2020

Недавно мы рассказывали, что анализатор кода PVS-Studio начал интегрироваться с PlatformIO. Естественно, при этом команда разработчиков PVS-Studio общалась с командой PlatformIO и те предложили ради интереса проверить код операционной системы реального времени Zephyr. Почему бы и нет, подумали мы, и вот перед вами статья о таком исследовании.

PVS-Studio

PVS-Studio пока мало известен в мире встраиваемых систем, поэтому я на всякий случай сделаю вступление для новых читателей, пока ещё не знакомых с этим инструментом. Наши постоянные читатели могут сразу перейти к следующему разделу.

You must be registered for see links

— это статический анализатор кода, позволяющий выявлять ошибки и потенциальные уязвимости в коде программ, написанных на языках 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++

Анализатор имеет свою собственную систему классификации

You must be registered for see links

, но в случае необходимости вы можете включить отображение предупреждений согласно стандартам кодирования

You must be registered for see links

,

You must be registered for see links

,

You must be registered for see links

.

Вы можете быстро начать регулярно использовать PVS-Studio даже в большом legacy-проекте. Для этого предусмотрен специальный механизм массового

You must be registered for see links

. Все текущие предупреждения считаются техническим долгом и прячутся, что позволяет сосредоточиться на предупреждениях, относящихся только к новому или модифицированному коду. Это позволяет команде сразу начать ежедневно использовать анализатор в своей работе, а к техническому долгу можно время от времени возвращаться и улучшать код.

Существует множество других сценариев использования PVS-Studio. Например, вы можете использовать его как плагин к SonarQube. Возможна интеграция с такими системами, как Travis CI, CircleCI, GitLab CI/CD и т.д. Более подробное описание PVS-Studio выходит за рамки этой статьи. Поэтому предлагаю ознакомиться со статьёй, в которой много полезных ссылок, и в которой даны ответы на многие вопросы: "

You must be registered for see links

".

Zephyr

Работая над

You must be registered for see links

, наши команды пообщались, и нам предложили проверить проект из мира embedded, а именно — Zephyr. Идея нам понравилась, что и послужило поводом к написанию этой статьи.

You must be registered for see links

— легковесная операционная система реального времени, предназначенная для работы на устройствах с ограниченными ресурсами различных архитектур. Код распространяется под открытой лицензией 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.

Некоторые особенности:

Единое адресное пространство. Специфичный код приложения в сочетании с кастомным ядром создают монолитный образ, исполняемый на устройстве.
Большие возможности настройки. Приложение получает только те возможности, которые ему нужны и когда они ему нужны.
Ресурсы определяются во время компиляции. Это уменьшает размер кода и увеличивает производительность.
Минимальный контроль ошибок. Служит для того же самого. При этом во время тестирования есть возможность получать полную отладочную информацию.
Богатый набор возможностей для разработчика: многопоточность, контроль прерываний, внутрипотоковая синхронизация, средства для работы с памятью, управление питанием и многое другое.

Из интересных для нас моментов, в разработке операционной системы принимает участие компания

You must be registered for see links

. В 2014 году компания Synopsys приобрела компанию Coverity, выпускавшую одноименный статический анализатор кода.

Совершенно естественно, что с самого начала при разработке Zephyr используется анализатор

You must be registered for see links

. Анализатор является лидером рынка и это не могло не сказаться в лучшую сторону на качестве кода операционной системы.

Качество кода Zephyr

По моему мнению, код операционной системы Zephyr является качественным. Вот что даёт повод мне так думать:

Анализатор PVS-Studio выдал 122 предупреждения общего назначения уровня High и 367 предупреждений уровня Medium. Это немного, учитывая, что было проверено 560 C/C++ файлов. Ядро проверяется путём проверки сэмплов. Всего же в проекте я насчитал 7810 C/C++ файлов и 10075 заголовочных файлов. Получается, что проверена только часть проекта. Впрочем, задача проверить всё и не ставилась, а полученные результаты всё равно говорят о небольшой плотности предупреждений.
Многие из выданных предупреждений являются ложными или полуложными. Что подразумевается под «полуложными» предупреждениями, я поясню ниже.
Утилита
You must be registered for see links
, проанализировав исходный код, выдала статистику, что 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:

You must be registered for see links

A part of conditional expression is always false: !fb. cfb.c 188

При взятии адреса статической переменной всегда получается ненулевой указатель. Поэтому указатель fb всегда не равен нулю и его проверка не имеет смысла.

Однако видно, что это никакая не ошибка, а просто избыточная проверка, которая ничем не вредит. Более того, при построении Release версии компилятор её выбросит, так что это даже не повлечёт замедления работы.

Подобный случай как раз и попадает в моём понимании под понятие «полуложное» срабатывание анализатора. Формально, анализатор совершенно прав. И лучше удалить лишнюю бессмысленную проверку из кода. Однако всё это мелко и подобные предупреждения неинтересно даже рассматривать в рамках статьи.

Пример «полуложного» срабатывания N2

int hex2char(u8_t x, char *c)
{
if (x = 10 && x code>

Предупреждение PVS-Studio:

You must be registered for see links

A part of conditional expression is always true: x >= 10. hex.c 31

Анализатор вновь формально прав, утверждая, что часть условия всегда истинна. Если переменная x не меньше/равна 9, то получается, что она всегда больше/равна 10. И код можно упростить:

} else if (x code>

Вновь понятно, что никакой настоящей вредной ошибки здесь нет, и лишнее сравнение написано просто для красоты кода.

Теперь давайте рассмотрим более сложный пример 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:

You must be registered for see links

[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, то можно воспользоваться
You must be registered for see links
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:

You must be registered for see links

[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:

You must be registered for see links

[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:

You must be registered for see links

[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 и призванная предотвратить выход за границу массива, явно великовата!

Этот код не обязательно приведёт к переполнению буфера. Возможно, всегда везёт, и подстроки всегда очень маленькие. Однако, в целом, этот код никак не защищён от переполнения и содержит классический дефект безопасности

You must be registered for see links

.

Фрагмент 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:

You must be registered for see links

[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:

You must be registered for see links

[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:

You must be registered for see links

[CWE-476] The 'pub' pointer was utilized before it was verified against nullptr. Check lines: 708, 719. access.c 708

Очень

You must be registered for see links

паттерн ошибки. Вначале указатель разыменовывается для инициализации члена структуры:

.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:

You must be registered for see links

[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:

You must be registered for see links

[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:

You must be registered for see links

[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:

You must be registered for see links

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:

You must be registered for see links

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

Заключение

Надеюсь, вам понравилось. Приходите к нам в

You must be registered for see links

читать про проверки других проектов и иные интересные публикации.

Используйте статические анализаторы в своей работе, чтобы сократить количество ошибок и потенциальных уязвимостей ещё на этапе написания кода. Особенно раннее обнаружение ошибок актуально для встраиваемых систем, обновление программ в которых часто является трудоёмким и дорогим процессом.

Также предлагаю не откладывать и попробовать проверить ваши проекты с помощью анализатора PVS-Studio. См. статью:

You must be registered for see links

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov.

You must be registered for see links

.

НОВОСТИ Исследуем качество кода операционной системы Zephyr

BDFpromo