Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален

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

Введение

Прошёл год с момента написания большой статьи про ошибки в операционной системе Tizen, и захотелось вновь провести столь же интересное исследование какой-то операционной системы. Выбор пал на Android.

Код операционной системы Android является качественным, хорошо протестированным. При разработке используется как минимум статический анализатор кода Coverity, свидетельством чему являются комментарии вида:

/* Coverity: [FALSE-POSITIVE error] intended fall through */
/* Missing break statement between cases in switch statement */
/* fall through */

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

Думаю, уже только по объему статьи читатель понимает, что анализатор PVS-Studio отлично справился с поставленной задачей и нашел массу дефектов в коде этой операционной системы.

Common Weakness Enumeration

В статье вы встретите отсылки к Common Weakness Enumeration (CWE). Поясним причину отсылки к этому списку и почему это важно с точки зрения безопасности.

Часто причиной уязвимостей в программах является не какое-то хитрое стечение обстоятельств, а банальная программная ошибка. Здесь будет уместно привести вот эту цитату с сайта prqa.com:

The National Institute of Standards and Technology (NIST) reports that 64% of software vulnerabilities stem from programming errors and not a lack of security features.

Вы можете ознакомиться в статье "Как PVS-Studio может помочь в поиске уязвимостей?" с некоторыми примерами простых ошибок, которые приводили к уязвимостям в таких проектах, как MySQL, iOS, NAS, illumos-gate.

Соответственно, многие уязвимости можно устранить, вовремя обнаружив обыкновенные ошибки и исправив их. И вот здесь на сцену выходит Common Weakness Enumeration.

Ошибки бывают разные, и не все ошибки опасны с точки зрения безопасности. Те ошибки, которые потенциально могут стать причиной уязвимости, собраны в Common Weakness Enumeration. Этот список пополняется, и наверняка существуют ошибки, которые могут приводить к уязвимостям, но они ещё не попали в этот список.

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

Схематически взаимосвязь между PVS-Studio, ошибками, CWE и CVE показана на рисунке:

Часть ошибок классифицируется как CWE. Многие из этих ошибок может обнаружить PVS-Studio, тем самым не дав некоторым из этих дефектов превратиться в уязвимости (CVE).

Можно сказать, что PVS-Studio выявляет многие потенциальные уязвимости до того, как они причинили вред. Таким образом, PVS-Studio является средством статического тестирования защищённости приложений (SAST).

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

Проверка Android

Для анализа проекта я использовал анализатор PVS-Studio версии 6.24. Анализатор на данный момент поддерживает следующие языки и компиляторы:

• Windows. Visual Studio 2010-2017 C, C++, C++/CLI, C++/CX (WinRT), C#
• Windows. IAR Embedded Workbench, C/C++ Compiler for ARM 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. Clang C, C++
• Linux/macOS. GCC C, C++
• Windows. MinGW C, C++

Примечание. Возможно, некоторые наши читатели пропустили новость о том, что мы поддержали работу в среде macOS, и им будет интересна вот эта публикация: "Релиз PVS-Studio для macOS: 64 weaknesses в Apple XNU Kernel".

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

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

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

Теперь давайте посмотрим, какие ошибки и потенциальные уязвимости мне удалось найти. Надеюсь, вам понравится то, что умеет находить статический анализатор кода PVS-Studio. Приятного чтения.

Бессмысленные сравнения

Анализатор считает выражения аномальными, если они всегда истинны или ложны. Такие предупреждения, согласно Common Weakness Enumeration, классифицируются как:

• CWE-570: Expression is Always False
• CWE-571: Expression is Always True

Анализатор выдаёт много таких предупреждений, и, к сожалению, большинство из них для кода Android являются ложными. При этом анализатор не виноват. Просто код так написан. Продемонстрирую это на простом примере.

#if GENERIC_TARGET
const char alternative_config_path[] = "/data/nfc/";
#else
const char alternative_config_path[] = "";
#endif

CNxpNfcConfig& CNxpNfcConfig::GetInstance() {
  ....
  if (alternative_config_path[0] != '') {
  ....
}

Здесь анализатор выдаёт предупреждение: V547 CWE-570 Expression 'alternative_config_path[0] != ''' is always false. phNxpConfig.cpp 401

Дело в том, что макрос GENERIC_TARGET не определён, и с точки зрения анализатора код выглядит следующим образом:

const char alternative_config_path[] = "";
....
if (alternative_config_path[0] != '') {

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

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

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

Начать хочется с классической ситуации, когда неверно реализована функция сравнения двух объектов. Почему классической? Это типовой паттерн ошибки, который постоянно встречается нам в разнообразных проектах. Скорее всего, есть три причины его возникновения:

1. Функции сравнения просты и пишутся «на автомате» и с использованием технологии Copy-Paste. Человек при написании подобного кода невнимателен и часто допускает опечатки.
2. Обычно для таких функций не выполняется code-review, так как лень смотреть простые и скучные функции.
3. Для таких функций обычно не делают юнит-тесты. Потому что лень. Вдобавок, функции просты, и программисты не думают, что в них возможны ошибки.

Более подробно эти мысли и примеры изложены в статье "Зло живёт в функциях сравнения".

static inline bool isAudioPlaybackRateEqual(
  const AudioPlaybackRate &pr1,
  const AudioPlaybackRate &pr2)
{
  return fabs(pr1.mSpeed - pr2.mSpeed) <
           AUDIO_TIMESTRETCH_SPEED_MIN_DELTA &&
         fabs(pr1.mPitch - pr2.mPitch) <
           AUDIO_TIMESTRETCH_PITCH_MIN_DELTA &&
         pr2.mStretchMode == pr2.mStretchMode &&
         pr2.mFallbackMode == pr2.mFallbackMode;
}

Итак, перед нами классическая функция сравнения двух объектов типа AudioPlaybackRate. И, как я думаю, читатель догадывается, что она неправильная. Анализатор PVS-Studio замечает здесь сразу две опечатки:

• V501 CWE-571 There are identical sub-expressions to the left and to the right of the '==' operator: pr2.mStretchMode == pr2.mStretchMode AudioResamplerPublic.h 107
• V501 CWE-571 There are identical sub-expressions to the left and to the right of the '==' operator: pr2.mFallbackMode == pr2.mFallbackMode AudioResamplerPublic.h 108

Поле pr2.mStretchMode и поле pr2.mFallbackMode сравниваются сами с собой. Получается, что функция сравнивает объекты недостаточно точно.

Следующее бессмысленное сравнение живёт в функции, сохраняющей информацию об отпечатке пальца в файл.

static void saveFingerprint(worker_thread_t* listener, int idx) {
  ....
  int ns = fwrite(&listener->secureid[idx],
                  sizeof(uint64_t), 1, fp);
  ....
  int nf = fwrite(&listener->fingerid[idx],
                  sizeof(uint64_t), 1, fp);

  if (ns != 1 || ns !=1)                               // <=
    ALOGW("Corrupt emulator fingerprints storage; "
          "could not save fingerprints");

  fclose(fp);
  return;
}

Аномалия выявляется в этом коде сразу двумя диагностиками:

• V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator: ns != 1 || ns != 1 fingerprint.c 126
• V560 CWE-570 A part of conditional expression is always false: ns != 1. fingerprint.c 126

Нет обработки ситуации, когда второй вызов функции fwrite не сможет записать данные в файл. Другими словами, не проверяется значение переменной nf. Правильная проверка должна выглядеть так:

if (ns != 1 || nf != 1)

Перейдем к следующей ошибке, связанной с использованием оператора &.

#define O_RDONLY 00000000
#define O_WRONLY 00000001
#define O_RDWR   00000002

static ssize_t verity_read(fec_handle *f, ....)
{
  ....
  /* if we are in read-only mode and expect to read a zero
     block, skip reading and just return zeros */
  if (f->mode & O_RDONLY && expect_zeros) {
      memset(data, 0, FEC_BLOCKSIZE);
      goto valid;
  }
  ....
}

Предупреждение PVS-Studio: V560 CWE-570 A part of conditional expression is always false: f->mode & 00000000. fec_read.cpp 322

Обратите внимание, что константа O_RDONLY равна нулю. Это делает выражение f->mode & O_RDONLY бессмысленным, так как оно всегда равно 0. Получается, что условие оператора if никогда не выполняется, и statement-true представляет из себя мёртвый код.

Правильная проверка должна быть такой:

if (f->mode == O_RDONLY && expect_zeros) {

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

enum {
  ....
  CHANGE_DISPLAY_INFO = 1 << 2,
  ....
};

void RotaryEncoderInputMapper::configure(nsecs_t when,
        const InputReaderConfiguration* config, uint32_t changes) {
  ....
  if (!changes ||
      (InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {
  ....
}

Предупреждение PVS-Studio: V768 CWE-571 The enumeration constant 'CHANGE_DISPLAY_INFO' is used as a variable of a Boolean-type. InputReader.cpp 3016

Условие всегда истинно, так как операнд InputReaderConfiguration::CHANGE_DISPLAY_INFO представляет собой константу, равную 4.

Если посмотреть, как написан код по соседству, то становится ясно, что на самом деле условие должно быть таким:

if (!changes ||
    (changes & InputReaderConfiguration::CHANGE_DISPLAY_INFO)) {

Следующее сравнение, не имеющее смысла, я встретил в операторе цикла.

void parse_printerAttributes(....) {
  ....
  ipp_t *collection = ippGetCollection(attrptr, i);

  for (j = 0, attrptr = ippFirstAttribute(collection);
      (j < 4) && (attrptr != NULL);
      attrptr = ippNextAttribute(collection))
  {
    if (strcmp("....", ippGetName(attrptr)) == 0) {
      ....TopMargin = ippGetInteger(attrptr, 0);
    } else if (strcmp("....", ippGetName(attrptr)) == 0) {
      ....BottomMargin = ippGetInteger(attrptr, 0);
    } else if (strcmp("....", ippGetName(attrptr)) == 0) {
      ....LeftMargin = ippGetInteger(attrptr, 0);
    } else if (strcmp("....", ippGetName(attrptr)) == 0) {
      ....RightMargin = ippGetInteger(attrptr, 0);
    }
  }
  ....
}

Предупреждение PVS-Studio: V560 CWE-571 A part of conditional expression is always true: (j < 4). ipphelper.c 926

Обратите внимание, что значение переменной j нигде не инкрементируется. Это значит, что подвыражение (j < 4) всегда истинно.

Наибольшее количество полезных срабатываний анализатора PVS-Studio, касающихся всегда истинных/ложных условий, относится к коду, где проверяется результат создания объектов с помощью оператора new. Другими словами, анализатор выявляет следующий паттерн кода:

T *p = new T;
if (p == nullptr)
  return ERROR;

Такие проверки бессмысленны. Если не удалось выделить память под объект, то генерируется исключение типа std::bad_alloc, и до проверки значения указателя дело просто не дойдёт.

Примечание. Оператор new может вернуть nullptr, если написать new (std::nothrow) T. Однако это не относится к обсуждаемым ошибкам. Анализатор PVS-Studio учитывает (std::nothrow) и не выдаёт предупреждение, если объект создаётся таким способом.

Может показаться, что подобные ошибки безобидны. Ну, подумаешь, лишняя проверка, которая никогда не срабатывает. Всё равно ведь сгенерируется исключение, которое будет где-то обработано. К сожалению, некоторые разработчики размещают в statement-true оператора if действия, освобождающие ресурсы, и т.д. Так как этот код не выполняется, то это может приводить к утечкам памяти и другим ошибкам.

Рассмотрим один из таких случаев, замеченных мною в коде Android.

int parse_apk(const char *path, const char *target_package_name)
{
  ....
  FileMap *dataMap = zip->createEntryFileMap(entry);
  if (dataMap == NULL) {
    ALOGW("%s: failed to create FileMapn", __FUNCTION__);
    return -1;
  }
  char *buf = new char[uncompLen];
  if (NULL == buf) {
    ALOGW("%s: failed to allocate %" PRIu32 " byten",
          __FUNCTION__, uncompLen);
    delete dataMap;
    return -1;
  }
  ....
}

Предупреждение PVS-Studio: V668 CWE-570 There is no sense in testing the 'buf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. scan.cpp 213

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

delete dataMap;

Этот код никогда не получит управление. Это мёртвый код. Если возникнет исключение, то произойдёт утечка памяти.

Писать подобный код принципиально неправильно. Для таких случаев придумали умные указатели.

Всего анализатор PVS-Studio обнаружил в коде Android 176 мест, где выполняется проверка указателя после создания объектов с помощью new. Я не стал разбираться, насколько каждое из этих мест опасно, и, конечно, я не стану загромождать статью всеми этими предупреждениями. Желающие могут посмотреть другие предупреждения в файле Android_V668.txt.

Разыменовывание нулевого указателя

Разыменовывание нулевого указателя приводит к неопределённому поведению программы, поэтому такие места полезно найти и исправить. В зависимости от ситуации анализатор PVS-Studio может классифицировать данные ошибки согласно Common Weakness Enumeration следующим образом:

• CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer
• CWE-476: NULL Pointer Dereference
• CWE-628: Function Call with Incorrectly Specified Arguments
• CWE-690: Unchecked Return Value to NULL Pointer Dereference

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

bool parseEffect(....) {
  ....
  if (xmlProxyLib == nullptr) {
    ALOGE("effectProxy must contain a <%s>: %s",
          tag, dump(*xmlProxyLib));
    return false;
  }
  ....
}

Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'xmlProxyLib' might take place. EffectsConfig.cpp 205

Если указатель xmlProxyLib равен nullptr, то программист выводит отладочное сообщение, для чего требуется разыменовать этот самый указатель. Упс…

Теперь рассмотрим более интересный вариант ошибки.

static void soinfo_unload_impl(soinfo* root) {
  ....
  soinfo* needed = find_library(si->get_primary_namespace(),
                library_name, RTLD_NOLOAD, nullptr, nullptr);

  if (needed != nullptr) {                                // <=
    PRINT("warning: couldn't find %s needed by %s on unload.",
      library_name, si->get_realpath());
    return;
  } else if (local_unload_list.contains(needed)) {
    return;
  } else if (needed->is_linked() &&                       // <=
             needed->get_local_group_root() != root) {
    external_unload_list.push_back(needed);
  } else {
    unload_list.push_front(needed);
  }
  ....
}

Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'needed' might take place. linker.cpp 1847

Если указатель needed != nullptr, то производится вывод предупреждения, что является очень подозрительным поведением программы. Окончательно становится ясно, что код содержит ошибку, если посмотреть ниже и увидеть, что при needed == nullptr произойдёт разыменование нулевого указателя в выражении needed->is_linked().

Скорее всего, здесь просто перепутаны операторы != и ==. Если провести замену, то код функции обретает смысл, и ошибка исчезает.

Основная масса предупреждений про потенциальное разыменование нулевого указателя относится к ситуации вида:

T *p = (T *) malloc (N);
*p = x;

Такие функции, как malloc, strdup и так далее могут вернуть NULL, если невозможно выделить память. Поэтому нельзя разыменовывать указатели, которые вернули эти функции без предварительной проверки указателя.

Подобных ошибок много, поэтому я приведу только два простых фрагмента кода: первый с malloc и второй с strdup.

DownmixerBufferProvider::DownmixerBufferProvider(....)
{
  ....
  effect_param_t * const param = (effect_param_t *)
                                 malloc(downmixParamSize);
  param->psize = sizeof(downmix_params_t);
  ....
}

Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'param'. Check lines: 245, 244. BufferProviders.cpp 245

static char* descriptorClassToDot(const char* str)
{
  ....
  newStr = strdup(lastSlash);
  newStr[strlen(lastSlash)-1] = '';
  ....
}

Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'newStr'. Check lines: 203, 202. DexDump.cpp 203

Кто-то может сказать, что это несущественные ошибки. Если памяти недостаточно, то программа просто аварийно завершится при разыменовании нулевого указателя, и это нормально. Раз памяти нет, то и нечего пытаться как-то обработать эту ситуацию.

Такой человек неправ. Указатели нужно обязательно проверять! Я подробно разбирал эту тему в статье "Почему важно проверять, что вернула функция malloc". Очень рекомендую ознакомиться с ней всех, кто её ещё не читал.

Если совсем кратко, то опасность состоит в том, что запись в память может произойти не обязательно рядом с нулевым адресом. Можно записать данные куда-то очень далеко в страницу памяти, не защищенную от записи, и тем самым вызвать трудноуловимую ошибку или вообще эту ошибку можно будет начать использовать как уязвимость. Давайте посмотрим, что я имею в виду, на примере функции check_size.

int check_size(radio_metadata_buffer_t **metadata_ptr,
               const uint32_t size_int)
{
  ....
  metadata = realloc(metadata,
                     new_size_int * sizeof(uint32_t));
  memmove(
   (uint32_t *)metadata + new_size_int - (metadata->count + 1),
   (uint32_t *)metadata + metadata->size_int -
                           (metadata->count + 1),
   (metadata->count + 1) * sizeof(uint32_t));
  ....
}

Предупреждение PVS-Studio: V769 CWE-119 The '(uint32_t *) metadata' pointer in the '(uint32_t *) metadata + new_size_int' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 91, 89. radio_metadata.c 91

Я не разбирался в логике работы функции, но это и не нужно. Главное, что создаётся новый буфер и туда копируются данные. Если функция realloc вернёт NULL, то данные будут скопированы по адресу ((uint32_t *)NULL + metadata->size_int — (metadata->count + 1)).

Если значение metadata->size_int большое, то последствия будут прискорбны. Получается, что данные записываются в случайный участок памяти.

Кстати, есть ещё одна разновидность разыменования нулевого указателя, которую анализатор PVS-Studio классифицирует не как CWE-690, а как CWE-628 (неправильный аргумент).

static void
parse_tcp_ports(const char *portstring, uint16_t *ports)
{
  char *buffer;
  char *cp;

  buffer = strdup(portstring);
  if ((cp = strchr(buffer, ':')) == NULL)
  ....
}

Предупреждение PVS-Studio: V575 CWE-628 The potential null pointer is passed into 'strchr' function. Inspect the first argument. Check lines: 47, 46. libxt_tcp.c 47

Дело в том, что разыменование указателя произойдёт при вызове функции strchr. Поэтому анализатор интерпретирует эту ситуацию как передачу в функцию некорректного значения.

Остальные 194 предупреждения этого типа я привожу списком в файле Android_V522_V575.txt.

Кстати, особую пикантность всем этим ошибкам придают рассмотренные ранее предупреждения про проверку указателя после вызова оператора new. Получается, что есть 195 вызовов функций malloc/realloc/strdup и так далее, когда указатель не проверятся. Зато есть 176 мест, где указатель проверяется после вызова new. Согласитесь, странноватый подход!

Напоследок нам осталось рассмотреть предупреждения V595 и V1004, которые также связаны с использованием нулевых указателей.

V595 выявляет ситуации, когда указатель разыменовывается, а затем проверяется. Синтетический пример:

p->foo();
if (!p) Error();

V1004 выявляет обратные ситуации, когда сначала указатель проверяли, а потом забыли это сделать. Синтетический пример:

if (p) p->foo();
p->doo();

Давайте посмотрим на несколько фрагментов кода Android, где нашлись ошибки этого типа. Специально пояснять их особенности не потребуется.

PV_STATUS RC_UpdateBuffer(VideoEncData *video,
                          Int currLayer, Int num_skip)
{
  rateControl *rc  = video->rc[currLayer];
  MultiPass   *pMP = video->pMP[currLayer];

  if (video == NULL || rc == NULL || pMP == NULL)
    return PV_FAIL;
  ....
}

Предупреждение PVS-Studio: V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 385, 388. rate_control.cpp 385

static void resampler_reset(struct resampler_itfe *resampler)
{
  struct resampler *rsmp = (struct resampler *)resampler;

  rsmp->frames_in = 0;
  rsmp->frames_rq = 0;

  if (rsmp != NULL && rsmp->speex_resampler != NULL) {
    speex_resampler_reset_mem(rsmp->speex_resampler);
  }
}

Предупреждение PVS-Studio: V595 CWE-476 The 'rsmp' pointer was utilized before it was verified against nullptr. Check lines: 54, 57. resampler.c 54

void bta_gattc_disc_cmpl(tBTA_GATTC_CLCB* p_clcb,
                         UNUSED_ATTR tBTA_GATTC_DATA* p_data) {
  ....
  if (p_clcb->status != GATT_SUCCESS) {
    if (p_clcb->p_srcb) {
      std::vector<tBTA_GATTC_SERVICE>().swap(
        p_clcb->p_srcb->srvc_cache);
    }
    bta_gattc_cache_reset(p_clcb->p_srcb->server_bda);
  }  ....
}

Предупреждение PVS-Studio: V1004 CWE-476 The 'p_clcb->p_srcb' pointer was used unsafely after it was verified against nullptr. Check lines: 695, 701. bta_gattc_act.cc 701

Рассматривать другие предупреждения этого типа неинтересно. Среди них есть как ошибки, так и ложные предупреждения, которые возникают из-за плохо или сложно написанного кода.

Я выписал десяток полезных предупреждений:

• V1004 CWE-476 The 'ain' pointer was used unsafely after it was verified against nullptr. Check lines: 101, 105. rsCpuIntrinsicBLAS.cpp 105
• V595 CWE-476 The 'outError' pointer was utilized before it was verified against nullptr. Check lines: 437, 450. Command.cpp 437
• V595 CWE-476 The 'out_last_reference' pointer was utilized before it was verified against nullptr. Check lines: 432, 436. AssetManager2.cpp 432
• V595 CWE-476 The 'set' pointer was utilized before it was verified against nullptr. Check lines: 4524, 4529. ResourceTypes.cpp 4524
• V595 CWE-476 The 'reply' pointer was utilized before it was verified against nullptr. Check lines: 126, 133. Binder.cpp 126
• V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 532, 540. rate_control.cpp 532
• V595 CWE-476 The 'video' pointer was utilized before it was verified against nullptr. Check lines: 702, 711. rate_control.cpp 702
• V595 CWE-476 The 'pInfo' pointer was utilized before it was verified against nullptr. Check lines: 251, 254. ResolveInfo.cpp 251
• V595 CWE-476 The 'address' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. DeviceHalHidl.cpp 53
• V595 CWE-476 The 'halAddress' pointer was utilized before it was verified against nullptr. Check lines: 55, 82. DeviceHalHidl.cpp 55

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

Хочу только обратить внимание моих новых читателей на ошибки вот такого вида:

NJ_EXTERN NJ_INT16 njx_search_word(NJ_CLASS *iwnn, ....) {
  ....
  NJ_PREVIOUS_SELECTION_INFO *prev_info =
      &(iwnn->previous_selection);

  if (iwnn == NULL) {
    return NJ_SET_ERR_VAL(NJ_FUNC_NJ_SEARCH_WORD,
                          NJ_ERR_PARAM_ENV_NULL);
  }
  ....
}

Предупреждение PVS-Studio: V595 CWE-476 The 'iwnn' pointer was utilized before it was verified against nullptr. Check lines: 686, 689. ndapi.c 686

Некоторые считают, что ошибки здесь нет, так как «нет настоящего разыменования указателя». Просто вычисляется адрес несуществующей переменной. Далее, если указатель iwnn нулевой, то произойдёт выход из функции. Следовательно, ничего плохого не случилось, что мы ранее некорректно вычисляли адрес члена класса.

Нет, так рассуждать нельзя. Этот код приводит к неопределённому поведению, и поэтому так писать нельзя. Неопределённое поведение может проявить себя, например, следующим образом:

1. Компилятор видит, что указатель разыменовывается: iwnn->previous_selection
2. Разыменовывать нулевой указатель нельзя, ведь это undefined behavior
3. Компилятор делает вывод, что указатель iwnn всегда ненулевой
4. Компилятор удаляет лишнюю проверку: if (iwnn == NULL)
5. Всё, теперь при выполнении программы проверка на нулевой указатель не выполняется, и начинается работа с некорректным указателем на член класса

Более подробно эта тема описана в моей статье "Разыменовывание нулевого указателя приводит к неопределённому поведению".

Приватные данные не затираются в памяти

Рассмотрим серьезный вид потенциальной уязвимости, которая классифицируется согласно Common Weakness Enumeration как CWE-14: Compiler Removal of Code to Clear Buffers.

Если кратко, то суть в следующем: компилятор вправе удалить вызов функции memset, если после этого буфер больше не используется.

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

• Роман Фомичев. Безопасная очистка приватных данных.
• Common Weakness Enumeration. CWE-14.
• Описание диагностики V597.

В общем, всё серьезно. Есть ли такие ошибки в Android? Конечно, есть. Они вообще много где есть: proof :).

Вернёмся к коду Android и рассмотрим начало и конец функции FwdLockGlue_InitializeRoundKeys, так как середина нам неинтересна.

static void FwdLockGlue_InitializeRoundKeys() {
  unsigned char keyEncryptionKey[KEY_SIZE];
  ....
  memset(keyEncryptionKey, 0, KEY_SIZE); // Zero out key data.
}

Предупреждение PVS-Studio: V597 CWE-14 The compiler could delete the 'memset' function call, which is used to flush 'keyEncryptionKey' buffer. The memset_s() function should be used to erase the private data. FwdLockGlue.c 102

Массив keyEncryptionKey создаётся на стеке и хранит приватную информацию. В конце функции этот массив хотят заполнить нулями, чтобы он случайно не попал куда не следует. Как информация может попасть туда, куда не следует, расскажет статья "Перезаписывать память — зачем?".

Для заполнения массива с приватной информацией нулями используется функция memset. Комментарий «Zero out key data» подтверждает, что мы всё понимаем правильно.

Беда в том, что с очень большой вероятностью компилятор при сборке release-версии удалит вызов функции memset. Раз буфер после вызова memset не используется, то и сам вызов функции memset с точки зрения компилятора является лишним.

Ещё 10 предупреждений я выписал в файл Android_V597.txt.

Нашлась ещё одна ошибка, где память не затирается, хотя в этом случае функция memset ни при чём.

void SHA1Transform(uint32_t state[5], const uint8_t buffer[64])
{
  uint32_t a, b, c, d, e;
  ....
  /* Wipe variables */
  a = b = c = d = e = 0;
}

Предупреждение PVS-Studio: V1001 CWE-563 The 'a' variable is assigned but is not used until the end of the function. sha1.c 213

PVS-Studio выявил аномалию, связанную с тем, что после присваивания переменным значений, они больше не используются. Анализатор классифицировал этот дефект как CWE-563: Assignment to Variable without Use. И, формально, он прав, хотя, на самом деле, здесь мы опять имеем дело с CWE-14. Компилятор выбросит эти присваивания, так как с точки зрения языка C и C++ они лишние. В результате в стеке останутся прежние значения переменных a, b, c, d и e, хранящих приватные данные.

Unspecified/implementation-defined behavior

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

typedef int32_t  GGLfixed;

GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
{
  if ((d>>24) && ((d>>24)+1)) {
    n >>= 8;
    d >>= 8;
  }
  return gglMulx(n, gglRecip(d));
}

Предупреждение PVS-Studio: V793 It is odd that the result of the '(d >> 24) + 1' statement is a part of the condition. Perhaps, this statement should have been compared with something else. fixed.cpp 75

Программист хотел проверить, что 8 старших бит переменной d содержат единицы, но при этом не все биты сразу. Другими словами, программист хотел проверить, что в старшем байте находится любое значение, отличное от 0x00 и 0xFF.

Подошёл он к решению этой задачи излишне творчески. Для начала он проверил, что старшие биты ненулевые, написав выражение (d>>24). К этому выражению есть претензии, но интереснее разобрать правую часть выражения: ((d>>24)+1). Программист сдвигает старшие восемь бит в младший байт. При этом он рассчитывает, что самый старший знаковый бит дублируется во всех остальных битах. Т.е. если переменная d равна 0b11111111'00000000'00000000'00000000, то после сдвига получится значение 0b11111111'11111111'11111111'11111111. Прибавив 1 к значению 0xFFFFFFFF типа int, программист планирует получить 0. То есть: -1+1=0. Таким образом, выражением ((d>>24)+1) он проверяет, что не все старшие восемь бит равны 1. Я понимаю, что это довольно сложно, поэтому прошу не торопиться и попробовать разобраться, как и что работает :).

Теперь давайте разберём, что не так с этим кодом.

При сдвиге старший знаковый бит вовсе не обязательно «размазывается». Вот что написано про это в стандарте: «The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined».

Нам важно самое последнее предложение. Итак, мы повстречали поведение, определяемое реализацией (implementation-defined behavior). Как будет работать этот код, зависит от архитектуры микропроцессора и реализации компилятора. После сдвига в старших битах вполне могут оказаться нули, и тогда выражение ((d>>24)+1) всегда будет отлично от 0, т.е. будет всегда истинным значением.

Отсюда вывод: не надо зря мудрить. Код станет надёжней и понятней, если написать, например, так:

GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
{
  uint32_t hibits = static_cast<uint32_t>(d) >> 24;
  if (hibits != 0x00 && hibits != 0xFF) {
    n >>= 8;
    d >>= 8;
  }
  return gglMulx(n, gglRecip(d));
}

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

Вы заслужили чашечку кофе или чая. Отвлекитесь, и продолжим: нас ждёт интересный случай неуточненного поведения.

На собеседовании в качестве одного из первых вопросов соискателю я задаю следующий: «Что напечатает функция printf и почему?»

int i = 5;
printf("%d,%d", ++i, ++i)

Правильный ответ: это неуточненное поведение. Порядок вычисления фактических аргументов при вызове функции не определён. Изредка я даже демонстрирую, что этот код, собранный с помощью Visual C++, выводит на экран «7,6», чем ставлю в полный тупик новичков, слабых знанием и духом :).

Может показаться, что это надуманная проблема. Но нет, подобный код можно встретить и в серьезных приложениях, например, в коде Android.

bool ComposerClient::CommandReader::parseSetLayerCursorPosition(
  uint16_t length)
{
  if (length != CommandWriterBase::kSetLayerCursorPositionLength) {
    return false;
  }

  auto err =
    mHal.setLayerCursorPosition(mDisplay, mLayer,
                                readSigned(), readSigned());
  if (err != Error::NONE) {
    mWriter.setError(getCommandLoc(), err);
  }

  return true;
}

Предупреждение PVS-Studio: V681 CWE-758 The language standard does not define an order in which the 'readSigned' functions will be called during evaluation of arguments. ComposerClient.cpp 836

Нам интереса вот эта строка кода:

mHal.setLayerCursorPosition(...., readSigned(), readSigned());

С помощью вызова функции readSigned читаются два значения. Но вот в какой последовательности будут прочитаны значения, предсказать невозможно. Это классический случай Unspecified Behavior.

Польза от использования статического анализатора кода

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

const std::map<std::string, int32_t> kBootReasonMap = {
    ....
    {"watchdog_sdi_apps_reset", 106},
    {"smpl", 107},
    {"oem_modem_failed_to_powerup", 108},
    {"reboot_normal", 109},
    {"oem_lpass_cfg", 110},                           // <=
    {"oem_xpu_ns_error", 111},                        // <= 
    {"power_key_press", 112},
    {"hardware_reset", 113},
    {"reboot_by_powerkey", 114},
    {"reboot_verity", 115},
    {"oem_rpm_undef_error", 116},
    {"oem_crash_on_the_lk", 117},  
    {"oem_rpm_reset", 118},
    {"oem_lpass_cfg", 119},                           // <=
    {"oem_xpu_ns_error", 120},                        // <=
    {"factory_cable", 121},
    {"oem_ar6320_failed_to_powerup", 122},
    {"watchdog_rpm_bite", 123},
    {"power_on_cable", 124},
    {"reboot_unknown", 125},
    ....
};

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

• V766 CWE-462 An item with the same key '«oem_lpass_cfg»' has already been added. bootstat.cpp 264
• V766 CWE-462 An item with the same key '«oem_xpu_ns_error»' has already been added. bootstat.cpp 265

В отсортированный ассоциативный контейнер (std::map) вставляются разные значения с одинаковыми ключами. С точки зрения Common Weakness Enumeration — это CWE-462: Duplicate Key in Associative List.

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

Рассмотрим ещё один фрагмент кода, который очень сложен для восприятия, так как однотипен и неинтересен.

MtpResponseCode MyMtpDatabase::getDevicePropertyValue(....) {
  ....
  switch (type) {
  case MTP_TYPE_INT8:
    packet.putInt8(longValue);
    break;
  case MTP_TYPE_UINT8:
    packet.putUInt8(longValue);
    break;
  case MTP_TYPE_INT16:
    packet.putInt16(longValue);
    break;
  case MTP_TYPE_UINT16:
    packet.putUInt16(longValue);
    break;
  case MTP_TYPE_INT32:
    packet.putInt32(longValue);
    break;
  case MTP_TYPE_UINT32:
    packet.putUInt32(longValue);
    break;
  case MTP_TYPE_INT64:
    packet.putInt64(longValue);
    break;
  case MTP_TYPE_UINT64:
    packet.putUInt64(longValue);
    break;
  case MTP_TYPE_INT128:
    packet.putInt128(longValue);
    break;
  case MTP_TYPE_UINT128:
    packet.putInt128(longValue);        // <=
    break;
  ....
}

Предупреждение PVS-Studio: V525 CWE-682 The code contains the collection of similar blocks. Check items 'putInt8', 'putUInt8', 'putInt16', 'putUInt16', 'putInt32', 'putUInt32', 'putInt64', 'putUInt64', 'putInt128', 'putInt128' in lines 620, 623, 626, 629, 632, 635, 638, 641, 644, 647. android_mtp_MtpDatabase.cpp 620

В случае MTP_TYPE_UINT128 должна была быть вызвана функция putUInt128, а не putInt128.

И последний в этом разделе шикарный пример неудачного Copy-Paste.

static void btif_rc_upstreams_evt(....)
{
 ....
 case AVRC_PDU_REQUEST_CONTINUATION_RSP: {
   BTIF_TRACE_EVENT(
     "%s() REQUEST CONTINUATION: target_pdu: 0x%02d",
     __func__, pavrc_cmd->continu.target_pdu);
   tAVRC_RESPONSE avrc_rsp;
   if (p_dev->rc_connected == TRUE) {
     memset(&(avrc_rsp.continu), 0, sizeof(tAVRC_NEXT_RSP));
     avrc_rsp.continu.opcode =
         opcode_from_pdu(AVRC_PDU_REQUEST_CONTINUATION_RSP);
     avrc_rsp.continu.pdu = AVRC_PDU_REQUEST_CONTINUATION_RSP;
     avrc_rsp.continu.status = AVRC_STS_NO_ERROR;
     avrc_rsp.continu.target_pdu = pavrc_cmd->continu.target_pdu;
     send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp);
   }
 } break;

 case AVRC_PDU_ABORT_CONTINUATION_RSP: {
   BTIF_TRACE_EVENT(
     "%s() ABORT CONTINUATION: target_pdu: 0x%02d", __func__,
     pavrc_cmd->abort.target_pdu);
   tAVRC_RESPONSE avrc_rsp;
   if (p_dev->rc_connected == TRUE) {
     memset(&(avrc_rsp.abort), 0, sizeof(tAVRC_NEXT_RSP));
     avrc_rsp.abort.opcode =
         opcode_from_pdu(AVRC_PDU_ABORT_CONTINUATION_RSP);
     avrc_rsp.abort.pdu = AVRC_PDU_ABORT_CONTINUATION_RSP;
     avrc_rsp.abort.status = AVRC_STS_NO_ERROR;
     avrc_rsp.abort.target_pdu = pavrc_cmd->continu.target_pdu;
     send_metamsg_rsp(p_dev, -1, label, ctype, &avrc_rsp);
   }
 }
 break;
 ....
}

Прежде чем прочитать предупреждение анализатора и дальнейший текст, предлагаю поискать ошибку самостоятельно.

Чтобы вы сразу случайно не прочитали ответ, вот вам картинка для отвлечения внимания. Если Вас заинтересовало, что означает яйцо с надписью Java, то вам сюда.

Итак, надеюсь, вы получили удовольствие от поиска опечатки. Теперь время привести предупреждение анализатора: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'abort' variable should be used instead of 'continu'. btif_rc.cc 1554

Видимо, код писался методом Copy-Paste, и человек, как всегда, не смог быть внимательным в процессе правки скопированного фрагмента кода. В результате в самом конце он не заменил "continu" на "abort".

Т.е. во втором блоке должно быть написано:

avrc_rsp.abort.target_pdu = pavrc_cmd->abort.target_pdu;

Эта ситуация вполне попадает под определение "эффекта последней строки", так как ошибка при замене имён допущена в конце.

Facepalm

Очень забавный баг связан с преобразованием между little-endian и big-endian форматами данных (см. Порядок байтов).

inline uint32_t bswap32(uint32_t pData) {
  return
    (((pData & 0xFF000000) >> 24) | ((pData & 0x00FF0000) >> 8) |
     ((pData & 0x0000FF00) << 8) | ((pData & 0x000000FF) << 24));
}

bool ELFAttribute::merge(....) {
  ....
  uint32_t subsection_length =
    *reinterpret_cast<const uint32_t*>(subsection_data);

  if (llvm::sys::IsLittleEndianHost !=
      m_Config.targets().isLittleEndian())
    bswap32(subsection_length);
  ....
}

Предупреждение PVS-Studio: V530 CWE-252 The return value of function 'bswap32' is required to be utilized. ELFAttribute.cpp 84

К функции bswap32 претензий нет. А вот используется она неправильно:

bswap32(subsection_length);

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

Анализатор идентифицировал эту ошибку как CWE-252: Unchecked Return Value. Но, на самом деле, здесь уместнее назвать CWE-198: Use of Incorrect Byte Ordering. К сожалению, анализатор не может понять в чём состоит ошибка с высокоуровневой точки зрения. Впрочем, это не мешает ему выявить этот серьёзный дефект в коде.

Правильный код:

subsection_length = bswap32(subsection_length);

В коде Android есть ещё три места с идентичной ошибкой:

• V530 CWE-252 The return value of function 'bswap32' is required to be utilized. ELFAttribute.cpp 218
• V530 CWE-252 The return value of function 'bswap32' is required to be utilized. ELFAttribute.cpp 346
• V530 CWE-252 The return value of function 'bswap32' is required to be utilized. ELFAttribute.cpp 352

Чтобы избежать подобных ошибок, можно порекомендовать использовать атрибут [[nodiscard]]. Этот атрибут используется, чтобы обозначить, что возвращаемое значение функции должно быть обязательно использовано при вызове. Поэтому, если написать так:

[[nodiscard]] inline uint32_t bswap32(uint32_t pData) { ... }

то ошибка была бы выявлена ещё на этапе компиляции файла. Подробнее про некоторые новые полезные атрибуты вы можете узнать из статьи моего коллеги "С++17".

Недостижимый код

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

С точки зрения Common Weakness Enumeration — это CWE-561: Dead Code.

virtual sp<IEffect> createEffect(....)
{
  ....
  if (pDesc == NULL) {
    return effect;
    if (status != NULL) {
      *status = BAD_VALUE;
    }
  }
  ....
}

Предупреждение PVS-Studio: V779 CWE-561 Unreachable code detected. It is possible that an error is present. IAudioFlinger.cpp 733

Оператор return явно должен располагаться ниже.

Другие ошибки этого типа:

• V779 CWE-561 Unreachable code detected. It is possible that an error is present. bta_hf_client_main.cc 612
• V779 CWE-561 Unreachable code detected. It is possible that an error is present. android_media_ImageReader.cpp 468
• V779 CWE-561 Unreachable code detected. It is possible that an error is present. AMPEG4AudioAssembler.cpp 187

break

Забытый break внутри switch — это классическая ошибка C и C++ программистов. Чтобы с ней бороться, в C++17 появилась такая полезная аннотация, как [[fallthrough]]. Подробнее про эту ошибку и [[fallthrough]] вы можете прочитать в моей статье "break и fallthrough".

Но пока в мире полно старого кода, где [[fallthrough]] не используется, вам пригодится PVS-Studio. Рассмотрим несколько ошибок, найденных в Android. Согласно Common Weakness Enumeration, эти ошибки классифицируются как CWE-484: Omitted Break Statement in Switch.

bool A2dpCodecConfigLdac::setCodecConfig(....) {
  ....
  case BTAV_A2DP_CODEC_SAMPLE_RATE_192000:
    if (sampleRate & A2DP_LDAC_SAMPLING_FREQ_192000) {
      result_config_cie.sampleRate =
          A2DP_LDAC_SAMPLING_FREQ_192000;
      codec_capability_.sample_rate =
          codec_user_config_.sample_rate;
      codec_config_.sample_rate =
          codec_user_config_.sample_rate;
    }
  case BTAV_A2DP_CODEC_SAMPLE_RATE_16000:
  case BTAV_A2DP_CODEC_SAMPLE_RATE_24000:
  case BTAV_A2DP_CODEC_SAMPLE_RATE_NONE:
    codec_capability_.sample_rate =
        BTAV_A2DP_CODEC_SAMPLE_RATE_NONE;
    codec_config_.sample_rate =
        BTAV_A2DP_CODEC_SAMPLE_RATE_NONE;
    break;
  ....
}

Предупреждение PVS-Studio: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. a2dp_vendor_ldac.cc 912

Думаю, ошибка в пояснении не нуждается. Отмечу только, что нередко аномалия в коде обнаруживается более чем одним способом. Например, эта ошибка также выявляется предупреждениями V519:

• V519 CWE-563 The 'codec_capability_.sample_rate' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 910, 916. a2dp_vendor_ldac.cc 916
• V519 CWE-563 The 'codec_config_.sample_rate' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 911, 917. a2dp_vendor_ldac.cc 917

И ещё парочка ошибок:

Return<void> EffectsFactory::getAllDescriptors(....)  {
  ....
  switch (status) {
    case -ENOSYS: {
      // Effect list has changed.
      goto restart;
    }
    case -ENOENT: {
      // No more effects available.
      result.resize(i);
    }
    default: {
      result.resize(0);
      retval = Result::NOT_INITIALIZED;
    }
  }
  ....
}

Предупреждение PVS-Studio: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. EffectsFactory.cpp 118

int Reverb_getParameter(....)
{
  ....
  case REVERB_PARAM_REFLECTIONS_LEVEL:
    *(uint16_t *)pValue = 0;
  case REVERB_PARAM_REFLECTIONS_DELAY:
    *(uint32_t *)pValue = 0;
  case REVERB_PARAM_REVERB_DELAY:
    *(uint32_t *)pValue = 0;
  break;
  ....
}

Предупреждение PVS-Studio: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. EffectReverb.cpp 1847

static SLresult IAndroidConfiguration_GetConfiguration(....)
{
  ....
  switch (IObjectToObjectID((thiz)->mThis)) {
  case SL_OBJECTID_AUDIORECORDER:
    result = android_audioRecorder_getConfig(
      (CAudioRecorder *) thiz->mThis, configKey,
      pValueSize, pConfigValue);
    break;
  case SL_OBJECTID_AUDIOPLAYER:
    result = android_audioPlayer_getConfig(
      (CAudioPlayer *) thiz->mThis, configKey,
      pValueSize, pConfigValue);
  default:
    result = SL_RESULT_FEATURE_UNSUPPORTED;
    break;
  }  
  ....
}

Предупреждение PVS-Studio: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. IAndroidConfiguration.cpp 90

Неправильное управление памятью

Здесь я собрал ошибки, связанные с неверным управление памятью. Такие предупреждения, согласно Common Weakness Enumeration, классифицируются как:

• CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')
• CWE-562: Return of Stack Variable Address
• CWE-762: Mismatched Memory Management Routines

Начнём с функций, возвращающих ссылку на уже разрушенную переменную.

TransformIterator& operator++(int) {
  TransformIterator tmp(*this);
  ++*this;
  return tmp;
}

TransformIterator& operator--(int) {
  TransformIterator tmp(*this);
  --*this;
  return tmp;
}

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

• V558 CWE-562 Function returns the reference to temporary local object: tmp. transform_iterator.h 77
• V558 CWE-562 Function returns the reference to temporary local object: tmp. transform_iterator.h 92

Когда функции заканчивают своё выполнение, переменная tmp разрушается, так как она создана на стеке. Следовательно, функции возвращают ссылку на уже разрушенный (несуществующий) объект.

Правильным решением будет возврат по значению:

TransformIterator operator++(int) {
  TransformIterator tmp(*this);
  ++*this;
  return tmp;
}

TransformIterator operator--(int) {
  TransformIterator tmp(*this);
  --*this;
  return tmp;
}

Рассмотрим ещё более грустный код, заслуживающий пристального внимания.

int register_socket_transport(
  int s, const char* serial, int port, int local)
{
  atransport* t = new atransport();

  if (!serial) {
    char buf[32];
    snprintf(buf, sizeof(buf), "T-%p", t);
    serial = buf;
  }
  ....
}

Предупреждение PVS-Studio: V507 CWE-562 Pointer to local array 'buf' is stored outside the scope of this array. Such a pointer will become invalid. transport.cpp 1030

Это опасный код. Если фактическое значение аргумента serial равно NULL, то должен быть использован временный буфер на стеке. Когда тело оператора if закончится, массив buf перестанет существовать. Место, где был создан буфер, может использоваться для хранения других переменных, создаваемых на стеке. Начнётся адская мешанина в данных, и последствия подобной ошибки плохо предсказуемы.

Следующие ошибки связаны с несовместимыми способами создания и уничтожения объектов.

void
SensorService::SensorEventConnection::reAllocateCacheLocked(....)
{
  sensors_event_t *eventCache_new;
  const int new_cache_size = computeMaxCacheSizeLocked();
  eventCache_new = new sensors_event_t[new_cache_size];
  ....
  delete mEventCache;
  mEventCache = eventCache_new;
  mCacheSize += count;
  mMaxCacheSize = new_cache_size;
}

Предупреждение PVS-Studio: V611 CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] mEventCache;'. Check lines: 391, 384. SensorEventConnection.cpp 391

Здесь всё просто. Буфер, указатель на который хранится в члене класса mEventCache, выделяется с помощью оператора new []. А освобождают эту память, используя оператор delete. Это неправильно и приводит к неопределенному поведению программы.

Аналогичная ошибка:

aaudio_result_t AAudioServiceEndpointCapture::open(....) {
  ....
  delete mDistributionBuffer;
  int distributionBufferSizeBytes =
    getStreamInternal()->getFramesPerBurst() *
    getStreamInternal()->getBytesPerFrame();
  mDistributionBuffer = new uint8_t[distributionBufferSizeBytes];
  ....
}

Предупреждение PVS-Studio: V611 CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] mDistributionBuffer;'. AAudioServiceEndpointCapture.cpp 50

Думаю, что ошибка не требует пояснения.

Следующий случай немного интересней, но суть ошибки в точности та же.

struct HeifFrameInfo
{
  ....
  void set(....) {
    ....
    mIccData.reset(new uint8_t[iccSize]);
    ....
  }
  ....
  std::unique_ptr<uint8_t> mIccData;
};

V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. HeifDecoderAPI.h 62

По умолчанию класс умного указателя std::unique_ptr для разрушения объекта вызывает оператор delete. Однако в функции set память выделяется с помощью оператора new [].

Правильный вариант:

std::unique_ptr<uint8_t[]> mIccData;

Другие ошибки:

• V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. atrace.cpp 949
• V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. atrace.cpp 950
• V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. HeifDecoderImpl.cpp 102
• V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. HeifDecoderImpl.cpp 166
• V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. ColorSpace.cpp 360

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

Asset* Asset::createFromUncompressedMap(FileMap* dataMap,
  AccessMode mode)
{
  _FileAsset* pAsset;
  status_t result;

  pAsset = new _FileAsset;
  result = pAsset->openChunk(dataMap);
  if (result != NO_ERROR)
    return NULL;

  pAsset->mAccessMode = mode;
  return pAsset;
}

Предупреждение PVS-Studio: V773 CWE-401 The function was exited without releasing the 'pAsset' pointer. A memory leak is possible. Asset.cpp 296

Если не удалось открыть некий chunk, то функция завершает свою работу без разрушения объекта, указатель на который хранится в переменной pAsset. В результате произойдёт утечка памяти.

Другие ошибки аналогичны, поэтому я не вижу смысла рассматривать их в статье. Желающие могут посмотреть другие предупреждения в файле: Android_V773.txt.

Выход за границу массива

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

if (i < 0 || i > MAX)
  return;
A[i] = x;

В C и C++ ячейки массива нумеруются с 0, поэтому максимальный индекс элемента в массиве должен быть на единицу меньше, чем размер самого массива. Правильная проверка должна быть такой:

if (i < 0 || i >= MAX)
  return;
A[i] = x;

Выход за границу массива, согласно Common Weakness Enumeration, классифицируется как CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer.

Взглянем, как эти ошибки выглядят в коде Android.

static btif_hf_cb_t btif_hf_cb[BTA_AG_MAX_NUM_CLIENTS];

static bool IsSlcConnected(RawAddress* bd_addr) {
  if (!bd_addr) {
    LOG(WARNING) << __func__ << ": bd_addr is null";
    return false;
  }
  int idx = btif_hf_idx_by_bdaddr(bd_addr);
  if (idx < 0 || idx > BTA_AG_MAX_NUM_CLIENTS) {
    LOG(WARNING) << __func__ << ": invalid index "
                 << idx << " for " << *bd_addr;
    return false;
  }
  return btif_hf_cb[idx].state ==
           BTHF_CONNECTION_STATE_SLC_CONNECTED;
}

Предупреждение PVS-Studio: V557 CWE-119 Array overrun is possible. The value of 'idx' index could reach 6. btif_hf.cc 277

Правильный вариант проверки:

if (idx < 0 || idx >= BTA_AG_MAX_NUM_CLIENTS) {

Точно таких же ошибок нашлось ещё две штуки:

• V557 CWE-119 Array overrun is possible. The value of 'idx' index could reach 6. btif_hf.cc 869
• V557 CWE-119 Array overrun is possible. The value of 'index' index could reach 6. btif_rc.cc 374

Сломанные циклы

Есть масса способов написать неправильно работающий цикл. В коде Android я встретил ошибки, которые, согласно Common Weakness Enumeration, можно классифицировать как:

• CWE-20: Improper Input Validation
• CWE-670: Always-Incorrect Control Flow Implementation
• CWE-691: Insufficient Control Flow Management
• CWE-834: Excessive Iteration

При этом, конечно, существуют и другие способы «отстрелить себе ногу» при написании циклов, но в этот раз они мне не повстречались.

int main(int argc, char **argv)
{
  ....
  char c;
  printf("%s is already in *.base_fs format, just copying ....);
  rewind(blk_alloc_file);
  while ((c = fgetc(blk_alloc_file)) != EOF) {
    fputc(c, base_fs_file);
  }
  ....
}

Предупреждение PVS-Studio: V739 CWE-20 EOF should not be compared with a value of the 'char' type. The '(c = fgetc(blk_alloc_file))' should be of the 'int' type. blk_alloc_to_base_fs.c 61

Анализатор обнаружил, что константа EOF сравнивается с переменной типа char. Давайте разберёмся, почему этот код некорректен.

Функция fgetc возвращает значение типа int, а именно: она может вернуть число от 0 до 255 или EOF (-1). Прочитанное значение помещается в переменную типа char. Из-за этого символ со значением 0xFF (255) превращается в -1 и интерпретируется точно так же, как конец файла (EOF).

Из-за таких ошибок пользователи, использующие Extended ASCII Codes, иногда сталкиваются с ситуацией, когда один из символов их алфавита некорректно обрабатывается программами. Например, последняя буква русского алфавита в кодировке Windows-1251 как раз имеет код 0xFF и воспринимается некоторыми программами как конец файла.

Подводя итог, можно сказать, что условие остановки цикла написано некорректно. Чтобы исправить ситуацию нужно, чтобы переменная c имела тип int.

Продолжим и рассмотрим более привычные ошибки при использовании оператора for.

status_t AudioPolicyManager::registerPolicyMixes(....)
{
  ....
  for (size_t i = 0; i < mixes.size(); i++) {
    ....
    for (size_t j = 0; i < mHwModules.size(); j++) {       // <=
      if (strcmp(AUDIO_HARDWARE_MODULE_ID_REMOTE_SUBMIX,
                 mHwModules[j]->mName) == 0
          && mHwModules[j]->mHandle != 0) {
        rSubmixModule = mHwModules[j];
        break;
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio: V534 CWE-691 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i'. AudioPolicyManager.cpp 2489

Из-за опечатки во вложенном цикле в условии используется переменная i, хотя необходимо использовать переменную j. В результате переменная j неконтролируемо инкрементируется, что со временем приведёт к выходу за границу массива mHwModules. Что произойдёт дальше, предсказать невозможно, так как возникнет неопределённое поведение программы.

Кстати, этот фрагмент с ошибкой полностью скопировали в другую функцию. Поэтому точно такую же ошибку анализатор нашёл здесь: AudioPolicyManager.cpp 2586.

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

Первый фрагмент:

void ce_t3t_handle_check_cmd(....) {
  ....
  for (i = 0; i < p_cb->cur_cmd.num_blocks; i++) {
    ....
    for (i = 0; i < T3T_MSG_NDEF_ATTR_INFO_SIZE; i++) {
      checksum += p_temp[i];
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio: V535 CWE-691 The variable 'i' is being used for this loop and for the outer loop. Check lines: 398, 452. ce_t3t.cc 452

Обратите внимание, что переменная i используется как для внешнего, так и для внутреннего цикла.

Ещё два аналогичных срабатывания анализатора:

• V535 CWE-691 The variable 'xx' is being used for this loop and for the outer loop. Check lines: 801, 807. sdp_db.cc 807
• V535 CWE-691 The variable 'xx' is being used for this loop and for the outer loop. Check lines: 424, 438. nfa_hci_act.cc 438

Вы ещё не устали? Предлагаю сделать паузу и скачать PVS-Studio, чтобы попробовать проверить свой проект.

А теперь продолжим.

#define NFA_HCI_LAST_PROP_GATE 0xFF

tNFA_HCI_DYN_GATE* nfa_hciu_alloc_gate(uint8_t gate_id,
                                       tNFA_HANDLE app_handle) {
  ....
  for (gate_id = NFA_HCI_FIRST_HOST_SPECIFIC_GENERIC_GATE;
       gate_id <= NFA_HCI_LAST_PROP_GATE; gate_id++) {
    if (gate_id == NFA_HCI_CONNECTIVITY_GATE) gate_id++;
    if (nfa_hciu_find_gate_by_gid(gate_id) == NULL) break;
  }

  if (gate_id > NFA_HCI_LAST_PROP_GATE) {
    LOG(ERROR) << StringPrintf(
        "nfa_hci_alloc_gate - no free Gate ID: %u  "
        "App Handle: 0x%04x", gate_id, app_handle);
    return (NULL);
  }
  ....
}

Предупреждение PVS-Studio: V654 CWE-834 The condition 'gate_id <= 0xFF' of loop is always true. nfa_hci_utils.cc 248

Обратите внимание на следующее:

• Константа NFA_HCI_LAST_PROP_GATE равна значению 0xFF.
• В качестве счётчика цикла используется переменная типа uint8_t. Следовательно, диапазон значений этой переменной [0..0xFF].

Получается, что условие gate_id <= NFA_HCI_LAST_PROP_GATE всегда истинно и не может остановить выполнение цикла.

Анализатор классифицировал эту ошибку как CWE-834, но её можно также интерпретировать и как CWE-571: Expression is Always True.

Следующая ошибка в цикле связана с неопределённым поведением.

status_t SimpleDecodingSource::doRead(....) {
  ....
  for (int retries = 0; ++retries; ) {
  ....
}

Предупреждение PVS-Studio: V654 CWE-834 The condition '++ retries' of loop is always true. SimpleDecodingSource.cpp 226

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

Цикл должен остановиться, когда выражение ++retries будет равно 0. А это возможно только в том случае, если произойдёт переполнение переменной. Поскольку переменная имеет знаковый тип, то её переполнение приводит к неопределённому поведению. Следовательно, этот код некорректен и может привести к непредсказуемым последствиям. Например, компилятор имеет полное право удалить проверку и оставить только инструкцию для инкремента счётчика.

И последняя ошибка в этом разделе.

status_t Check(const std::string& source) {
  ....
  int pass = 1;
  ....
  do {
    ....
    switch(rc) {
    case 0:
      SLOGI("Filesystem check completed OK");
      return 0;

    case 2:
      SLOGE("Filesystem check failed (not a FAT filesystem)");
      errno = ENODATA;
      return -1;

    case 4:
      if (pass++ <= 3) {
          SLOGW("Filesystem modified - rechecking (pass %d)",
                  pass);
          continue;                                         // <=
      }
      SLOGE("Failing check after too many rechecks");
      errno = EIO;
      return -1;

    case 8:
      SLOGE("Filesystem check failed (no filesystem)");
      errno = ENODATA;
      return -1;

    default:
      SLOGE("Filesystem check failed (unknown exit code %d)", rc);
      errno = EIO;
      return -1;
    }
  } while (0);                                              // <=

  return 0;
}

Предупреждение PVS-Studio: V696 CWE-670 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. Check lines: 105, 121. Vfat.cpp 105

Перед нами цикл вида:

do {
  ....
  if (x) continue;
  ....
} while (0)

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

Чтобы исправить ошибку, код можно переписать, например, так:

for (;;) {
  ....
  if (x) continue;
  ....
  break;
}

Повторное присваивание переменной

Очень распространённой ошибкой является повторная запись в переменную ещё до того, как предыдущее значение было использовано. Чаще всего такие ошибки возникают из-за опечатки или неудачного Copy-Paste. Согласно Common Weakness Enumeration, такие ошибки классифицируются как CWE-563: Assignment to Variable without Use. Не обошлось без таких ошибок и в Android.

status_t XMLNode::flatten_node(....) const
{
  ....
  memset(&namespaceExt, 0, sizeof(namespaceExt));
  if (mNamespacePrefix.size() > 0) {
    namespaceExt.prefix.index =
      htodl(strings.offsetForString(mNamespacePrefix));
  } else {
    namespaceExt.prefix.index = htodl((uint32_t)-1);
  }
  namespaceExt.prefix.index =
    htodl(strings.offsetForString(mNamespacePrefix));
  namespaceExt.uri.index =
    htodl(strings.offsetForString(mNamespaceUri));
  ....
}

Предупреждение PVS-Studio: V519 CWE-563 The 'namespaceExt.prefix.index' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1535, 1539. XMLNode.cpp 1539

Чтобы выделить суть ошибки, напишу псевдокод:

if (a > 0)
  X = 1;
else
  X = 2;
X = 1;

Независимо от условия, переменной X (в настоящем коде это namespaceExt.prefix.index) будет всегда присвоено одно значение.

bool AudioFlinger::RecordThread::threadLoop()
{
 ....
 size_t framesToRead = mBufferSize / mFrameSize;
 framesToRead = min(mRsmpInFramesOA - rear, mRsmpInFramesP2 / 2);
 ....
}

Предупреждение PVS-Studio: V519 CWE-563 The 'framesToRead' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 6341, 6342. Threads.cpp 6342

Непонятно, зачем надо было инициализировать переменную при объявлении, если затем в неё сразу записывается другое значение. Что-то здесь не так.

void SchedulingLatencyVisitorARM::VisitArrayGet(....) {
  ....
  if (index->IsConstant()) {
    last_visited_latency_ = kArmMemoryLoadLatency;
  } else {
    if (has_intermediate_address) {
    } else {
      last_visited_internal_latency_ += kArmIntegerOpLatency;
    }
    last_visited_internal_latency_ = kArmMemoryLoadLatency;
  }
  ....
}

Предупреждение PVS-Studio: V519 CWE-563 The 'last_visited_internal_latency_' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 680, 682. scheduler_arm.cc 682

Очень странный, бессмысленный код. Рискну предположить, что должно было быть написано:

last_visited_internal_latency_ += kArmMemoryLoadLatency;

И последняя ошибка, демонстрирующая, как анализатор без устали находит ошибки, которые с большой вероятностью будут пропущены даже при внимательном code review.

void multiprecision_fast_mod(uint32_t* c, uint32_t* a) {
  uint32_t U;
  uint32_t V;
  ....
  c[0] += U;
  V = c[0] < U;
  c[1] += V;
  V = c[1] < V;
  c[2] += V;                //
  V = c[2] < V;             // <=
  c[2] += U;                //
  V = c[2] < U;             // <=
  c[3] += V;
  V = c[3] < V;
  c[4] += V;
  V = c[4] < V;
  c[5] += V;
  V = c[5] < V;
  ....  
}

Предупреждение PVS-Studio: V519 CWE-563 The 'V' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 307, 309. p_256_multprecision.cc 309

Код настолько «вырви глаз», что мне не хочется в нём разбираться. При этом явно видно: здесь имеет место опечатка в коде, который я выделил комментариями.

Другие ошибки

Остались разрозненные ошибки, для которых нет смысла делать отдельные главы. Однако они столь же интересны и коварны, как и рассмотренные ранее.

Приоритет операций

void TagMonitor::parseTagsToMonitor(String8 tagNames) {
  std::lock_guard<std::mutex> lock(mMonitorMutex);

  // Expand shorthands
  if (ssize_t idx = tagNames.find("3a") != -1) {
    ssize_t end = tagNames.find(",", idx);
    char* start = tagNames.lockBuffer(tagNames.size());
    start[idx] = '';
    ....
  }
  ....
}

Предупреждение PVS-Studio: V593 CWE-783 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. TagMonitor.cpp 50

По классификации Common Weakness Enumeration: CWE-783: Operator Precedence Logic Error.

Программист задумал следующее. Происходит поиск подстроки «3a» и в переменную idx записывается позиция этой подстроки. Если подстрока найдена (idx != -1), то начинает выполняться код, в котором используется значение переменной idx.

К сожалению, программист запутался в приоритетах операций. На самом деле проверка работает так:

if (ssize_t idx = (tagNames.find("3a") != -1))

Вначале проверяется, есть ли в строке подстрока «3a», и результат false/true помещается в переменную idx. В итоге переменная idx имеет значение 0 или 1.

Если условие истинно (переменная idx равна 1), то начинает выполнятся логика, использующая переменную idx. Переменная всегда равная 1 приведёт к неправильному поведению программы.

Исправить ошибку можно, если вынести инициализацию переменной из условия:

ssize_t idx = tagNames.find("3a");
if (idx != -1)

Новая версия языка C++17 также позволяет написать:

if (ssize_t idx = tagNames.find("3a"); idx != -1)

Неправильный конструктор

struct HearingDevice {
  ....
  HearingDevice() { HearingDevice(RawAddress::kEmpty, false); }
  ....
};

Предупреждение PVS-Studio: V603 CWE-665 The object was created but it is not being used. If you wish to call constructor, 'this->HearingDevice::HearingDevice(....)' should be used. hearing_aid.cc 176

По классификации Common Weakness Enumeration: CWE-665: Improper Initialization.

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

Происходит следующее. Создаётся новый неименованный объект типа HearingDevice и тут же разрушается. В результате поля класса остаются неинициализированными.

Чтобы исправить ошибку, можно использовать делегирующий конструктор (эта возможность появилась в C++11). Правильный код:

HearingDevice() : HearingDevice(RawAddress::kEmpty, false) { }

Функция не возвращает значение

int NET_RecvFrom(int s, void *buf, int len, unsigned int flags,
       struct sockaddr *from, int *fromlen) {
  socklen_t socklen = *fromlen;
  BLOCKING_IO_RETURN_INT(
    s, recvfrom(s, buf, len, flags, from, &socklen) );
  *fromlen = socklen;
}

Предупреждение PVS-Studio: V591 CWE-393 Non-void function should return a value. linux_close.cpp 139

По классификации Common Weakness Enumeration: CWE-393: Return of Wrong Status Code.

Функция вернёт случайное значение. Это не очень интересная ошибка. Давайте я лучше размещу здесь «пасхалку». Тот герой, который дочитал статью до этого места, может бесплатно получить от нас по почте бумажный набор статусов для программистов. Просто напишите мне на ak@pvs-studio.com до 1 сентября 2018 и укажите свой почтовый адрес. Только прошу не обсуждать это в комментариях, иначе станет неинтересно и придётся отказаться от рассылки призов нашим самым внимательным читателям. Ещё одна такая ошибка: V591 CWE-393 Non-void function should return a value. linux_close.cpp 158

Неправильное вычисление размера структуры

int MtpFfsHandle::handleControlRequest(....) {
  ....
  struct mtp_device_status *st =
    reinterpret_cast<struct mtp_device_status*>(buf.data());
  st->wLength = htole16(sizeof(st));
  ....
}

Предупреждение PVS-Studio: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'st' class object. MtpFfsHandle.cpp 251

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

st->wLength = htole16(sizeof(*st));

Аналогичные срабатывания анализатора:

• V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'cacheinfo' class object. NetlinkEvent.cpp 220
• V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'page->next' class object. linker_block_allocator.cpp 146
• V568 It's odd that the argument of sizeof() operator is the '& session_id' expression. reference-ril.c 1775

Бессмысленные битовые операции

#define EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR  0x00000001
#define EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR 0x00000002
#define EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR 0x00000004

EGLContext eglCreateContext(....)
{
  ....
  case EGL_CONTEXT_FLAGS_KHR:
    if ((attrib_val | EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) ||
        (attrib_val | EGL_CONTEXT_OPENGL_FORWARD_C....) ||
        (attrib_val | EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR))
    {
      context_flags = attrib_val;
    } else {
      RETURN_ERROR(EGL_NO_CONTEXT,EGL_BAD_ATTRIBUTE);
    }
  ....
}

Предупреждение PVS-Studio: V617 CWE-480 Consider inspecting the condition. The '0x00000001' argument of the '|' bitwise operation contains a non-zero value. egl.cpp 1329

По классификации Common Weakness Enumeration: CWE-480: Use of Incorrect Operator.

Выражение вида (A | 1) || (A | 2) || (A | 4) не имеет смысла, так как результат всегда будет true. На самом деле, надо использовать оператор &, и тогда код приобретает смысл:

if ((attrib_val & EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) ||
    (attrib_val & EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR) ||
    (attrib_val & EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR))

Аналогичная ошибка: V617 CWE-480 Consider inspecting the condition. The '0x00000001' argument of the '|' bitwise operation contains a non-zero value. egl.cpp 1338

Неправильный битовый сдвиг

template <typename AddressType>
struct RegsInfo {
  ....
  uint64_t saved_reg_map = 0;
  AddressType saved_regs[64];
  ....
  inline AddressType* Save(uint32_t reg) {
    if (reg > sizeof(saved_regs) / sizeof(AddressType)) {
      abort();
    }
    saved_reg_map |= 1 << reg;
    saved_regs[reg] = (*regs)[reg];
    return &(*regs)[reg];
  }
  ....
}

Предупреждение PVS-Studio: V629 CWE-190 Consider inspecting the '1 << reg' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. RegsInfo.h 47

По классификации Common Weakness Enumeration: CWE-190: Integer Overflow or Wraparound.

При сдвиге 1 << reg значение переменной reg лежит в диапазоне [0..63]. Выражение служит для того, чтобы получать различные степени двойки, начиная с 2^0 и заканчивая 2^63.

Код не работает. Дело в том, что числовой литерал 1 имеет 32-битный тип int. Поэтому не получится получить значение большее чем 1^31. Сдвиг на большее значение приводит к переполнению переменной и возникновению неопределённого поведения.

Правильный код:

saved_reg_map |= static_cast<uint64_t>(1) << reg;

или:

saved_reg_map |= 1ULL << reg;

Строки копируются сами в себя

void PCLmGenerator::writeJobTicket() {
 // Write JobTicket
 char inputBin[256];
 char outputBin[256];

 if (!m_pPCLmSSettings) {
   return;
 }

 getInputBinString(m_pPCLmSSettings->userInputBin, &inputBin[0]);
 getOutputBin(m_pPCLmSSettings->userOutputBin, &outputBin[0]);
 strcpy(inputBin, inputBin);
 strcpy(outputBin, outputBin);
 ....
}

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

• V549 CWE-688 The first argument of 'strcpy' function is equal to the second argument. genPCLm.cpp 1181
• V549 CWE-688 The first argument of 'strcpy' function is equal to the second argument. genPCLm.cpp 1182

По классификации Common Weakness Enumeration: CWE-688: Function Call With Incorrect Variable or Reference as Argument.

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

Использование неинициализированной переменной

void mca_set_cfg_by_tbl(....) {
  tMCA_DCB* p_dcb;
  const tL2CAP_FCR_OPTS* p_opt;
  tMCA_FCS_OPT fcs = MCA_FCS_NONE;

  if (p_tbl->tcid == MCA_CTRL_TCID) {
    p_opt = &mca_l2c_fcr_opts_def;
  } else {
    p_dcb = mca_dcb_by_hdl(p_tbl->cb_idx);
    if (p_dcb) {
      p_opt = &p_dcb->p_chnl_cfg->fcr_opt;
      fcs = p_dcb->p_chnl_cfg->fcs;
    }
  }
  memset(p_cfg, 0, sizeof(tL2CAP_CFG_INFO));
  p_cfg->mtu_present = true;
  p_cfg->mtu = p_tbl->my_mtu;
  p_cfg->fcr_present = true;
  memcpy(&p_cfg->fcr, p_opt, sizeof(tL2CAP_FCR_OPTS));    // <=
  ....
}

Предупреждение PVS-Studio: V614 CWE-824 Potentially uninitialized pointer 'p_opt' used. Consider checking the second actual argument of the 'memcpy' function. mca_main.cc 252

По классификации Common Weakness Enumeration: CWE-824: Access of Uninitialized Pointer.

Если p_tbl->tcid != MCA_CTRL_TCID и p_dcb == nullptr, то указатель p_opt останется неинициализированным.

Более странное использование неинициализированной переменной

struct timespec
{
  __time_t tv_sec;    /* Seconds.  */
  long int tv_nsec;   /* Nanoseconds.  */
};

static inline timespec NsToTimespec(int64_t ns) {
  timespec t;
  int32_t remainder;

  t.tv_sec = ns / kNanosPerSecond;
  remainder = ns % kNanosPerSecond;
  if (remainder < 0) {
    t.tv_nsec--;
    remainder += kNanosPerSecond;
  }
  t.tv_nsec = remainder;

  return t;
}

Предупреждение PVS-Studio: V614 CWE-457 Uninitialized variable 't.tv_nsec' used. clock_ns.h 55

По классификации Common Weakness Enumeration: CWE-457: Use of Uninitialized Variable.

В момент декремента переменной t.tv_nsec она является неинициализированной. Переменная инициализируется позже: t.tv_nsec = remainder;. Что-то здесь явно напутано.

Избыточное выражение

void bta_dm_co_ble_io_req(....)
{
  ....
  *p_auth_req = bte_appl_cfg.ble_auth_req |
                (bte_appl_cfg.ble_auth_req & 0x04) |
                ((*p_auth_req) & 0x04);
  ....
}

Предупреждение PVS-Studio: V578 An odd bitwise operation detected. Consider verifying it. bta_dm_co.cc 259

Это выражение избыточно. Если удалить подвыражение (bte_appl_cfg.ble_auth_req & 0x04), то результат выражения не изменится. Возможно, здесь есть какая-то опечатка.

Утечка дескриптора

bool RSReflectionCpp::genEncodedBitCode() {
  FILE *pfin = fopen(mBitCodeFilePath.c_str(), "rb");
  if (pfin == nullptr) {
    fprintf(stderr, "Error: could not read file %sn",
            mBitCodeFilePath.c_str());
    return false;
  }

  unsigned char buf[16];
  int read_length;
  mOut.indent() << "static const unsigned char __txt[] =";
  mOut.startBlock();
  while ((read_length = fread(buf, 1, sizeof(buf), pfin)) > 0) {
    mOut.indent();
    for (int i = 0; i < read_length; i++) {
      char buf2[16];
      snprintf(buf2, sizeof(buf2), "0x%02x,", buf[i]);
      mOut << buf2;
    }
    mOut << "n";
  }
  mOut.endBlock(true);
  mOut << "n";
  return true;
}

Предупреждение PVS-Studio: V773 CWE-401 The function was exited without releasing the 'pfin' handle. A resource leak is possible. slang_rs_reflection_cpp.cpp 448

Анализатор классифицировал эту ошибку, согласно Common Weakness Enumeration, как: CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak'). Однако правильнее здесь было бы выдать CWE-775: Missing Release of File Descriptor or Handle after Effective Lifetime. Поручу коллегам поправить эту недоработку в PVS-Studio.

Дескриптор pfin нигде не освобождается. Просто забыли вызвать в конце функцию fclose. Неприятная ошибка, которая может быстро исчерпать весь запас доступных дескрипторов, после чего будет невозможно открытие новых файлов.

Заключение

Как видите, даже в таком известном и хорошо протестированном проекте, как Android, анализатор PVS-Studio легко находит множество ошибок и потенциальных уязвимостей. Подытожим, какие weakness (потенциальные уязвимости) были найдены:

• CWE-14: Compiler Removal of Code to Clear Buffers
• CWE-20: Improper Input Validation
• CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer
• CWE-190: Integer Overflow or Wraparound
• CWE-198: Use of Incorrect Byte Ordering
• CWE-393: Return of Wrong Status Code
• CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')
• CWE-457: Use of Uninitialized Variable
• CWE-462: Duplicate Key in Associative List
• CWE-480: Use of Incorrect Operator
• CWE-484: Omitted Break Statement in Switch
• CWE-561: Dead Code
• CWE-562: Return of Stack Variable Address
• CWE-563: Assignment to Variable without Use
• CWE-570: Expression is Always False
• CWE-571: Expression is Always True
• CWE-476: NULL Pointer Dereference
• CWE-628: Function Call with Incorrectly Specified Arguments
• CWE-665: Improper Initialization
• CWE-670: Always-Incorrect Control Flow Implementation
• CWE-682: Incorrect Calculation
• CWE-688: Function Call With Incorrect Variable or Reference as Argument
• CWE-690: Unchecked Return Value to NULL Pointer Dereference
• CWE-691: Insufficient Control Flow Management
• CWE-758: Reliance on Undefined, Unspecified, or Implementation-Defined Behavior
• CWE-762: Mismatched Memory Management Routines
• CWE-775: Missing Release of File Descriptor or Handle after Effective Lifetime
• CWE-783: Operator Precedence Logic Error
• CWE-824: Access of Uninitialized Pointer
• CWE-834: Excessive Iteration

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

Размер проверенной кодовой базы составляет около 2168000 строк кода. Из них 14,4% — это комментарии. Итого, получаем около 1855000 строк чистого кода.

Таким образом, мы имеем 490 CWE на 1855000 строк с кодом.

Получается, что анализатор PVS-Studio способен выявить более чем 1 weakness (потенциальную уязвимость) на каждые 4000 строк кода в проекте Android. Хороший результат для анализатора кода, я рад.

Спасибо за внимание! Желаю всем безбажного кода и предлагаю сделать следующее:

1. Скачать PVS-Studio и проверить рабочий проект.
2. Только прошу: не запускайте анализатор на синтетических тестах: Почему я не люблю синтетические тесты.
3. Подписывайтесь, чтобы быть в курсе публикации наших новых статей: twitter, RSS, vk.com.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. We Checked the Android Source Codes by PVS-Studio or Nothing is Perfect

Автор: Andrey2008

Источник