Embox – это кросс-платформенная мультизадачная операционная система реального времени для встраиваемых систем. Она рассчитана на работу в условиях невысоких вычислительных ресурсов и позволяет запускать Linux-приложения на микроконтроллерах без использования самого Linux. Конечно, как и любые другие приложения, Embox баги тоже не обошли стороной. Данная статья посвящена разбору ошибок, найденных в коде проекта Embox.

Несколько месяцев назад я уже писал статью про проверку FreeRTOS — еще одной ОС для встраиваемых систем. Ошибок в ней я тогда не нашел, зато нашел их в библиотеках, добавленных ребятами из Amazon при разработке собственной версии FreeRTOS.

Статья, которую вы видите сейчас перед собой, в каком-то смысле продолжает тематику предыдущей. Нам часто приходили просьбы проверить FreeRTOS, и мы это сделали. В этот раз просьбы проверить какой-то конкретный проект не поступало, но я стал получать письма и комментарии от embedded-разработчиков, которым понравилось и которые хотят еще.

Что ж, новый выпуск журнала «PVS-Studio for Embedded» созрел и лежит перед вами. Приятного просмотра!

Проведение анализа

Анализ проводился с помощью PVS-Studio – статического анализатора кода для C, C++, C# и Java. Перед анализом проект необходимо собрать – так мы будем уверены, что код проекта рабочий, а также дадим анализатору возможность собрать ту информацию о сборке, которая может быть полезна для более качественной проверки кода.

Инструкция в официальном репозитории Embox предлагает возможность сборки под разными системами (Arch Linux, macOS, Debian) и с помощью Docker. Я решил внести немного разнообразия в свой быт и провести сборку и анализ из-под Debian, который я недавно установил себе на виртуальную машину.

Сборка прошла без проблем. Теперь нужно было провести анализ. Debian является одной из поддерживаемых PVS-Studio Linux-based систем. Удобный способ для проверки проектов из-под Linux – трассировка компиляции. Это специальный режим, при котором анализатор собирает всю необходимую информацию о сборке, чтобы потом можно было одним кликом запустить анализ. Всё, что мне нужно было сделать, это:

1) Скачать и установить PVS-Studio;

2) Запустить отслеживание сборки, перейдя в папку с Embox и набрав в терминале

pvs-studio-analyzer analyze -- make

3) Дождавшись окончания сборки запустить команду:

pvs-studio-analyzer analyze -o /path/to/output.log

4) Конвертировать сырой отчёт в любой удобный для вас формат. В комплекте с анализатором идет специальная утилита PlogConverter, с помощью которой можно это сделать. Например, команда для конвертации отчёта в tasklist (для просмотра, например, в QtCreator) будет выглядеть вот так:

plog-converter -t tasklist -o /path/to/output.tasks /path/to/project

Всё! На выполнение этих пунктов у меня ушло не более 15 минут. Отчет готов, теперь можно просматривать ошибки. Ну, приступим :)

Странный цикл

Одной из ошибок, найденных анализатором, был странный цикл while:

int main(int argc, char **argv) {
  ....

  while (dp.skip != 0 ) {
    n_read = read(ifd, tbuf, dp.bs);
    if (n_read < 0) {
      err = -errno;
      goto out_cmd;
    }
    if (n_read == 0) {
      goto out_cmd;
    }

    dp.skip --;
  } while (dp.skip != 0);       // <=

  do {
    n_read = read(ifd, tbuf, dp.bs);
    if (n_read < 0) {
      err = -errno;
      break;
    }

    if (n_read == 0) {
      break;
    }

    ....

    dp.count --;
  } while (dp.count != 0);
  ....
}

Предупреждение PVS-Studio: V715 The 'while' operator has empty body. Suspicious pattern detected: 'while (expr) {...} while (dp.skip != 0) ;'. dd.c 225

Хм. Действительно странный цикл. Выражение while (dp.skip != 0) написано два раза, причем один раз прямо над циклом, а второй – точно под ним. По факту, сейчас это два различных цикла: один содержит в себе выражения в фигурных скобках, а второй – пустой. При этом второй цикл не будет выполнен никогда.

Ниже идет цикл do...while с похожим условием, что наводит меня на мысль: изначально странный цикл подразумевался как do...while, но что-то пошло не так. Я думаю, что этот участок кода с большой вероятностью содержит в себе логическую ошибку.

Утечки памяти

Да, не обошлось и без них.

int krename(const char *oldpath, const char *newpath) {
  
  char *newpatharg, *oldpatharg;

  ....

  oldpatharg =
    calloc(strlen(oldpath) + diritemlen + 2, sizeof(char));
  newpatharg =
    calloc(strlen(newpath) + diritemlen + 2, sizeof(char));
  if (NULL == oldpatharg || NULL == newpatharg) {
    SET_ERRNO(ENOMEM);
    return -1;
  }

  ....
}

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

V773 The function was exited without releasing the 'newpatharg' pointer. A memory leak is possible. kfsop.c 611
V773 The function was exited without releasing the 'oldpatharg' pointer. A memory leak is possible. kfsop.c 611

Функция создает внутри себя локальные переменные newpatharg и oldpatharg. Этим указателям присваивают адреса новых участков памяти, выделенных внутри с помощью calloc. В случае, если при выделении памяти возникает проблема, calloc возвращает нулевой указатель.

Что, если получится выделить память только один блок памяти? Функция аварийно завершится, не производя при этом никакого освобождения памяти. Тот участок, который получилось выделить, останется в памяти без какой-либо возможности снова получить к нему доступ и освободить его для дальнейшего использования.

Еще один пример утечки памяти, чуть поярче:

static int block_dev_test(....) {
  int8_t *read_buf, *write_buf;
  
  ....

  read_buf = malloc(blk_sz * m_blocks);
  write_buf = malloc(blk_sz * m_blocks);

  if (read_buf == NULL || write_buf == NULL) {
    printf("Failed to allocate memory for buffer!\n");

    if (read_buf != NULL) {
      free(read_buf);
    }

    if (write_buf != NULL) {
      free(write_buf);
    }

    return -ENOMEM;
  }

  if (s_block >= blocks) {
    printf("Starting block should be less than number of blocks\n");
    return -EINVAL;            // <=
  }

  ....
}

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

V773 The function was exited without releasing the 'read_buf' pointer. A memory leak is possible. block_dev_test.c 195
V773 The function was exited without releasing the 'write_buf' pointer. A memory leak is possible. block_dev_test.c 195

Здесь программист уже проявил аккуратность и корректно обработал случай, при котором удалось выделить только один участок памяти. Обработал корректно… и буквально в следующем выражении допустил другую ошибку.

Благодаря правильно написанной проверке, мы можем быть уверены, что на момент выполнения выражения return -EINVAL; у нас точно будет выделена память как под read_buf, так и под write_buf. Таким образом, при таком возврате из функции мы будем иметь сразу две утечки.

Думаю, что получить утечку памяти на встраиваемом устройстве может быть больнее, чем на классическом ПК. В условиях, когда ресурсы жестко ограничены, надо следить за ними особенно тщательно.

Неправильное обращение с указателями

Следующий код с ошибкой достаточно лаконичен и прост:

static int scsi_write(struct block_dev *bdev, char *buffer,
                      size_t count, blkno_t blkno) {
  struct scsi_dev *sdev;
  int blksize;

  ....

  sdev = bdev->privdata;
  blksize = sdev->blk_size; // <=

  if (!sdev) {              // <=
    return -ENODEV;
  }

  ....
}

Предупреждение PVS-Studio: V595 The 'sdev' pointer was utilized before it was verified against nullptr. Check lines: 116, 118. scsi_disk.c 116

Указатель sdev разыменовывается прямо перед тем, как он будет проверен на NULL. Логично предположить, что если кто-то написал такую проверку, то этот указатель может быть нулевым. В этом случае мы имеем потенциальное разыменование нулевого указателя в строке blksize = sdev->blk_size.

Ошибка в том, что проверка расположена не там, где надо. Она должна была идти после строки "sdev = bdev->privdata;", но перед строкой "blksize = sdev->blk_size;". Тогда и потенциального обращения по нулевому адресу можно было бы избежать.

Еще две ошибки PVS-Studio обнаружил в следующем коде:

void xdrrec_create(....)
{
  char *buff;

  ....

  buff = (char *)malloc(sendsz + recvsz);
  assert(buff != NULL);

  ....

  xs->extra.rec.in_base = xs->extra.rec.in_curr = buff;
  xs->extra.rec.in_boundry 
    = xs->extra.rec.in_base + recvsz;                    // <=

  ....
  xs->extra.rec.out_base
    = xs->extra.rec.out_hdr = buff + recvsz;             // <= 
  xs->extra.rec.out_curr 
    = xs->extra.rec.out_hdr + sizeof(union xdrrec_hdr);

  ....
}

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

Указатель buf инициализируется с помощью malloc, а далее его значение используется для инициализации других указателей. Функция malloc может вернуть нулевой указатель, и это всегда нужно проверять. Казалось бы, вот assert, проверяющий buf на NULL, и всё должно работать хорошо.

Но нет! Дело в том, что assert'ы используются для отладки, и при сборке проекта в Release-конфигурации этот assert будет удалён. Получается, что при работе в Debug программа будет работать правильно, а при сборке в Release нулевой указатель «пройдет» дальше.

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

Кто-то может возразить, что отсутствие проверки указателя после malloc/realloc/calloc – это нестрашно. Мол, при первом доступе по нулевому указателю возникнет сигнал/исключение и ничего страшного не будет. На практике всё намного сложнее, и, если отсутствие проверки не кажется вам опасным, – предлагаю вам взглянуть на статью "Почему важно проверять, что вернула функция malloc".

Неправильное обращение с массивами

Следующая ошибка во многом похожа на позапрошлый пример:


int fat_read_filename(struct fat_file_info *fi,
                      void *p_scratch,
                      char *name) {
  int offt = 1;

  ....

  offt = strlen(name);
  while (name[offt - 1] == ' ' && offt > 0) { // <=
    name[--offt] = '\0';
  }
  log_debug("name(%s)", name);

  return DFS_OK;
}

Предупреждение PVS-Studio: V781 The value of the 'offt' index is checked after it was used. Perhaps there is a mistake in program logic. fat_common.c 1813

Переменная offt сначала используется внутри операции индексирования, а только потом проверяется, что её значение больше нуля. А что будет, если name окажется пустой строкой? Функция strlen() вернёт 0, дальше – громкий выстрел в ногу. Программа обратится по отрицательному индексу, что приведёт к неопределённому поведению. Произойти может что угодно, в том числе и падение программы. Непорядок!

Подозрительные условия

А куда же без них? Мы находим такие ошибки буквально в каждом проекте, который проверяем.

int index_descriptor_cloexec_set(int fd, int cloexec) {
  struct idesc_table *it;

  it = task_resource_idesc_table(task_self());
  assert(it);

  if (cloexec | FD_CLOEXEC) {
    idesc_cloexec_set(it->idesc_table[fd]);
  } else {
    idesc_cloexec_clear(it->idesc_table[fd]);
  }
  return 0;
}

Предупреждение PVS-Studio: V617 Consider inspecting the condition. The '0x0010' argument of the '|' bitwise operation contains a non-zero value. index_descriptor.c 55

Чтобы понять, в чем кроется ошибка, заглянем в определение константы FD_CLOEXEC:

#define FD_CLOEXEC 0x0010

Получается, что в выражении if (cloexec | FD_CLOEXEC) справа от побитового «или» всегда находится ненулевая константа. Результатом такой операции всегда будет ненулевое число. Таким образом, это выражение всегда будет равнозначно выражению if(true), и мы всегда будем обрабатывать только then-ветвь if-выражения.

Я подозреваю, что эта макросная константа используется для предварительной настройки операционной системы Embox, но даже в этом случае подобное всегда истинное условие выглядит странно. Возможно, здесь хотели использовать оператор &, но допустили опечатку.

Целочисленное деление

Следующая ошибка связана с одной особенностью языка Си:

#define SBSIZE    1024

static int ext2fs_format(struct block_dev *bdev, void *priv) {
  size_t dev_bsize;
  float dev_factor;

  ....

  dev_size = block_dev_size(bdev);
  dev_bsize = block_dev_block_size(bdev);
  dev_factor = SBSIZE / dev_bsize;            // <=

  ext2_dflt_sb(&sb, dev_size, dev_factor);
  ext2_dflt_gd(&sb, &gd);

  ....
}

Предупреждение PVS-Studio: V636 The '1024 / dev_bsize' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. ext2.c 777

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

Иногда программисты об этом забывают, и получаются ошибки вроде этой. Константа SBSIZE и переменная dev_bsize имеют целочисленный тип (int и size_t соответственно). Поэтому результат выражения SBSIZE / dev_bsize будет также иметь целочисленный тип.

Но подождите-ка. Переменная dev_factor имеет тип float! Очевидно, что программист рассчитывал получить дробный результат деления. В этом можно дополнительно удостовериться, если обратить внимание на дальнейшее использование этой переменной. Например, функция ext2_dflt_sb, куда dev_factor передаётся в качестве третьего параметра, имеет следующую сигнатуру:

static void ext2_dflt_sb(struct ext2sb *sb, size_t dev_size, float dev_factor);

Аналогично и в других местах, где используется переменная dev_factor: всё указывает на то, что ожидается число с плавающей точкой.

Чтобы исправить эту ошибку, достаточно лишь привести один из операндов деления к вещественному типу. Например:

dev_factor = float(SBSIZE) / dev_bsize;

Тогда и результатом деления будет дробное число.

Непроверенные входные данные

Следующая ошибка связана с использованием непроверенных данных, полученных программой извне.

int main(int argc, char **argv) {
  int ret;
  char text[SMTP_TEXT_LEN + 1];

  ....

  if (NULL == fgets(&text[0], sizeof text - 2, /* for \r\n */
      stdin)) { ret = -EIO; goto error; }
    text[strlen(&text[0]) - 1] = '\0'; /* remove \n */    // <=

  ....
}

Предупреждение PVS-Studio: V1010 Unchecked tainted data is used in index: 'strlen(& text[0])'. sendmail.c 102

Для начала рассмотрим, что именно возвращает функция fgets. В случае успешного считывания строки, функция возвращает указатель на эту строку. В случае, если считывание встречает end-of-file до того, как будет считан хоть один элемент, или если произошла ошибка ввода, функция fgets возвращает NULL.

Таким образом, выражение NULL == fgets(....) проверяет, корректный ли получен ввод. Но есть один нюанс. Если в качестве первого символа для считывания передать нуль-терминал (это можно сделать, например, нажатием сочетания Ctrl + 2 в Legacy-режиме командной строки Windows), то функция fgets считает его, не вернув при этом NULL. При этом строка, в которую производится запись, будет иметь лишь один элемент – '\0'.

Что произойдет дальше? Выражение strlen(&text[0]) вернет 0. В результате мы получим обращение по отрицательному индексу:

text[ 0 - 1 ] = '\0';

В результате мы можем «опрокинуть» программу, просто передав на вход символ окончания строки. Это некруто, и потенциально может использоваться для атак на системы, использующие Embox.

Мой коллега, который разрабатывал это диагностическое правило, даже сделал запись с примером такой атаки на проект NcFTP:

Рекомендую посмотреть, если все еще не верите в такую возможность :)

Также анализатор нашел еще два места с такой же ошибкой:

V1010 Unchecked tainted data is used in index: 'strlen(& from[0])'. sendmail.c 55
V1010 Unchecked tainted data is used in index: 'strlen(& to[0])'. sendmail.c 65

MISRA

MISRA – это набор рекомендаций и правил по написанию безопасного C и C++ кода для встраиваемых систем высокой ответственности. В каком-то смысле, это методичка, следование которой позволит вам избавиться не только от так называемых «code smells», но и обезопасить свою программу от уязвимостей.

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

PVS-Studio имеет обширный набор диагностических правил, позволяющих проверить ваш код на соответствие стандартам MISRA C и MISRA С++. По умолчанию режим с этими диагностиками выключен, но раз уж мы с вами ищем ошибки в проекте для встраиваемых систем, то обойтись без MISRA я просто не смог.

Вот, что у меня получилось найти:

/* find and read symlink file */
static int ext2_read_symlink(struct nas *nas,
                             uint32_t parent_inumber,
                             const char **cp) {
  char namebuf[MAXPATHLEN + 1];

  ....

  *cp = namebuf;              // <=
  if (*namebuf != '/') {
    inumber = parent_inumber;
  } else {
    inumber = (uint32_t) EXT2_ROOTINO;
  }
  rc = ext2_read_inode(nas, inumber);

  return rc;
}

Предупреждение PVS-Studio: V2548 [MISRA C 18.6] Address of the local array 'namebuf' should not be stored outside the scope of this array. ext2.c 298

Анализатор обнаружил подозрительное присвоение, которое потенциально может привести к неопределённому поведению.

Давайте рассмотрим код внимательнее. Здесь namebuf – массив, созданный в локальной области видимости функции, а указатель cp передаётся в функцию по указателю.

Согласно синтаксису языка Си, имя массива является указателем на первый элемент в области памяти, в которой хранится массив. Получается, что выражение *cp = namebuf присвоит адрес массива namebuf в переменную, на которую указывает cp. Так как cp передан в функцию по указателю, изменение значения, на которое он указывает, отразится и в том месте, где функция была вызвана.

Получается, что после того, как функция ext2_read_symlink завершит свою работу, её третий параметр будет указывать на область, которую когда-то занял массив namebuf.

Только есть одно «но»: так как namebuf – массив, зарезервированный на стеке, при выходе из функции он будет удалён. Таким образом, указатель, существующий вне функции, будет указывать на освобожденный участок памяти.

Что будет находиться по тому адресу? Предсказать нельзя. Возможно, что какое-то время содержимое массива еще продолжит находиться в памяти, а возможно, что программа сразу затрёт эту область чем-то другим. В общем случае обращение по такому адресу вернёт неопределённое значение, и использование такого значения является грубой ошибкой.

Анализатор также нашел еще одну ошибку с таким же предупреждением:

V2548 [MISRA C 18.6] Address of the local variable 'dst_haddr' should not be stored outside the scope of this variable. net_tx.c 82

Заключение

Мне понравилось работать с проектом Embox. Несмотря на то, что я выписал в статью не все найденные ошибки, суммарное количество предупреждений было относительно небольшим, и в целом код проекта написан качественно. Поэтому выражаю свою благодарность разработчикам, а также тем, кто вносил свой вклад в проект от лица сообщества. Вы молодцы!

Пользуясь случаем, передаю разработчикам привет из Тулы. Буду верить, что в Петербурге сейчас не сильно холодно :)

На этом моя статья подходит к концу. Надеюсь, вам понравилось её читать, и вы нашли для себя что-нибудь новенькое.

Если вас заинтересовал PVS-Studio и вам хочется самостоятельно проверить с помощью него какой-нибудь проект – скачайте и попробуйте. Это займёт не больше 15 минут.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. About embedded again: searching for bugs in the Embox project.

И снова про embedded: ищем баги в проекте Embox