Static code analysis

Нам предложили проверить с помощью анализатора PVS-Studio коллекцию открытых библиотек PMDK, предназначенную для разработки и отладки приложений с поддержкой энергонезависимой памяти. Собственно, почему бы и нет. Тем более это небольшой проект на языке C и C++ с общим размером кодовой базы около 170 KLOC, если не считать комментарии. А значит, обзор результатов анализа не займёт много сил и времени. Let's go.

Для анализа исходного кода будет использован инструмент PVS-Studio версии 7.08. Читатели нашего блога, естественно, давно знакомы с нашим инструментом, и я не буду на нём останавливаться. Для тех, кто впервые зашёл к нам, предлагаю обратиться к статье "Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?" и попробовать бесплатную триальную версию анализатора.

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

Итак, PMDK это коллекция библиотек и инструментальных средств с открытым исходным кодом, предназначенных для упрощения разработки, отладки и управления приложениями с поддержкой энергонезависимой памяти. Подробнее здесь: PMDK Introduction. Исходники здесь: pmdk.

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

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

Размер выделяемой памяти

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

int main(int argc, char *argv[]){  ....  struct pool *pop = malloc(sizeof(pop));  ....}

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

Классическая опечатка, из-за которой выделяется неверное количество памяти. Оператор sizeof вернёт не размер структуры, а размер указателя на эту структуру. Правильным вариантом будет:

struct pool *pop = malloc(sizeof(pool));

или

struct pool *pop = malloc(sizeof(*pop));

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

struct pool {  struct ctl *ctl;};

Получается, что структура занимает ровно столько, сколько и указатель. Всё хорошо.

Длина строки

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

typedef void *(*pmem2_memcpy_fn)(void *pmemdest, const void *src, size_t len,    unsigned flags);static const char *initial_state = "No code.";static inttest_rwx_prot_map_priv_do_execute(const struct test_case *tc,  int argc, char *argv[]){  ....  char *addr_map = pmem2_map_get_address(map);  map->memcpy_fn(addr_map, initial_state, sizeof(initial_state), 0);  ....}

Предупреждение PVS-Studio: V579 [CWE-687] The memcpy_fn function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pmem2_map_prot.c 513

Для копирования строки используется указатель на специальную функцию копирования. Обратите внимание на вызов этой функции, а вернее на её третий аргумент.

Программист предполагает, что оператор sizeof вычислит размер строкового литерала. Но, на самом деле, вновь вычисляется размер указателя.

Везение в том, что строка состоит из 8 символов, и её размер совпадает с размером указателя, если происходит сборка 64-битного приложения. В результате все 8 символов строки "No code." будут успешно скопированы.

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

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

static const char initial_state [] = "No code.";

Теперь значение sizeof(initial_state) равно 9.

Сценарий 2. Терминальный ноль вовсе и не требуется. Например, ниже можно увидеть вот такую строчку кода:

UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);

Как видите, функция strlen вернёт значение 8 и терминальный ноль не участвует в сравнении. Тогда это действительно везение и всё хорошо.

Побитовый сдвиг

Следующий пример связан с операцией побитового сдвига.

static intclo_parse_single_uint(struct benchmark_clo *clo, const char *arg, void *ptr){  ....  uint64_t tmax = ~0 >> (64 - 8 * clo->type_uint.size);  ....}

Предупреждение PVS-Studio: V610 [CWE-758] Unspecified behavior. Check the shift operator '>>'. The left operand '~0' is negative. clo.cpp 205

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

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

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

#define BTT_CREATE_DEF_SIZE  (20 * 1UL << 20) /* 20 MB */

Предупреждение PVS-Studio: V634 [CWE-783] The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. bttcreate.c 204

Чтобы получить константу, равную значению 20 MB, программист решил выполнить следующие действия:

• Сдвинул 1 на 20 разрядов, чтобы получить значение 1048576, т.е. 1 MB.
• Умножил 1 MB на 20.

Другими словами, программист думает, что вычисления происходят так: (20 * (1UL << 20))

Но на самом деле приоритет оператора умножения выше, чем приоритет оператора сдвига и выражение вычисляется так: ((20 * 1UL) << 20).

Согласитесь, вряд ли программист хотел, чтобы выражение вычислилось в такой последовательности. Нет смысла в умножении 20 на 1. Так что перед нами тот случай, когда код работает не так, как задумывал программист.

Но эта ошибка никак не проявит себя. Неважно, как написать:

• (20 * 1UL << 20)
• (20 * (1UL << 20))
• ((20 * 1UL) << 20)

Результат всё равно всегда одинаковый! Всегда получается нужное значение 20971520 и программа работает совершенно корректно.

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

Не там поставленная скобка

#define STATUS_INFO_LENGTH_MISMATCH 0xc0000004static voidenum_handles(int op){  ....  NTSTATUS status;  while ((status = NtQuerySystemInformation(      SystemExtendedHandleInformation,      hndl_info, hi_size, &req_size)        == STATUS_INFO_LENGTH_MISMATCH)) {    hi_size = req_size + 4096;    hndl_info = (PSYSTEM_HANDLE_INFORMATION_EX)REALLOC(hndl_info,        hi_size);  }  UT_ASSERT(status >= 0);  ....}

Предупреждение PVS-Studio: V593 [CWE-783] Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. ut.c 641

Внимательно посмотрите вот сюда:

while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))

Программист хотел сохранить в переменной status значение, которое возвращает функцию NtQuerySystemInformation, а затем сравнить его с константой.

Программист скорее всего знал, что приоритет оператора сравнения (==) выше, чем у оператора присваивания (=), и поэтому следует использовать скобки. Но опечатался и поставил их не там, где надо. В результате скобки никак не помогают. Корректный код:

while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)

Из-за этой ошибки, макрос UT_ASSERT никогда не сработает. Ведь в переменную status всегда заносится результат сравнения, то есть ложь (0) или истина (1). Значит условие ([0..1] >= 0) всегда истинно.

Потенциальная утечка памяти

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)    return POCLI_ERR_PARS;  ....}

Предупреждение PVS-Studio: V773 [CWE-401] The function was exited without releasing the 'input' pointer. A memory leak is possible. pmemobjcli.c 238

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

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  if (!oidp)    return POCLI_ERR_PARS;  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  ....}

Или можно явно освобождать память:

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)  {    free(input);    return POCLI_ERR_PARS;  }  ....}

Потенциальное переполнение

typedef long long os_off_t;voiddo_memcpy(...., int dest_off, ....., size_t mapped_len, .....){  ....  LSEEK(fd, (os_off_t)(dest_off + (int)(mapped_len / 2)), SEEK_SET);  ....}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands, not the result. memcpy_common.c 62

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

Думаю, правильнее будет написать так:

LSEEK(fd, dest_off + (os_off_t)(mapped_len) / 2, SEEK_SET);

Здесь беззнаковое значение типа size_t превращается в знаковое (чтоб не было какого-нибудь предупреждения от компилятора). И заодно точно не возникнет переполнение при сложении.

Неправильная защита от переполнения

static DWORDget_rel_wait(const struct timespec *abstime){  struct __timeb64 t;  _ftime64_s(&t);  time_t now_ms = t.time * 1000 + t.millitm;  time_t ms = (time_t)(abstime->tv_sec * 1000 +    abstime->tv_nsec / 1000000);  DWORD rel_wait = (DWORD)(ms - now_ms);  return rel_wait < 0 ? 0 : rel_wait;}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359

Мне не очень понятно, от какой именно ситуации должна защищать проверка, но она в любом случае не работает. Переменная rel_wait имеет беззнаковый тип DWORD. А значит, сравнение rel_wait < 0 не имеет смысла, так как результатом всегда является истина.

Отсутствие проверки, что память успешно выделена

Проверка того, что память выделена, осуществляется с помощью макросов assert, которые ничего не делают, если компилируется Release версия приложения. Так что можно сказать, что нет никакой обработки ситуации, когда функции malloc возвращают NULL. Пример:

static voidremove_extra_node(TOID(struct tree_map_node) *node){  ....  unsigned char *new_key = (unsigned char *)malloc(new_key_size);  assert(new_key != NULL);  memcpy(new_key, D_RO(tmp)->key, D_RO(tmp)->key_size);  ....}

Предупреждение PVS-Studio: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 340, 338. rtree_map.c 340

В других местах нет даже assert:

static voidcalc_pi_mt(void){  ....  HANDLE *workers = (HANDLE *) malloc(sizeof(HANDLE) * pending);  for (i = 0; i < pending; ++i) {    workers[i] = CreateThread(NULL, 0, calc_pi,      &tasks[i], 0, NULL);    if (workers[i] == NULL)      break;  }  ....}

Предупреждение PVS-Studio: V522 [CWE-690] There might be dereferencing of a potential null pointer 'workers'. Check lines: 126, 124. pi.c 126

Таких фрагментов кода я насчитал минимум 37 штук. Так что я не вижу смысла перечислять все их в статье.

На первый взгляд, отсутствие проверок можно посчитать просто неаккуратностью и сказать, что это код с запахом. Я не согласен с такой позицией. Программисты недооценивают опасность отсутствия таких проверок. Нулевой указатель вовсе не обязательно сразу проявит себя падением программы при попытке его разыменования. Последствия могут быть более причудливыми и опасными, особенно в многопоточных программах. Чтобы подробнее понять, в чём дело и почему проверки нужны, настоятельно рекомендую всем познакомиться со статьёй "Почему важно проверять, что вернула функция malloc".

Код с запахом

Двойной вызов CloseHandle

static voidprepare_map(struct pmem2_map **map_ptr,  struct pmem2_config *cfg, struct pmem2_source *src){  ....  HANDLE mh = CreateFileMapping(....);  ....  UT_ASSERTne(CloseHandle(mh), 0);  ....}

Предупреждение PVS-Studio: V586 [CWE-675] The 'CloseHandle' function is called twice for deallocation of the same resource. pmem2_map.c 76

Смотря на этот код и предупреждение PVS-Studio понятно, что ничего непонятно. Где тут возможен повторный вызов CloseHandle? Чтобы найти ответ, давайте посмотрим на реализацию макроса UT_ASSERTne.

#define UT_ASSERTne(lhs, rhs)\  do {\    /* See comment in UT_ASSERT. */\    if (__builtin_constant_p(lhs) && __builtin_constant_p(rhs))\      UT_ASSERT_COMPILE_ERROR_ON((lhs) != (rhs));\    UT_ASSERTne_rt(lhs, rhs);\  } while (0)

Сильно понятнее не стало. Что такое UT_ASSERT_COMPILE_ERROR_ON? Что такое UT_ASSERTne_rt?

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

do {  if (0 && 0) (void)((CloseHandle(mh)) != (0));  ((void)(((CloseHandle(mh)) != (0)) ||    (ut_fatal(".....", 76, __FUNCTION__, "......: %s (0x%llx) != %s (0x%llx)",              "CloseHandle(mh)", (unsigned long long)(CloseHandle(mh)), "0",              (unsigned long long)(0)), 0))); } while (0);

Удалим всегда ложное условие (0 && 0) и, вообще, всё не относящееся к делу. Получается:

((void)(((CloseHandle(mh)) != (0)) ||  (ut_fatal(...., "assertion failure: %s (0x%llx) != %s (0x%llx)",            ....., (unsigned long long)(CloseHandle(mh)), .... ), 0)));

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

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

Несоответствие интерфейса реализации (снятие константности)

static intstatus_push(PMEMpoolcheck *ppc, struct check_status *st, uint32_t question){  ....  } else {    status_msg_info_and_question(st->msg);            // <=    st->question = question;    ppc->result = CHECK_RESULT_ASK_QUESTIONS;    st->answer = PMEMPOOL_CHECK_ANSWER_EMPTY;    PMDK_TAILQ_INSERT_TAIL(&ppc->data->questions, st, next);  }  ....}

Анализатор выдаёт сообщение: V530 [CWE-252] The return value of function 'status_msg_info_and_question' is required to be utilized. check_util.c 293

Причина в том, что функция status_msg_info_and_question, с точки зрения анализатора, не меняет состояние внешних по отношению к ней объектов, в том числе переданную константную строчку. Т.е. функция просто что-то считает и возвращает результат. А раз так, то странно не использовать результат, который возвращает эта функция. И, хотя анализатор в этот раз не прав, он указывает на код с запахом. Давайте посмотрим, как устроена вызываемая функция status_msg_info_and_question.

static inline intstatus_msg_info_and_question(const char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

При вызове функции strchr происходит неявное снятие константности. Дело в том, что в C она объявлена так:

char * strchr ( const char *, int );

Не лучшее решение. Но язык C такой, какой есть :).

Анализатор запутался и не понял, что переданная строка на самом деле изменяется. А раз так, то возвращаемое значения не самое главное и его можно не использовать.

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

static inline intstatus_msg_info_and_question(char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

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

Переусложненный код

static struct memory_blockheap_coalesce(struct palloc_heap *heap,  const struct memory_block *blocks[], int n){  struct memory_block ret = MEMORY_BLOCK_NONE;  const struct memory_block *b = NULL;  ret.size_idx = 0;  for (int i = 0; i < n; ++i) {    if (blocks[i] == NULL)      continue;    b = b ? b : blocks[i];    ret.size_idx += blocks[i] ? blocks[i]->size_idx : 0;  }  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'blocks[i]' is always true. heap.c 1054

Если blocks[i] == NULL, то сработает оператор continue и цикл начнёт следующую итерацию. Поэтому повторная проверка элемента blocks[i] не имеет смысла и тернарный оператор является лишним. Код можно упростить:

....for (int i = 0; i < n; ++i) {  if (blocks[i] == NULL)    continue;  b = b ? b : blocks[i];  ret.size_idx += blocks[i]->size_idx;}....

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

void win_mmap_fini(void){  ....  if (mt->BaseAddress != NULL)    UnmapViewOfFile(mt->BaseAddress);  size_t release_size =    (char *)mt->EndAddress - (char *)mt->BaseAddress;  void *release_addr = (char *)mt->BaseAddress + mt->FileLen;  mmap_unreserve(release_addr, release_size - mt->FileLen);  ....}

Предупреждение PVS-Studio: V1004 [CWE-119] The '(char *) mt->BaseAddress' pointer was used unsafely after it was verified against nullptr. Check lines: 226, 235. win_mmap.c 235

Указатель mt->BaseAddress может быть нулевым, о чём свидетельствует проверка:

if (mt->BaseAddress != NULL)

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

size_t release_size =  (char *)mt->EndAddress - (char *)mt->BaseAddress;

Будет получено некое большое целочисленное значение, равное фактически значению указателя mt->EndAddress. Возможно, это и не ошибка, но выглядит всё это очень подозрительно, и мне кажется, код следует перепроверить. Запах заключается в том, что код непонятен и ему явно не хватает поясняющих комментариев.

Короткие имена глобальных переменных

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

static struct critnib *c;

Предупреждения PVS-Studio на такие переменные:

• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'ri' variable. map.c 131
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'c' variable. obj_critnib_mt.c 56
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.h 68
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.c 34

Странное

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

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, dstshadow + dest_off, bytes / 2);  verify_contents(file_name, 0, dstshadow, dst, bytes);  verify_contents(file_name, 1, srcshadow, src, bytes);  ....}

Предупреждение PVS-Studio: V549 [CWE-688] The first argument of 'memmove' function is equal to the second argument. memmove_common.c 71

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

• Хотелось "потрогать" блок памяти. Но произойдёт ли это в реальности? Не удалит ли оптимизирующий компилятор код, который копирует блок памяти сам в себя?
• Это какой-то юнит-тест на работу функции memmove.
• Код содержит опечатку.

А вот не менее странный фрагмент в этой же функции:

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, srcshadow + src_off, 0);  verify_contents(file_name, 2, dstshadow, dst, bytes);  verify_contents(file_name, 3, srcshadow, src, bytes);  ....}

Предупреждение PVS-Studio: V575 [CWE-628] The 'memmove' function processes '0' elements. Inspect the third argument. memmove_common.c 82

Функция перемещает 0 байт. Что это? Юнит-тест? Опечатка?

Для меня этот код непонятен и странен.

Зачем использовать анализаторы кода?

Может показаться, что раз найдено мало ошибок, то и внедрение анализатора в процесс разработки кода малообоснованно. Но смысл использования инструментов статического анализа не в разовых проверках, а в регулярном выявлении ошибок ещё на этапе написания кода. В противном случае, эти ошибки выявляются более дорогими и медленными способами (отладка, тестирование, отзывы пользователей и так далее). Более подробно эта мысль изложена в статье "Ошибки, которые не находит статический анализ кода, потому что он не используется", с которой я рекомендую познакомиться. А затем приходите к нам на сайт скачать и попробовать PVS-Studio для проверки своих проектов.

Спасибо за внимание!


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Static code analysis of the PMDK library collection by Intel and errors that are not actual errors.

Совсем недавно произошло знаменательное событие: PVS-Studio появился в Compiler Explorer! Теперь вы можете быстро и легко проанализировать код на наличие ошибок прямо на сайте godbolt.org (Compiler Explorer). Это нововведение открывает большое количество новых возможностей от утоления любопытства по поводу способностей анализатора до возможности быстро поделиться результатом проверки с другом. О том, как использовать эти возможности, и пойдёт речь в этой статье. Осторожно большие гифки!

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

Теперь давайте обо всём по порядку. Compiler Explorer интерактивный онлайн-сервис для исследования компиляторов. Здесь вы можете прямо на сайте писать код и сразу видеть, какой ассемблерный вывод сгенерирует для него тот или иной компилятор:


Помимо этого, на сайте существует еще целый набор возможностей, и сегодня мы рассмотрим возможность проведения анализа с помощью PVS-Studio. Для того, чтобы провести анализ, необходимо сделать следующее:

1. Зайти на сайт godbolt.org,
2. Во вкладке с выводом компилятора нажать "Add tool...",
3. В выпадающем списке выбрать "PVS-Studio".

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


Готово! Теперь в появившемся окне будут автоматически выводиться все предупреждения, которые выдаются анализатором на ваш код. Анализировать можно как собственноручно написанный код, так и исходные файлы, загруженные по отдельности. Естественно, файлы должны быть самодостаточными и не использовать include на файлы, не входящие в стандартную библиотеку. В противном случае файл не получится ни скомпилировать, ни проверить.

На данный момент анализ с помощью PVS-Studio доступен на сайте для всех версий GCC и Clang под платформы x86 и x64. Мы планируем расширить возможности сайта и на другие поддерживаемые нами компиляторы (например, MSVC или компиляторы для ARM), если на это будет спрос.

Сейчас на сайте включены только General-диагностики уровней error, warning и note. Мы специально не стали включать остальные режимы (Optimization, 64-bit, Custom и MISRA), чтобы в выводе остались только самые важные предупреждения. Также, в отличие от самого PVS-Studio, Compiler Explorer пока не поддерживает языки C# и Java мы планируем запустить анализ кода на этих языках, как только они там появятся :)

Compiler Explorer имеет весьма умную систему окон, поэтому вы можете двигать их или, например, накладывать друг на друга. Если сейчас вас не интересует вывод компилятора, его можно "спрятать". Вот так:


Вы можете как сразу писать код в окне Compiler Explorer, так и загружать отдельные файлы. Для этого нужно нажать "Save/Load" и в открывшейся вкладке выбрать "File system". Также можно "скачать" написанный вами код на компьютер, нажав Ctrl + S.


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

Если вам хочется увидеть вывод вашей программы, то можно открыть окно выполнения, нажав "Add new > Execution only" в окне для написания кода (не в окне с компилятором). На гифке ниже вы можете увидеть вывод лабораторной работы, взятой из нашей страницы про бесплатное использование PVS-Studio студентами и преподавателями.


Кстати, вы заметили, что при переходе по ссылкам на godbolt у вас открывается уже заранее вписанный код в заранее расставленных окнах? Да, вы можете генерировать постоянные ссылки, полностью сохраняющие состояние страницы в момент генерации! Для этого вам нужно нажать на кнопку "Share" в правом верхнем углу экрана.


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

Мы планируем использовать эту возможность для работы с клиентами в техподдержке: зачастую использование Compiler Explorer является очень удобным для составления минимально воспроизводимых примеров, которые можно быстро взять и отправить по почте.

Также в выпадающей вкладке "Share" есть пункт создания Embedded-ссылки, с помощью которой можно встроить окно с Compiler Explorer на какой-нибудь другой сайт.

В Compiler Explorer всегда находится актуальная версия PVS-Studio, поэтому после каждого нашего релиза на сайте можно будет найти всё больше и больше ошибок. Тем не менее, использование PVS-Studio на godbolt.org не дает полного представления о его возможностях, ведь PVS-Studio это не только диагностики, но и развитая инфраструктура:

• Анализ кода на языках C, C++, C# и Java для куда большего количества платформ и компиляторов;
• Плагины для Visual Studio 2010-2019, JetBrains Rider, IntelliJ IDEA;
• Возможность интеграции в TeamCity, PlatformIO, Azure DevOps, Travis CI, CircleCI, GitLab CI/CD, Jenkins, SonarQube и т.д.
• Утилита мониторинга компиляции для проведения анализа независимо от IDE или сборочной системы;
• И многое, многое другое.

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

Чтобы всегда быть в курсе, следите за нашими новостями. Также читайте наш блог: там мы публикуем не только новости и статьи о поиске багов в реальных проектах, но и различные интересные моменты, связанные с C, C++, C# и Java.

Наши соцсети:

• Twitter @pvsstudio_rus
• Вконтакте Анализатор PVS-Studio
• Facebook @StaticCodeAnalyzer
• Telegram PVS-Studio rus

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. PVS-Studio is now in Compiler Explorer!.

Устав от нескончаемого code review или отладки, временами задумываешься, как бы упростить себе жизнь. И немного поискав, ну или случайно наткнувшись, можно увидеть волшебное словосочетание: "Статический анализ". Давайте посмотрим, что это такое и как он может взаимодействовать с вашим проектом.


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


В этом выводе мы видим, что переменная var так и не была использована нигде в функции. Так что на самом деле вы почти всегда пользовались простеньким статическим анализатором кода. Однако, в отличие от профессиональных анализаторов, таких как Coverity, Klocwork или PVS-Studio, предоставляемые компилятором предупреждения могут указывать только на небольшой спектр проблем.

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

Зачем нужен статический анализ?

В двух словах: ускорение и упрощение.

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

auto x = obj.x;auto y = obj.y;auto z = obj.z;

Вы написали следующий код:

auto x = obj.x;auto y = obj.y;auto z = obj.x;

Как видите, в последней строке появилась опечатка. Например, PVS-Studio выдаёт следующее предупреждение:

V537 Consider reviewing the correctness of 'y' item's usage.

Если хотите потыкать в эту ошибку руками, то попробуйте готовый пример на Compiler Explorer: *клик*.

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

Однако это явная ошибка. А если разработчик написал неоптимальный код из-за того, что позабыл какую-либо тонкость языка? Или же вовсе допустил в коде undefined behavior? К сожалению, подобные случаи совершенно обыденны и львиная часть времени тратится на то, чтобы отладить специфично работающий код, который содержит опечатки, типичные ошибки или undefined behavior.

Именно для этих ситуаций и появился статический анализ. Это помощник для разработчика, который укажет ему на различные проблемы в коде и объяснит в документации почему так писать не нужно, к чему это может привести и как это исправить. Вот пример как это может выглядеть: *клик*.

Больше интересных ошибок, которые может обнаружить анализатор, вы можете найти в статьях:

• Топ 10 ошибок в проектах C++ за 2019 год
• Топ 10 ошибок в проектах C# за 2019 год
• Топ 10 ошибок в проектах Java за 2019 год

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

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

0. Знакомство с инструментом

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

Что вы узнаете на этом этапе:

• Какие есть способы взаимодействия с анализатором;
• Совместим ли анализатор с вашей средой разработки;
• Какие проблемы есть сейчас в ваших проектах.

После того, как вы установили себе всё необходимое, то первым делом стоит запустить анализ всего проекта (Windows, Linux, macOS). В случае с PVS-Studio в Visual Studio вы увидите подобную картину (кликабельно):


Дело в том, что обычно на проекты с большой кодовой базой статические анализаторы выдают огромное количество предупреждений. Нет необходимости исправлять их все, так как ваш проект уже работает, а значит эти проблемы не являются критичными. Однако вы можете посмотреть на самые интересные предупреждения и исправить их при необходимости. Для этого нужно отфильтровать вывод и оставить только наиболее достоверные сообщения. В плагине PVS-Studio для Visual Studio это делается фильтрацией по уровням и категориям ошибок. Для наиболее точного вывода оставьте включёнными только High и General (тоже кликабельно):


Действительно, 178 предупреждений просмотреть значительно проще, чем несколько тысяч

Во вкладках Medium и Low часто попадаются хорошие предупреждения, однако в эти категории занесены те диагностики, которые имеют меньшую точность (достоверность). Подробнее про уровни предупреждений и варианты работы под Windows можно посмотреть тут: *клик*.

Успешно просмотрев самые интересные ошибки (и успешно исправив их) стоит подавить оставшиеся предупреждения. Это нужно для того, чтобы новые предупреждения не терялись среди старых. К тому же статический анализатор это помощник для программиста, а не список для багов. :)

1. Автоматизация

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

Что вы узнаете на данном этапе:

• Какие варианты автоматизации предоставляет инструмент;
• Совместим ли анализатор с вашей сборочной системой.

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

А теперь приступим к сервисам непрерывной интеграции (CI). Любой анализатор можно внедрить в них без каких-либо серьезных проблем. Для этого нужно создать отдельный этап в pipeline, который обычно находится после сборки и юнит-тестов. Делается это при помощи различных консольных утилит. Например, PVS-Studio предоставляет следующие утилиты:

• PVS-Studio_Cmd.exe (анализ решений, C#, C++ проектов на Windows)
• CLMonitor.exe (мониторинг компиляции)
• pvs-studio-analyzer (анализ C++ проектов на Linux / macOS)
• pvs-studio-dotnet (анализ решений, C# проектов на Linux / macOS)
• pvs-studio.jar (анализ Java проектов)
• PlogConverter (конвертер файлов отчёта)

Для интеграции анализа в CI нужно сделать три вещи:

• Установить анализатор;
• Запустить анализ;
• Доставить результаты.

Например, для установки PVS-Studio на Linux (Debian-base) нужно выполнить следующие команды:

wget -q -O - https://files.viva64.com/etc/pubkey.txt \    | sudo apt-key add -sudo wget -O /etc/apt/sources.list.d/viva64.list \  https://files.viva64.com/etc/viva64.list  sudo apt-get update -qqsudo apt-get install -qq pvs-studio

В системах под управлением Windows отсутствует возможность установить анализатор из пакетного менеджера, однако есть возможность развернуть анализатор из командной строки:

PVS-Studio_setup.exe /verysilent /suppressmsgboxes /norestart /nocloseapplications

Подробнее о развёртывании PVS-Studio в системах под управлением Windows можно почитать *тут*.

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

Так как способ запуска зависит от платформы и особенностей проекта, я покажу вариант для C++ (Linux) в качестве примера:

pvs-studio-analyzer analyze -j8 \                            -o PVS-Studio.logplog-converter -t errorfile PVS-Studio.log --cerr -w

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

Примечание. Текстовый формат это неудобно. Он приводится просто для примера. Обратите внимание на более интересный формат отчёта FullHtml. Он позволяет осуществлять навигацию по коду.

Подробнее про настройку анализа на CI можно прочитать в статье "PVS-Studio и Continuous Integration" (Windows) или "Как настроить PVS-Studio в Travis CI" (Linux).

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

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

git diff --name-only HEAD origin/$MERGE_BASE > .pvs-pr.list

Теперь нужно передать анализатору на вход этот список файлов. Например, в PVS-Studio это реализовано при помощи флага -S:

pvs-studio-analyzer analyze -j8 \                            -o PVS-Studio.log \                            -S .pvs-pr.list

Подробнее про анализ pull request'ов можно узнать *тут*. Даже если вашего CI нет в списке указанных в статье сервисов, вам будет полезен общий раздел, посвященный теории этого типа анализа.

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

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

2. Интеграция на машины разработчиков

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

Как самый простой вариант разработчики сами могут установить необходимый анализатор. Однако это займёт много времени и отвлечёт их от разработки, поэтому вы можете автоматизировать этот процесс, используя установщик и нужные флаги. Для PVS-Studio есть различные флаги для автоматизированной установки. Впрочем, всегда есть пакетные менеджеры, например, Chocolatey (Windows), Homebrew (macOS) или десятки вариантов для Linux.

Затем нужно будет установить необходимые плагины, например, для Visual Studio, IDEA, Rider etc.

3. Ежедневное использование

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

В случае, если анализатор обнаружит в недавно измененном коде проблемы, то сообщит об этом самостоятельно. Например, PVS-Studio скажет вам об этом при помощи оповещения:


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

• Как запустить PVS-Studio в Windows (C, C++, C#)
• Как запустить PVS-Studio в Linux и macOS (C, C++)
• Как запустить PVS-Studio Java

Подобные статьи дают всю необходимую для повседневного использования информацию и не отнимают много времени. :)

Ещё на этапе знакомства с инструментом мы подавили очень много предупреждений во время одного из первых запусков. Увы, но статические анализаторы не идеальны, поэтому время от времени выдают ложные срабатывания. Подавить их обычно легко, например в плагине PVS-Studio для Visual Studio достаточно нажать на одну кнопку:


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

После интеграции

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

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Maxim Zvyagintsev. Static Analysis: From Getting Started to Integration.

Приложения, использующиеся в области Big Data, обрабатывают огромные объемы информации, причем часто это происходит в реальном времени. Естественно, такие приложения должны обладать высокой надежностью, чтобы никакая ошибка в коде не могла помешать обработке данных. Для достижения высокой надежности необходимо пристально следить за качеством кода проектов, разрабатываемых для этой области. Решением данной проблемы и занимается статический анализатор PVS-Studio. Сегодня в качестве подопытного для анализатора был выбран проект Apache Flink, разработанный организацией Apache Software Foundation одним из лидеров на рынке ПО для Big Data.

Что же такое Apache Flink? Это open-source фреймворк для распределенной обработки больших объемов данных. Он был разработан как альтернатива Hadoop MapReduce в 2010 году в Техническом университете Берлина. Основу фреймворка составляет движок распределенного исполнения приложений пакетной и потоковой обработки данных. Написан этот движок на языках Java и Scala. Сегодня Apache Flink возможно использовать в проектах, написанных с использованием языков Java, Scala, Python и даже SQL.

Анализ проекта

Загрузив исходный код проекта, я запустил сборку проекта командой 'mvn clean package -DskipTests', указанной в инструкции на GitHub. Пока шла сборка, я при помощи утилиты CLOC выяснил что в проекте 10838 Java-файлов, в которых имеется около 1.3 миллионов строк кода. Причем тестовых Java файлов оказалось аж 3833 штуки, а это больше 1/3 всех Java файлов. Также я заметил, что в проекте используется статический анализатор кода FindBugs и утилита Cobertura, предоставляющая информацию о покрытии кода тестами. Учитывая все это, становится ясно, что разработчики Apache Flink тщательно следили за качеством кода и покрытием тестами при разработке.

После удачной сборки я открыл проект в IntelliJ IDEA и запустил анализ при помощи плагина PVS-Studio for IDEA and Android Studio. Предупреждения анализатора распределились следующим образом:

• 183 High;
• 759 Medium;
• 545 Low.

Примерно 2/3 срабатываний анализатора PVS-Studio выданы на тестовые файлы. Если учесть этот факт и размер кодовой базы проекта, то можно сказать, что разработчикам Apache Flink удалось сохранить качество кода на высоте.

Изучив предупреждения анализатора более подробно, я выбрал самые интересные на мой взгляд. Так давайте же посмотрим, что же удалось найти PVS-Studio в данном проекте!

Всего лишь немного невнимательности

V6001 There are identical sub-expressions 'processedData' to the left and to the right of the '==' operator. CheckpointStatistics.java(229)

@Overridepublic boolean equals(Object o) {  ....  CheckpointStatistics that = (CheckpointStatistics) o;  return id == that.id &&    savepoint == that.savepoint &&    triggerTimestamp == that.triggerTimestamp &&    latestAckTimestamp == that.latestAckTimestamp &&    stateSize == that.stateSize &&    duration == that.duration &&    alignmentBuffered == that.alignmentBuffered &&    processedData == processedData &&                // <=    persistedData == that.persistedData &&    numSubtasks == that.numSubtasks &&    numAckSubtasks == that.numAckSubtasks &&    status == that.status &&    Objects.equals(checkpointType, that.checkpointType) &&    Objects.equals(      checkpointStatisticsPerTask,       that.checkpointStatisticsPerTask);}

На фоне других выражений в return данная ошибка не сильно бросается в глаза. При переопределении метода equals для класса CheckpointStatistics программист допустил ошибку в выражении processedData == processedData, которое не имеет смысла, потому что всегда истинно. Аналогично остальным выражениям в return должны были сравниваться поля текущего объекта this и объекта that: processedData == that.processedData. Данная ситуация это один из типичных паттернов ошибок, найденных в функциях сравнения, которые подробно описаны в статье "Зло живет в функциях сравнения". Вот так и получается, что всего лишь "немного невнимательности" сломало логику проверки эквивалентности объектов класса CheckpointStatistics.

Выражение всегда истинно

V6007 Expression 'input2.length > 0' is always true. Operator.java(283)

public static <T> Operator<T> createUnionCascade(Operator<T> input1,                                                  Operator<T>... input2) {  if (input2 == null || input2.length == 0)   {    return input1;                                // <=  }   else if (input2.length == 1 && input1 == null)   {    return input2[0];  }  ....  if (input1 != null)   {    ....  }   else if (input2.length > 0 && input2[0] != null) // <=  {    ....  }   else   {    ...  }}

В этом методе анализатор оказался внимательнее человека, о чем и решил сообщить в своей своеобразной манере, указав на то, что выражение input2.length > 0 всегда будет истинно. Причина в том, что если длина массива input2 будет равна 0, то условие input2 == null || input2.length == 0 первого if в методе будет истинно, и выполнение метода будет прервано, так и не дойдя до строчки с выражением input2.length > 0.

Всевидящий анализатор

V6007 Expression 'slotSharingGroup == null' is always false. StreamGraphGenerator.java(510)

private <T> Collection<Integer> transformFeedback(...){  ....  String slotSharingGroup = determineSlotSharingGroup(null, allFeedbackIds);  if (slotSharingGroup == null)  {    slotSharingGroup = "SlotSharingGroup-" + iterate.getId();  }  ....}

Анализатор сообщил, что выражение slotSharingGroup == null всегда ложно. А это наталкивает на мысль о том, что метод determineSlotSharingGroup никогда не вернет null. Неужели анализатор настолько умный, что смог вычислить все значения, которые может вернуть этот метод? Давайте-ка лучше все перепроверим сами:

public class StreamGraphGenerator {  ....  public static final String DEFAULT_SLOT_SHARING_GROUP = "default";  ....  private String determineSlotSharingGroup(String specifiedGroup,                                            Collection<Integer> inputIds)   {    if (specifiedGroup != null)    {      return specifiedGroup; // <= 1    }    else    {      String inputGroup = null;      for (int id: inputIds)      {        String inputGroupCandidate = streamGraph.getSlotSharingGroup(id);        if (inputGroup == null)        {          inputGroup = inputGroupCandidate;        }        else if (!inputGroup.equals(inputGroupCandidate))        {          return DEFAULT_SLOT_SHARING_GROUP; // <= 2        }      }      return inputGroup == null              ? DEFAULT_SLOT_SHARING_GROUP              : inputGroup; // <= 3    }  }  ...}

По порядку пройдемся по всем return и посмотрим, что же может вернуть данный метод:

• В первом return вернется аргумент метода specifiedGroup, но только если он не будет равен null.
• return в цикле for вернет значение статического финального поля DEFAULT_SLOT_SHARING_GROUP, инициализированного строковым литералом;
• И последний return в методе вернет значение переменной inputGroup, если оно не будет равно null. В противном случае вернется значение поля DEFAULT_SLOT_SHARING_GROUP.

Получается, что анализатор действительно смог вычислить невозможность возврата null из метода determineSlotSharingGroup и предупредил нас об этом, указав на бессмысленность проверки slotSharingGroup == null. И хотя данная ситуация не является ошибочной, однако подобная дополнительная защита анализатора сможет обнаружить ошибку в каком-нибудь другом случае. Например, когда необходимо, чтобы метод возвращал null при определенных условиях.

Собери их всех

V6007 Expression 'currentCount <= lastEnd' is always true. CountSlidingWindowAssigner.java(75)

V6007 Expression 'lastStart <= currentCount' is always true. CountSlidingWindowAssigner.java(75)

@Overridepublic Collection<CountWindow> assignWindows(....) throws IOException {  Long countValue = count.value();  long currentCount = countValue == null ? 0L : countValue;  count.update(currentCount + 1);  long lastId = currentCount / windowSlide;  long lastStart = lastId * windowSlide;  long lastEnd = lastStart + windowSize - 1;  List<CountWindow> windows = new ArrayList<>();  while (lastId >= 0 &&          lastStart <= currentCount &&          currentCount <= lastEnd)   {    if (lastStart <= currentCount && currentCount <= lastEnd) // <=    {      windows.add(new CountWindow(lastId));    }    lastId--;    lastStart -= windowSlide;    lastEnd -= windowSlide;  }  return windows;}

Анализатор предупреждает, что выражения currentCount <= lastEnd и lastStart <= currentCount всегда истинны. И ведь действительно, если посмотреть на условие цикла while, то там имеются точно такие же выражения. Это значит, что внутри цикла эти выражения всегда будут истинны, поэтому в список windows будут добавлены все объекты типа CountWindow созданные в цикле. Вариантов появления этой бессмысленной проверки множество, и первое, что приходит в голову, либо артефакт рефакторинга, либо перестраховка разработчика. Но это может быть и ошибка, если хотелось проверить что-то иное...

Некорректный порядок аргументов

V6029 Possible incorrect order of arguments passed to method: 'hasBufferForReleasedChannel', 'hasBufferForRemovedChannel'. NettyMessageClientDecoderDelegateTest.java(165), NettyMessageClientDecoderDelegateTest.java(166)

private void testNettyMessageClientDecoding(       boolean hasEmptyBuffer,       boolean hasBufferForReleasedChannel,       boolean hasBufferForRemovedChannel) throws Exception {  ....  List<BufferResponse> messages = createMessageList (    hasEmptyBuffer,    hasBufferForReleasedChannel,    hasBufferForRemovedChannel);  ....}

Отсутствие в Java возможности вызова метода с именованными параметрами иногда играет злую шутку с разработчиками. Именно это и произошло при вызове метода createMessageList, на который указал анализатор. При взгляде на определение этого метода становится ясно, что параметр hasBufferForRemovedChannel должен передаваться в метод перед параметром hasBufferForReleasedChannel:

private List<BufferResponse> createMessageList(  boolean hasEmptyBuffer,  boolean hasBufferForRemovedChannel,  boolean hasBufferForReleasedChannel) {  ....  if (hasBufferForReleasedChannel) {    addBufferResponse(messages,                       releasedInputChannelId,                       Buffer.DataType.DATA_BUFFER,                       BUFFER_SIZE,                       seqNumber++);  }  if (hasBufferForRemovedChannel) {    addBufferResponse(messages,                       new InputChannelID(),                       Buffer.DataType.DATA_BUFFER,                       BUFFER_SIZE,                       seqNumber++);  }  ....  return messages;}

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

Ох уж этот копипаст

V6032 It is odd that the body of method 'seekToFirst' is fully equivalent to the body of another method 'seekToLast'. RocksIteratorWrapper.java(53), RocksIteratorWrapper.java(59)

public class RocksIteratorWrapper implements RocksIteratorInterface, Closeable {  ....  private RocksIterator iterator;  ....  @Override  public void seekToFirst() {    iterator.seekToFirst(); // <=    status();   }    @Override  public void seekToLast() {    iterator.seekToFirst();  // <=    status();  }    ....}

Тела методов seekToFirst и seekToLast совпадают. Причем оба метода используются в коде.

Что-то здесь нечисто! И действительно, если посмотреть какие методы имеются у объекта iterator, то станет понятно, какую ошибку помог найти анализатор:

public class RocksIterator extends AbstractRocksIterator<RocksDB>{  ....}public abstract class AbstractRocksIterator<...> extends ...{  ....  public void seekToFirst() // <=  {    assert this.isOwningHandle();    this.seekToFirst0(this.nativeHandle_);  }    public void seekToLast() // <=  {    assert this.isOwningHandle();    this.seekToLast0(this.nativeHandle_);  }  ....}

Получается, что метод seekToLast класса RocksIteratorWrapper был создан копипастом метода seekToFirst этого же класса. Однако по каким-то причинам разработчик забыл заменить вызов метода seekToFirst у iterator на seekToLast.

Путаница с форматными строками

V6046 Incorrect format. A different number of format items is expected. Arguments not used: 1. UnsignedTypeConversionITCase.java(102)

public static void prepareMariaDB() throws IllegalStateException {  ....  if (!initDbSuccess) {    throw new IllegalStateException(      String.format(        "Initialize MySQL database instance failed after {} attempts," + // <=        " please open an issue.", INITIALIZE_DB_MAX_RETRY));  }}

Форматные строки метода String.format и логгеров в Java различаются. В отличие от форматной строки метода String.format, где места подстановки аргументов указываются при помощи символа '%', в форматных строках логгеров вместо этого используется комбинация символов '{}'. Из-за этой путаницы и произошла эта ошибка. В качестве форматной строки в метод String.format передается строка, которая, скорее всего, была скопирована из другого места, где она использовалась в каком-нибудь логгере. В результате, в сообщении исключения IllegalStateException не произойдет подстановки значения поля INITIALIZE_DB_MAX_RETRY вместо '{}', и тот, кто поймает или залоггирует это исключение, так и не узнает сколько попыток подключения к БД было произведено.

Ненормальное распределение

V6048 This expression can be simplified. Operand 'index' in the operation equals 0. CollectionUtil.java(76)

public static <T> Collection<List<T>> partition(Collection<T> elements,                                                 int numBuckets) {  Map<Integer, List<T>> buckets = new HashMap<>(numBuckets);    int initialCapacity = elements.size() / numBuckets;  int index = 0;  for (T element : elements)   {    int bucket = index % numBuckets;                                 // <=    buckets.computeIfAbsent(bucket,                             key -> new ArrayList<>(initialCapacity))           .add(element);   }  return buckets.values();}

Метод partition разделяет элементы из коллекции elements на несколько сегментов, после чего возвращает эти сегменты. Однако из-за ошибки, на которую указал анализатор, никакого разделения происходить не будет. Выражение, при помощи которого определяют номер сегмента index % numBuckets, всегда будет равно 0, потому что index всегда равен 0. Изначально я подумал, что код этого метода был подвергнут рефакторингу, в результате которого забыли добавить увеличение переменной index в цикле for. Но, просмотрев коммит, где этот метод был добавлен, выяснилось, что эта ошибка появилась вместе с этим методом. Исправленный вариант кода:

public static <T> Collection<List<T>> partition(Collection<T> elements,                                                 int numBuckets) {  Map<Integer, List<T>> buckets = new HashMap<>(numBuckets);    int initialCapacity = elements.size() / numBuckets;  int index = 0;  for (T element : elements)   {    int bucket = index % numBuckets;     buckets.computeIfAbsent(bucket,                             key -> new ArrayList<>(initialCapacity))           .add(element);    index++;  }  return buckets.values();}

Несовместимый тип

V6066 The type of object passed as argument is incompatible with the type of collection: String, ListStateDescriptor<NextTransactionalIdHint>. FlinkKafkaProducer.java(1083)

public interface OperatorStateStore {  Set<String> getRegisteredStateNames();}public class FlinkKafkaProducer<IN> extends ....{  ....  private static final   ListStateDescriptor<FlinkKafkaProducer.NextTransactionalIdHint>  NEXT_TRANSACTIONAL_ID_HINT_DESCRIPTOR = ....;  @Override  public void initializeState(FunctionInitializationContext context)....   {    ....    if (context.getOperatorStateStore()               .getRegisteredStateNames()               .contains(NEXT_TRANSACTIONAL_ID_HINT_DESCRIPTOR))    // <=    {       migrateNextTransactionalIdHindState(context);    }    ....  }}

Выражение, на которое указал анализатор, всегда будет ложно, а значит вызова метода migrateNextTransactionalIdHindState никогда не произойдет. Как же так случилось, что кто-то ищет в коллекции типа Set<String> элемент совсем другого типа ListStateDescriptor<FlinkKafkaProducer.NextTransactionalIdHint>? Без помощи анализатора такая ошибка, скорее всего, очень долго бы жила в коде, так как в глаза она не бросается и без тщательной проверки данного метода её просто невозможно найти.

Неатомарное изменение переменной

V6074 Non-atomic modification of volatile variable. Inspect 'currentNumAcknowledgedSubtasks'. PendingCheckpointStats.java(131)

boolean reportSubtaskStats(JobVertexID jobVertexId, SubtaskStateStats subtask) {  TaskStateStats taskStateStats = taskStats.get(jobVertexId);  if (taskStateStats != null && taskStateStats.reportSubtaskStats(subtask)) {    currentNumAcknowledgedSubtasks++;                // <=    latestAcknowledgedSubtask = subtask;    currentStateSize += subtask.getStateSize();      // <=    long processedData = subtask.getProcessedData();    if (processedData > 0) {      currentProcessedData += processedData;         // <=    }    long persistedData = subtask.getPersistedData();    if (persistedData > 0) {      currentPersistedData += persistedData;         // <=    }    return true;  } else {    return false;  }}

Плюс еще 3 предупреждения анализатора в том же самом методе:

• V6074 Non-atomic modification of volatile variable. Inspect 'currentStateSize'. PendingCheckpointStats.java(134)
• V6074 Non-atomic modification of volatile variable. Inspect 'currentProcessedData'. PendingCheckpointStats.java(138)
• V6074 Non-atomic modification of volatile variable. Inspect 'currentPersistedData'. PendingCheckpointStats.java(143)

Анализатор подсказал, что аж 4 volatile поля в методе изменяются неатомарно. И анализатор, как всегда, оказывается прав, потому что операции ++ и +=, на самом деле, это последовательность из нескольких операций чтения-изменения-записи. Как известно, значение volatile поля видно всем потокам, а это значит, что из-за состояния гонки часть изменений поля может быть утеряна. Более подробную информацию об этом вы можете прочитать в описании диагностики.

Заключение

В Big Data проектах надежность является одним из ключевых требований, поэтому за качеством кода в них необходимо пристально следить. В этом разработчикам Apache Flink помогали несколько инструментов, а также они написали значительное количество тестов. Однако даже в таких условиях анализатор PVS-Studio смог найти ошибки. От ошибок невозможно полностью избавиться, но использование различных инструментов статического анализа кода регулярно позволит приблизится к этому идеалу. Да-да, именно регулярно. Только при регулярном использовании статический анализ показывает свою эффективность, о чём более подробно рассказано в этой статье.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valery Komarov. Big / Bug Data: Analyzing the Apache Flink Source Code.

Один из самых актуальных сценариев использования анализатора PVS-Studio его интеграция с CI системами. И хотя анализ проекта PVS-Studio практически из-под любой continuous integration системы можно встроить всего в несколько команд, мы продолжаем делать этот процесс ещё удобнее. В PVS-Studio появилась поддержка преобразования вывода анализатора в формат для TeamCity TeamCity Inspections Type. Давайте посмотрим, как это работает.

Информация об используемом ПО

PVS-Studio статический анализатор С, С++, C# и Java кода, предназначенный для облегчения задачи поиска и исправления различного рода ошибок. Анализатор можно использовать в Windows, Linux и macOS. В данной статье мы будем активно использовать не только сам анализатор, но и некоторые утилиты из его дистрибутива.

CLMonitor представляет собой сервер мониторинга, который осуществляет отслеживание запусков компиляторов. Его необходимо запустить непосредственно перед началом сборки вашего проекта. В режиме отслеживания сервер будет перехватывать запуски всех поддерживаемых компиляторов. Стоит отметить, что данную утилиту можно использовать только для анализа C/С++ проектов.

PlogConverter утилита для конвертации отчёта анализатора в разные форматы.

Информация об исследуемом проекте

Давайте попробуем данную функциональность на практическом примере проанализируем проект OpenRCT2.

OpenRCT2 открытая реализация игры RollerCoaster Tycoon 2 (RCT2), расширяющая её новыми функциями и исправляющая ошибки. Игровой процесс вращается вокруг строительства и содержания парка развлечений, в котором находятся аттракционы, магазины и объекты. Игрок должен постараться получить прибыль и поддерживать хорошую репутацию парка, сохраняя при этом гостей счастливыми. OpenRCT2 позволяет играть как в сценарии, так и в песочнице. Сценарии требуют, чтобы игрок выполнил определенную задачу в установленное время, в то время как песочница позволяет игроку построить более гибкий парк без каких-либо ограничений или финансов.

Настройка

В целях экономии времени я, пожалуй, опущу процесс установки и начну с того момента, когда у меня на компьютере запущен сервер TeamCity. Нам нужно перейти: localhost:{указанный в процессе установки порт}(в моём случае, localhost:9090) и ввести данные для авторизации. После входа нас встретит:


Нажмём на кнопку Create Project. Далее выберем Manually, заполним поля.


После нажатия на кнопку Create, нас встречает окно с настройками.


Нажмём Create build configuration.


Заполняем поля, нажимаем Create. Мы видим окно с предложением выбора системы контроля версий. Так как исходники уже лежат локально, жмём Skip.


Наконец, мы переходим к настройкам проекта.


Добавим шаги сборки, для этого жмём: Build steps -> Add build step.


Тут выберем:

• Runner type -> Command Line
• Run -> Custom Script

Так как мы будем проводить анализ во время компиляции проекта, сборка и анализ должны быть одним шагом, поэтому заполним поле Custom Script:


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

Последнее, что нам нужно сделать, установить переменные окружения, которыми я обозначил некоторые пути для улучшения их читабельности. Для этого перейдём: Parameters -> Add new parameter и добавим три переменные:


Остаётся нажать на кнопку Run в правом верхнем углу. Пока идёт сборка и анализ проекта расскажу вам о скрипте.

Непосредственно скрипт

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

choco install pvs-studio -y

Далее запустим утилиту отслеживания сборки проекта CLMonitor.

%CLmon% monitor -attach

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

%MSB% %ProjPath% /t:clean%MSB% %ProjPath% /t:rebuild /p:configuration=release%MSB% %ProjPath% /t:g2%MSB% %ProjPath% /t:PublishPortable

Введём логин и ключ лицензии PVS-Studio:

%PVS-Studio_cmd% credentials --username %PVS_Name% --serialNumber %PVS_Key%

После завершения сборки ещё раз запустим CLMonitor для генерации препроцессированных файлов и статического анализа:

%CLmon% analyze -l "c:\ptest.plog"

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

%PlogConverter% "c:\ptest.plog" --renderTypes=TeamCity -o "C:\temp"

Последним действием выведем форматированный отчёт в stdout, где его подхватит парсер TeamCity.

type "C:\temp\ptest.plog_TeamCity.txt"

Полный код скрипта:

choco install pvs-studio -y%CLmon% monitor --attachset platform=x64%MSB% %ProjPath% /t:clean%MSB% %ProjPath% /t:rebuild /p:configuration=release%MSB% %ProjPath% /t:g2%MSB% %ProjPath% /t:PublishPortable%PVS-Studio_cmd% credentials --username %PVS_Name% --serialNumber %PVS_Key%%CLmon% analyze -l "c:\ptest.plog"%PlogConverter% "c:\ptest.plog" --renderTypes=TeamCity -o "C:\temp"type "C:\temp\ptest.plog_TeamCity.txt"

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


Теперь кликнем на Inspections Total, чтоб перейти к просмотру отчёта анализатора:


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

Просмотр результатов работы анализатора

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

Предупреждение N1

V773 [CWE-401] The exception was thrown without releasing the 'result' pointer. A memory leak is possible. libopenrct2 ObjectFactory.cpp 443

Object* CreateObjectFromJson(....){  Object* result = nullptr;  ....  result = CreateObject(entry);  ....  if (readContext.WasError())  {    throw std::runtime_error("Object has errors");  }  ....}Object* CreateObject(const rct_object_entry& entry){  Object* result;  switch (entry.GetType())  {    case OBJECT_TYPE_RIDE:      result = new RideObject(entry);      break;    case OBJECT_TYPE_SMALL_SCENERY:      result = new SmallSceneryObject(entry);      break;    case OBJECT_TYPE_LARGE_SCENERY:      result = new LargeSceneryObject(entry);      break;    ....    default:      throw std::runtime_error("Invalid object type");  }  return result;}

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

Предупреждение N2

V501 There are identical sub-expressions '(1ULL << WIDX_MONTH_BOX)' to the left and to the right of the '|' operator. libopenrct2ui Cheats.cpp 487

static uint64_t window_cheats_page_enabled_widgets[] = {  MAIN_CHEAT_ENABLED_WIDGETS |  (1ULL << WIDX_NO_MONEY) |  (1ULL << WIDX_ADD_SET_MONEY_GROUP) |  (1ULL << WIDX_MONEY_SPINNER) |  (1ULL << WIDX_MONEY_SPINNER_INCREMENT) |  (1ULL << WIDX_MONEY_SPINNER_DECREMENT) |  (1ULL << WIDX_ADD_MONEY) |  (1ULL << WIDX_SET_MONEY) |  (1ULL << WIDX_CLEAR_LOAN) |  (1ULL << WIDX_DATE_SET) |  (1ULL << WIDX_MONTH_BOX) |  // <=  (1ULL << WIDX_MONTH_UP) |  (1ULL << WIDX_MONTH_DOWN) |  (1ULL << WIDX_YEAR_BOX) |  (1ULL << WIDX_YEAR_UP) |  (1ULL << WIDX_YEAR_DOWN) |  (1ULL << WIDX_DAY_BOX) |  (1ULL << WIDX_DAY_UP) |  (1ULL << WIDX_DAY_DOWN) |  (1ULL << WIDX_MONTH_BOX) |  // <=  (1ULL << WIDX_DATE_GROUP) |  (1ULL << WIDX_DATE_RESET),  ....};

Мало кто, кроме статического анализатора, смог бы пройти данный тест на внимательность. Данный пример копипасты хорош именно этим.

Предупреждения N3

V703 It is odd that the 'flags' field in derived class 'RCT12BannerElement' overwrites field in base class 'RCT12TileElementBase'. Check lines: RCT12.h:570, RCT12.h:259. libopenrct2 RCT12.h 570

struct RCT12SpriteBase{  ....  uint8_t flags;  ....};struct rct1_peep : RCT12SpriteBase{  ....  uint8_t flags;  ....};

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

Предупреждение N4

V793 It is odd that the result of the 'imageDirection / 8' statement is a part of the condition. Perhaps, this statement should have been compared with something else. libopenrct2 ObservationTower.cpp 38

void vehicle_visual_observation_tower(...., int32_t imageDirection, ....){  if ((imageDirection / 8) && (imageDirection / 8) != 3)  {    ....  }  ....}

Давайте разберёмся поподробнее. Выражение imageDirection / 8 будет false в том случае, если imageDirection находится в диапазоне от -7 до 7. Вторая часть: (imageDirection / 8) != 3 проверяет imageDirection на нахождение вне диапазона: от -31 до -24 и от 24 до 31 соответственно. Мне кажется довольно странным проверять числа на вхождение в определённый диапазон таким способом и, даже если в данном фрагменте кода нет ошибки, я бы рекомендовал переписать данные условия на более явные. Это существенно упростило бы жизнь людям, которые будут читать и поддерживать этот код.

Предупреждение N5

V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 1115, 1118. libopenrct2ui MouseInput.cpp 1118

void process_mouse_over(....){  ....  switch (window->widgets[widgetId].type)  {    case WWT_VIEWPORT:      ebx = 0;      edi = cursorId;                                 // <=      // Window event WE_UNKNOWN_0E was called here,      // but no windows actually implemented a handler and      // it's not known what it was for      cursorId = edi;                                 // <=      if ((ebx & 0xFF) != 0)      {        set_cursor(cursorId);        return;      }      break;      ....  }  ....}

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

Предупреждение N6

V1004 [CWE-476] The 'player' pointer was used unsafely after it was verified against nullptr. Check lines: 2085, 2094. libopenrct2 Network.cpp 2094

void Network::ProcessPlayerList(){  ....  auto* player = GetPlayerByID(pendingPlayer.Id);  if (player == nullptr)  {    // Add new player.    player = AddPlayer("", "");    if (player)                                          // <=    {      *player = pendingPlayer;       if (player->Flags & NETWORK_PLAYER_FLAG_ISSERVER)       {         _serverConnection->Player = player;       }    }    newPlayers.push_back(player->Id);                    // <=  }  ....}

Данный код поправить довольно просто, нужно или третий раз проверять player на нулевой указатель, либо внести его в тело условного оператора. Я бы предложил второй вариант:

void Network::ProcessPlayerList(){  ....  auto* player = GetPlayerByID(pendingPlayer.Id);  if (player == nullptr)  {    // Add new player.    player = AddPlayer("", "");    if (player)    {      *player = pendingPlayer;      if (player->Flags & NETWORK_PLAYER_FLAG_ISSERVER)      {        _serverConnection->Player = player;      }      newPlayers.push_back(player->Id);    }  }  ....}

Предупреждение N7

V547 [CWE-570] Expression 'name == nullptr' is always false. libopenrct2 ServerList.cpp 102

std::optional<ServerListEntry> ServerListEntry::FromJson(...){  auto name = json_object_get(server, "name");  .....  if (name == nullptr || version == nullptr)  {    ....  }  else  {    ....    entry.name = (name == nullptr ? "" : json_string_value(name));    ....  }  ....}

Можно одним махом избавиться от трудночитаемой строки кода и решить проблему с проверкой на nullptr. Предлагаю изменить код следующим образом:

std::optional<ServerListEntry> ServerListEntry::FromJson(...){  auto name = json_object_get(server, "name");  .....  if (name == nullptr || version == nullptr)  {    name = ""    ....  }  else  {    ....    entry.name = json_string_value(name);    ....  }  ....}

Предупреждение N8

V1048 [CWE-1164] The 'ColumnHeaderPressedCurrentState' variable was assigned the same value. libopenrct2ui CustomListView.cpp 510

void CustomListView::MouseUp(....){  ....  if (!ColumnHeaderPressedCurrentState)  {    ColumnHeaderPressed = std::nullopt;    ColumnHeaderPressedCurrentState = false;    Invalidate();  }}

Код выглядит довольно странно. Мне кажется, имела место быть опечатка либо в условии, либо при повторном присвоении переменной ColumnHeaderPressedCurrentState значения false.

Вывод

Как мы видим, интегрировать статический анализатор PVS-Studio в свой проект на TeamCity довольно просто. Для этого достаточно написать всего один маленький файл конфигурации. Проверка кода же позволит выявлять проблемы сразу после сборки, что поможет устранять их тогда, когда сложность и стоимость правок ещё малы.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Vladislav Stolyarov. PVS-Studio and Continuous Integration: TeamCity. Analysis of the Open RollerCoaster Tycoon 2 project.

Статический анализ кода показывает наибольшую эффективность во время внесения изменений в проект, поскольку ошибки всегда сложнее исправлять в будущем, чем не допустить их появления на ранних этапах. Мы продолжаем расширять варианты использования PVS-Studio в системах непрерывной разработки и покажем, как настроить анализ pull request-ов при помощи self-hosted агентов в Microsoft Azure DevOps, на примере игры Minetest.

Вкратце о том, с чем мы имеем дело

Minetest это открытый кроссплатформенный игровой движок, содержащий около 200 тысяч строк кода на C, C++ и Lua. Он позволяет создавать разные игровые режимы в воксельном пространстве. Поддерживает мультиплеер, и множество модов от сообщества. Репозиторий проекта размещен здесь: https://github.com/minetest/minetest.

Для настройки регулярного поиска ошибок использованы следующие инструменты:

PVS-Studio статический анализатор кода на языках C, C++, C# и Java для поиска ошибок и дефектов безопасности.

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

Для выполнения задач разработки в Azure возможно использование виртуальных машин-агентов Windows и Linux. Однако запуск агентов на локальном оборудовании имеет несколько весомых преимуществ:

• У локального хоста может быть больше ресурсов, чем у ВМ Azure;
• Агент не "исчезает" после выполнения своей задачи;
• Возможность прямой настройки окружения, и более гибкое управление процессами сборки;
• Локальное хранение промежуточных файлов положительно влияет на скорость сборки;
• Можно бесплатно выполнять более 30 задач в месяц.

Подготовка к использованию self-hosted агента

Процесс начала работы в Azure подробно описан в статье "PVS-Studio идёт в облака: Azure DevOps", поэтому перейду сразу к созданию self-hosted агента.

Для того, чтобы агенты имели право подключиться к пулам проекта, им нужен специальный Access Token. Получить его можно на странице "Personal Access Tokens", в меню "User settings".


После нажатия на "New token" необходимо указать имя и выбрать Read & manage Agent Pools (может понадобиться развернуть полный список через "Show all scopes").


Нужно скопировать токен, так как Azure больше его не покажет, и придется делать новый.


В качестве агента будет использован Docker контейнер на основе Windows Server Core. Хостом является мой рабочий компьютер на Windows 10 x64 с Hyper-V.

Сначала понадобится расширить объём дискового пространства, доступный Docker контейнерам.

В Windows для этого нужно модифицировать файл 'C:\ProgramData\Docker\config\daemon.json' следующим образом:

{  "registry-mirrors": [],  "insecure-registries": [],  "debug": true,  "experimental": false,  "data-root": "d:\\docker",  "storage-opts": [ "size=40G" ]}

Для создания Docker образа для агентов со сборочной системой и всем необходимым в директории 'D:\docker-agent' добавим Docker файл с таким содержимым:

# escape=`FROM mcr.microsoft.com/dotnet/framework/runtimeSHELL ["cmd", "/S", "/C"]ADD https://aka.ms/vs/16/release/vs_buildtools.exe C:\vs_buildtools.exeRUN C:\vs_buildtools.exe --quiet --wait --norestart --nocache `  --installPath C:\BuildTools `  --add Microsoft.VisualStudio.Workload.VCTools `  --includeRecommendedRUN powershell.exe -Command `  Set-ExecutionPolicy Bypass -Scope Process -Force; `  [System.Net.ServicePointManager]::SecurityProtocol =    [System.Net.ServicePointManager]::SecurityProtocol -bor 3072; `  iex ((New-Object System.Net.WebClient)    .DownloadString('https://chocolatey.org/install.ps1')); `  choco feature enable -n=useRememberedArgumentsForUpgrades;  RUN powershell.exe -Command `  choco install -y cmake --installargs '"ADD_CMAKE_TO_PATH=System"'; `  choco install -y git --params '"/GitOnlyOnPath /NoShellIntegration"'RUN powershell.exe -Command `  git clone https://github.com/microsoft/vcpkg.git; `  .\vcpkg\bootstrap-vcpkg -disableMetrics; `  $env:Path += '";C:\vcpkg"'; `  [Environment]::SetEnvironmentVariable(    '"Path"', $env:Path, [System.EnvironmentVariableTarget]::Machine); `  [Environment]::SetEnvironmentVariable(    '"VCPKG_DEFAULT_TRIPLET"', '"x64-windows"',  [System.EnvironmentVariableTarget]::Machine)RUN powershell.exe -Command `  choco install -y pvs-studio; `  $env:Path += '";C:\Program Files (x86)\PVS-Studio"'; `  [Environment]::SetEnvironmentVariable(    '"Path"', $env:Path, [System.EnvironmentVariableTarget]::Machine)RUN powershell.exe -Command `  $latest_agent =    Invoke-RestMethod -Uri "https://api.github.com/repos/Microsoft/                          azure-pipelines-agent/releases/latest"; `  $latest_agent_version =    $latest_agent.name.Substring(1, $latest_agent.tag_name.Length-1); `  $latest_agent_url =    '"https://vstsagentpackage.azureedge.net/agent/"' + $latest_agent_version +  '"/vsts-agent-win-x64-"' + $latest_agent_version + '".zip"'; `  Invoke-WebRequest -Uri $latest_agent_url -Method Get -OutFile ./agent.zip; `  Expand-Archive -Path ./agent.zip -DestinationPath ./agentUSER ContainerAdministratorRUN reg add hklm\system\currentcontrolset\services\cexecsvc        /v ProcessShutdownTimeoutSeconds /t REG_DWORD /d 60  RUN reg add hklm\system\currentcontrolset\control        /v WaitToKillServiceTimeout /t REG_SZ /d 60000 /fADD .\entrypoint.ps1 C:\entrypoint.ps1SHELL ["powershell", "-Command",       "$ErrorActionPreference = 'Stop';     $ProgressPreference = 'SilentlyContinue';"]ENTRYPOINT .\entrypoint.ps1

В результате получится сборочная система на основе MSBuild для C++, с Chocolatey для установки PVS-Studio, CMake и Git. Для удобного управления библиотеками, от которых зависит проект, собирается Vcpkg. А также скачивается свежая версия, собственно, Azure Pipelines Agent.

Для инициализации агента из ENTRYPOINT-а Docker файла вызывается PowerShell скрипт 'entrypoint.ps1', в который нужно добавить URL "организации" проекта, токен пула агентов, и параметры лицензии PVS-Studio:

$organization_url = "https://dev.azure.com/<аккаунт Microsoft Azure>"$agents_token = "<token агента>"$pvs_studio_user = "<имя пользователя PVS-Studio>"$pvs_studio_key = "<ключ PVS-Studio>"try{  C:\BuildTools\VC\Auxiliary\Build\vcvars64.bat  PVS-Studio_Cmd credentials -u $pvs_studio_user -n $pvs_studio_key    .\agent\config.cmd --unattended `    --url $organization_url `    --auth PAT `    --token $agents_token `    --replace;  .\agent\run.cmd} finally{  # Agent graceful shutdown  # https://github.com/moby/moby/issues/25982    .\agent\config.cmd remove --unattended `    --auth PAT `    --token $agents_token}

Команды для сборки образа и запуска агента:

docker build -t azure-agent -m 4GB .docker run -id --name my-agent -m 4GB --cpu-count 4 azure-agent

Агент запущен и готов выполнять задачи.

Запуск анализа на self-hosted агенте

Для анализа PR создается новый pipeline со следующим скриптом:

trigger: nonepr:  branches:    include:    - '*'pool: Defaultsteps:- script: git diff --name-only    origin/%SYSTEM_PULLREQUEST_TARGETBRANCH% >    diff-files.txt  displayName: 'Get committed files'- script: |    cd C:\vcpkg    git pull --rebase origin    CMD /C ".\bootstrap-vcpkg -disableMetrics"    vcpkg install ^    irrlicht zlib curl[winssl] openal-soft libvorbis ^    libogg sqlite3 freetype luajit    vcpkg upgrade --no-dry-run  displayName: 'Manage dependencies (Vcpkg)'- task: CMake@1  inputs:    cmakeArgs: -A x64      -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake      -DCMAKE_BUILD_TYPE=Release -DENABLE_GETTEXT=0 -DENABLE_CURSES=0 ..  displayName: 'Run CMake'- task: MSBuild@1  inputs:    solution: '**/*.sln'    msbuildArchitecture: 'x64'    platform: 'x64'    configuration: 'Release'    maximumCpuCount: true  displayName: 'Build'- script: |    IF EXIST .\PVSTestResults RMDIR /Q/S .\PVSTestResults    md .\PVSTestResults    PVS-Studio_Cmd ^    -t .\build\minetest.sln ^    -S minetest ^    -o .\PVSTestResults\minetest.plog ^    -c Release ^    -p x64 ^    -f diff-files.txt ^    -D C:\caches    PlogConverter ^    -t FullHtml ^    -o .\PVSTestResults\ ^    -a GA:1,2,3;64:1,2,3;OP:1,2,3 ^    .\PVSTestResults\minetest.plog    IF NOT EXIST "$(Build.ArtifactStagingDirectory)" ^    MKDIR "$(Build.ArtifactStagingDirectory)"    powershell -Command ^    "Compress-Archive -Force ^    '.\PVSTestResults\fullhtml' ^    '$(Build.ArtifactStagingDirectory)\fullhtml.zip'"  displayName: 'PVS-Studio analyze'  continueOnError: true- task: PublishBuildArtifacts@1  inputs:    PathtoPublish: '$(Build.ArtifactStagingDirectory)'    ArtifactName: 'psv-studio-analisys'    publishLocation: 'Container'  displayName: 'Publish analysis report'

Этот скрипт сработает при получении PR и будет выполнен на агентах, назначенных в пул по умолчанию. Необходимо только дать ему разрешение на работу с этим пулом.




В скрипте происходит сохранение списка измененных файлов, полученного при помощи git diff. Затем обновляются зависимости, генерируется solution проекта через CMake, и производится его сборка.

Если сборка прошла успешно, запускается анализ изменившихся файлов (флаг '-f diff-files.txt'), игнорируя созданные CMake вспомогательные проекты (выбираем только нужный проект флагом '-S minetest'). Для ускорения поиска связей между заголовочными и исходными C++ файлами создается специальный кэш, который будет храниться в отдельной директории (флаг '-D C:\caches').

Таким образом, мы теперь можем получать отчеты об анализе изменений в проекте.




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

Некоторые ошибки, найденные в Minetest

Затирание результата

V519 The 'color_name' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 627. string.cpp 627

static bool parseNamedColorString(const std::string &value,                                  video::SColor &color){  std::string color_name;  std::string alpha_string;  size_t alpha_pos = value.find('#');  if (alpha_pos != std::string::npos) {    color_name = value.substr(0, alpha_pos);    alpha_string = value.substr(alpha_pos + 1);  } else {    color_name = value;  }  color_name = lowercase(value); // <=  std::map<const std::string, unsigned>::const_iterator it;  it = named_colors.colors.find(color_name);  if (it == named_colors.colors.end())    return false;  ....}

Эта функция должна производить разбор названия цвета с параметром прозрачности (например, Green#77) и вернуть его код. В зависимости от результата проверки условия в переменную color_name передается результат разбиения строки либо копия аргумента функции. Однако затем в нижний регистр переводится не сама полученная строка, а исходный аргумент. В результате её не удастся найти в словаре цветов при наличии параметра прозрачности. Можем исправить эту строку так:

color_name = lowercase(color_name);

Лишние проверки условий

V547 Expression 'nearest_emergefull_d == 1' is always true. clientiface.cpp 363

void RemoteClient::GetNextBlocks (....){  ....  s32 nearest_emergefull_d = -1;  ....  s16 d;  for (d = d_start; d <= d_max; d++) {    ....      if (block == NULL || surely_not_found_on_disk || block_is_invalid) {        if (emerge->enqueueBlockEmerge(peer_id, p, generate)) {          if (nearest_emerged_d == -1)            nearest_emerged_d = d;        } else {          if (nearest_emergefull_d == -1) // <=            nearest_emergefull_d = d;          goto queue_full_break;        }  ....  }  ....queue_full_break:  if (nearest_emerged_d != -1) { // <=    new_nearest_unsent_d = nearest_emerged_d;  } else ....}

Переменная nearest_emergefull_d в процессе работы цикла не меняется, и её проверка не влияет на ход выполнения алгоритма. Либо это результат неаккуратного copy-paste, либо с ней забыли провести какие-то вычисления.

V560 A part of conditional expression is always false: y > max_spawn_y. mapgen_v7.cpp 262

int MapgenV7::getSpawnLevelAtPoint(v2s16 p){  ....  while (iters > 0 && y <= max_spawn_y) {               // <=    if (!getMountainTerrainAtPoint(p.X, y + 1, p.Y)) {      if (y <= water_level || y > max_spawn_y)          // <=        return MAX_MAP_GENERATION_LIMIT; // Unsuitable spawn point      // y + 1 due to biome 'dust'      return y + 1;    }  ....}

Значение переменной 'y' проверяется перед очередной итерацией цикла. Последующее, противоположное ей, сравнение всегда вернет false и, в целом, не влияет на результат проверки условия.

Потеря проверки указателя

V595 The 'm_client' pointer was utilized before it was verified against nullptr. Check lines: 183, 187. game.cpp 183

void gotText(const StringMap &fields){  ....  if (m_formname == "MT_DEATH_SCREEN") {    assert(m_client != 0);    m_client->sendRespawn();    return;  }  if (m_client && m_client->modsLoaded())    m_client->getScript()->on_formspec_input(m_formname, fields);}

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

Бит или не бит?

V616 The '(FT_RENDER_MODE_NORMAL)' named constant with the value of 0 is used in the bitwise operation. CGUITTFont.h 360

typedef enum  FT_Render_Mode_{  FT_RENDER_MODE_NORMAL = 0,  FT_RENDER_MODE_LIGHT,  FT_RENDER_MODE_MONO,  FT_RENDER_MODE_LCD,  FT_RENDER_MODE_LCD_V,  FT_RENDER_MODE_MAX} FT_Render_Mode;#define FT_LOAD_TARGET_( x )   ( (FT_Int32)( (x) & 15 ) << 16 )#define FT_LOAD_TARGET_NORMAL  FT_LOAD_TARGET_( FT_RENDER_MODE_NORMAL )void update_load_flags(){  // Set up our loading flags.  load_flags = FT_LOAD_DEFAULT | FT_LOAD_RENDER;  if (!useHinting()) load_flags |= FT_LOAD_NO_HINTING;  if (!useAutoHinting()) load_flags |= FT_LOAD_NO_AUTOHINT;  if (useMonochrome()) load_flags |=     FT_LOAD_MONOCHROME | FT_LOAD_TARGET_MONO | FT_RENDER_MODE_MONO;  else load_flags |= FT_LOAD_TARGET_NORMAL; // <=}

Макрос FT_LOAD_TARGET_NORMAL разворачивается в ноль, и побитовое "или" не будет устанавливать никакие флаги в load_flags, ветка else может быть убрана.

Округление целочисленного деления

V636 The 'rect.getHeight() / 16' 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;. hud.cpp 771

void drawItemStack(....){  float barheight = rect.getHeight() / 16;  float barpad_x = rect.getWidth() / 16;  float barpad_y = rect.getHeight() / 16;  core::rect<s32> progressrect(    rect.UpperLeftCorner.X + barpad_x,    rect.LowerRightCorner.Y - barpad_y - barheight,    rect.LowerRightCorner.X - barpad_x,    rect.LowerRightCorner.Y - barpad_y);}

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

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

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. treegen.cpp 413

treegen::error make_ltree(...., TreeDef tree_definition){  ....  std::stack <core::matrix4> stack_orientation;  ....    if ((stack_orientation.empty() &&      tree_definition.trunk_type == "double") ||      (!stack_orientation.empty() &&      tree_definition.trunk_type == "double" &&      !tree_definition.thin_branches)) {      ....    } else if ((stack_orientation.empty() &&      tree_definition.trunk_type == "crossed") ||      (!stack_orientation.empty() &&      tree_definition.trunk_type == "crossed" &&      !tree_definition.thin_branches)) {      ....    } if (!stack_orientation.empty()) {                  // <=  ....  }  ....}

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

Неправильная проверка выделения памяти

V668 There is no sense in testing the 'clouds' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. game.cpp 1367

bool Game::createClient(....){  if (m_cache_enable_clouds) {    clouds = new Clouds(smgr, -1, time(0));    if (!clouds) {      *error_message = "Memory allocation error (clouds)";      errorstream << *error_message << std::endl;      return false;    }  }}

В случае, если new не сможет создать объект, будет брошено исключение std::bad_alloc, и оно должно быть обработано try-catch блоком. А проверка в таком виде бесполезна.

Чтение за границей массива

V781 The value of the 'i' index is checked after it was used. Perhaps there is a mistake in program logic. irrString.h 572

bool equalsn(const string<T,TAlloc>& other, u32 n) const{  u32 i;  for(i=0; array[i] && other[i] && i < n; ++i) // <=    if (array[i] != other[i])      return false;  // if one (or both) of the strings was smaller then they  // are only equal if they have the same length  return (i == n) || (used == other.used);}

Происходит обращение к элементам массивов перед проверкой индекса, что может привести к ошибке. Возможно, стоит переписать цикл так:

for (i=0; i < n; ++i) // <=  if (!array[i] || !other[i] || array[i] != other[i])    return false;

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

Данная статья посвящена анализу pull request-ов в Azure DevOps и не ставит целью провести подробный обзор ошибок проекта Minetest. Здесь выписаны только некоторые фрагменты кода, которые мне показались интересными. Предлагаем авторам проекта не руководствоваться этой статьёй для исправления ошибок и выполнить более тщательный анализ предупреждений, которые выдаст PVS-Studio.

Заключение

Благодаря гибкой конфигурации в режиме командной строки, анализ PVS-Studio может быть встроен в самые разнообразные сценарии CI/CD. А правильное использование доступных ресурсов окупается увеличением производительности.

Нужно отметить, что режим проверки pull request-ов доступен только в Enterprise редакции анализатора. Чтобы получить демонстрационную Enterprise лицензию, укажите это в комментарии при запросе лицензии на странице скачивания. Более подробно о разнице между лицензиями можно узнать на странице заказа PVS-Studio.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexey Govorov. PVS-Studio: analyzing pull requests in Azure DevOps using self-hosted agents.

Моё знакомство с Open XML SDK началось с того, что мне понадобилась библиотека для создания документов Word с некоторой отчётностью. После работы с Word API более 7 лет, захотелось попробовать что-нибудь новое и более удобное. Так я узнал, что у Microsoft есть альтернативное решение. По традиции, используемые в компании программы и библиотеки мы предварительно проверяем с помощью анализатора PVS-Studio.

Введение

Office Open XML, также известный как OpenXML или OOXML, представляет собой формат на основе XML для офисных документов, включая текстовые документы, электронные таблицы, презентации, а также диаграммы, фигуры и другой графический материал. Спецификация была разработана Microsoft и принята ECMA International в 2006 году. В июне 2014 года Microsoft выпустила Open XML SDK в open source. Сейчас исходники доступны на GitHub под лицензий MIT.

Для поиска ошибок в исходном коде библиотеки использовался PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS.

Проект достаточно маленький и предупреждений нашлось немного. Но выбор титульной картинки основывался как раз на результатах. Уж очень много бесполезных условных операторов в коде нашлось. Мне кажется, если отрефакторить все такие места в коде, то объём заметно сократится. Вследствие чего ещё и читаемость кода повысится.

Почему Word API, а не Open XML SDK

Как Вы могли догадаться из заголовка, я продолжил использовать Word API. У этого способа достаточно много минусов:

• Старый неудобный API;
• Должен быть установлен Microsoft Office;
• Необходимость распространять дистрибутив с библиотеками Office;
• Зависимость работы Word API от настроек локали системы;
• Низкая скорость работы.

С локалью вообще забавный случай произошёл. В Windows есть десяток региональных настроек. На одном из серверных компьютеров оказалась каша из локалей USA и UK. Из-за этого создавались документы Word, где вместо символа доллара был рубль, а фунты вообще не выводились. Доработка настроек операционной системы исправила проблему.

Перечисляя всё это, я снова задумался, почему я до сих пор этим пользуюсь

Но нет, Word API мне пока нравится больше, и вот почему.

OOXML выглядит таким образом:

<?xml version="1.0" encoding="utf-8" standalone="yes"?><w:document ....>  <w:body>    <w:p w:rsidR="00E22EB6"         w:rsidRDefault="00E22EB6">      <w:r>        <w:t>This is a paragraph.</w:t>      </w:r>    </w:p>    <w:p w:rsidR="00E22EB6"         w:rsidRDefault="00E22EB6">      <w:r>        <w:t>This is another paragraph.</w:t>      </w:r>    </w:p>  </w:body></w:document>

Где <w:r> (Word Run) не предложение, и даже не слово, а любой фрагмент текста, имеющий атрибуты, отличные от соседних фрагментов текста.

Программируется это примерно таким кодом:

Paragraph para = body.AppendChild(new Paragraph());Run run = para.AppendChild(new Run());run.AppendChild(new Text(txt));

У документа специфичная внутренняя структура, и в коде нужно создавать те же самые элементы. У Open XML SDK, я считаю, недостаточно абстрактный уровень доступа к данным. Создание документа с помощью Word API будет более понятым и коротким. Особенно, когда дело дойдёт до таблиц и других сложных структур данных.

В свою очередь, Open XML SDK решает большой ряд задач. С ним можно создавать документы не только для Word, но и для Excel и PowerPoint. Наверное, для некоторых задач эта библиотека больше подходит, но я решил пока остаться на Word API. Полностью от него отказаться в любом случае не получится, т.к. для внутренних нужд мы разрабатываем плагин для Word, а там возможно использование только Word API.

Два значения для string

V3008 The '_rawOuterXml' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 164, 161. OpenXmlElement.cs 164

internal string RawOuterXml{    get => _rawOuterXml;    set    {        if (string.IsNullOrEmpty(value))        {            _rawOuterXml = string.Empty;        }        _rawOuterXml = value;    }}

Тип string может иметь 2 типа значений: null и текстовое значение. Использовать текстовое значение определённо безопаснее, но оба подхода имеют права на существование. Вот в этом проекте значение null использовать неприемлемо и его перезаписывают на string.Empty по крайней мере, так задумывалось. Но из-за ошибки в RawOuterXml всё же можно записать null, а потом обратиться к этому полю, получив NullReferenceException.

V3022 Expression 'namespaceUri != null' is always true. OpenXmlElement.cs 497

public OpenXmlAttribute GetAttribute(string localName, string namespaceUri){    ....    if (namespaceUri == null)    {        // treat null string as empty.        namespaceUri = string.Empty;    }    ....    if (HasAttributes)    {        if (namespaceUri != null)  // <=        {            ....        }        ....    }    ....}

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

Про компактность кода

V3009 It's odd that this method always returns one and the same value of '".xml"'. CustomXmlPartTypeInfo.cs 31

internal static string GetTargetExtension(CustomXmlPartType partType){    switch (partType)    {        case CustomXmlPartType.AdditionalCharacteristics:            return ".xml";        case CustomXmlPartType.Bibliography:            return ".xml";        case CustomXmlPartType.CustomXml:            return ".xml";        case CustomXmlPartType.InkContent:            return ".xml";        default:            return ".xml";    }}

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

Это не единственное такое место. Вот ещё парочка таких предупреждений:

• V3009 It's odd that this method always returns one and the same value of '".xml"'. CustomPropertyPartTypeInfo.cs 25
• V3009 It's odd that this method always returns one and the same value of '".bin"'. EmbeddedControlPersistenceBinaryDataPartTypeInfo.cs 22

Интересно было бы услышать, зачем так писать.

V3139 Two or more case-branches perform the same actions. OpenXmlPartReader.cs 560

private void InnerSkip(){    Debug.Assert(_xmlReader != null);    switch (_elementState)    {        case ElementState.Null:            ThrowIfNull();            break;        case ElementState.EOF:            return;        case ElementState.Start:            _xmlReader.Skip();            _elementStack.Pop();            GetElementInformation();            return;        case ElementState.End:        case ElementState.MiscNode:            // cursor is end element, pop stack            _xmlReader.Skip();            _elementStack.Pop();            GetElementInformation();            return;        ....    }    ....}

К этому коду возникает меньше вопросов. Скорее всего, идентичные кейсы можно объединить и код станет короче и очевиднее.

Ещё несколько таких мест:

• V3139 Two or more case-branches perform the same actions. OpenXmlMiscNode.cs 312
• V3139 Two or more case-branches perform the same actions. CustomPropertyPartTypeInfo.cs 30
• V3139 Two or more case-branches perform the same actions. CustomXmlPartTypeInfo.cs 15
• V3139 Two or more case-branches perform the same actions. OpenXmlElement.cs 1803

Те самые Always true/false

Настало время привести примеры кода, которые определили мой выбор титульной картинки.

Предупреждение 1

V3022 Expression 'Complete()' is always false. ParticleCollection.cs 243

private bool IsComplete => Current is null ||                           Current == _collection._element.FirstChild;public bool MoveNext(){    ....    if (IsComplete)    {        return Complete();    }    if (....)    {        return Complete();    }    return IsComplete ? Complete() : true;}

Свойство IsComlete используется 2 раза, и по коду легко понять, что его значение не изменится. Таким образом, в конце функции можно просто возвращать второе значение тернарного оператора true.

Предупреждение 2

V3022 Expression '_elementStack.Count > 0' is always true. OpenXmlDomReader.cs 501

private readonly Stack<OpenXmlElement> _elementStack;private bool MoveToNextSibling(){    ....    if (_elementStack.Count == 0)    {        _elementState = ElementState.EOF;        return false;    }    ....    if (_elementStack.Count > 0) // <=    {        _elementState = ElementState.End;    }    else    {        // no more element, EOF        _elementState = ElementState.EOF;    }    ....}

Очевидно, что если в стеке_elementStack не 0 элементов, то их больше. Код можно сократить как минимум на 8 строк.

Предупреждение 3

V3022 Expression 'rootElement == null' is always false. OpenXmlPartReader.cs 746

private static OpenXmlElement CreateElement(string namespaceUri, string name){    if (string.IsNullOrEmpty(name))    {        throw new ArgumentException(....);    }    if (NamespaceIdMap.TryGetNamespaceId(namespaceUri, out byte nsId)        && ElementLookup.Parts.Create(nsId, name) is OpenXmlElement element)    {        return element;    }    return new OpenXmlUnknownElement();}private bool ReadRoot(){  ....  var rootElement = CreateElement(....);  if (rootElement == null) // <=  {      throw new InvalidDataException(....);  }  ....}

Функция CreateElement не может вернуть null. Если в компании было принято правило писать методы для создания xml-нод, которые либо возвращают валидный объект, либо кидают исключение, то пользователям таких функций можно не злоупотреблять дополнительными проверками.

Предупреждение 5

V3022 Expression 'nameProvider' is always not null. The operator '?.' is excessive. OpenXmlSimpleTypeExtensions.cs 50

public static XmlQualifiedName GetSimpleTypeQualifiedName(....){    foreach (var validator in validators)    {        if (validator is INameProvider nameProvider &&            nameProvider?.QName is XmlQualifiedName qname) // <=        {            return qname;        }    }    return type.GetSimpleTypeQualifiedName();}

Оператор is имеет такой паттерн:

expr is type varname

Если выражение expr принимает значение true, то объект varname будет валидным и его не надо снова сравнивать с null, как это сделано в этом фрагменте кода.

Предупреждение 6

V3022 Expression 'extension == ".xlsx" || extension == ".xlsm"' is always false. PresentationDocument.cs 246

public static PresentationDocument CreateFromTemplate(string path){    ....    string extension = Path.GetExtension(path);    if (extension != ".pptx" && extension != ".pptm" &&        extension != ".potx" && extension != ".potm")    {        throw new ArgumentException("...." + path, nameof(path));    }    using (PresentationDocument template = PresentationDocument.Open(....)    {        PresentationDocument document = (PresentationDocument)template.Clone();        if (extension == ".xlsx" || extension == ".xlsm")        {            return document;        }        ....    }    ....}

Интересный код получился. Сначала автор отсеял все документы с расширениями не .pptx, .pptm, .potx и .potm, а потом решил для перестраховки проверить, что среди них нет .xlsx и .xlsm. Функция PresentationDocument определённо жертва рефакторинга.

Предупреждение 7

V3022 Expression 'OpenSettings.MarkupCompatibilityProcessSettings == null' is always false. OpenXmlPackage.cs 661

public MarkupCompatibilityProcessSettings MarkupCompatibilityProcessSettings{    get    {        if (_mcSettings is null)        {            _mcSettings = new MarkupCompatibilityProcessSettings(....);        }        return _mcSettings;    }    set    {        _mcSettings = value;    }}public MarkupCompatibilityProcessSettings MarkupCompatibilityProcessSettings{    get    {        if (OpenSettings.MarkupCompatibilityProcessSettings == null) // <=        {            return new MarkupCompatibilityProcessSettings(....);        }        else        {            return OpenSettings.MarkupCompatibilityProcessSettings;        }    }}

Свойство MarkupCompatibilityProcessSettings никогда не возвращает null. Если в геттере выясняется, что поле класса имеет значение null, то объект перезаписывается на новый. Ещё обратите внимание, что это не рекурсивный вызов одного свойства, а это одноимённые свойства из разных классов. Возможно, некоторая путаница и привела к написанию лишних проверок.

Остальные предупреждения

Предупреждение 1

V3080 Possible null dereference. Consider inspecting 'previousSibling'. OpenXmlCompositeElement.cs 380

public OpenXmlElement PreviousSibling(){    if (!(Parent is OpenXmlCompositeElement parent))    {        return null;    }    ....}public override T InsertBefore<T>(T newChild, OpenXmlElement referenceChild){    ....    OpenXmlElement previousSibling = nextNode.PreviousSibling();    prevNode.Next = nextNode;    previousSibling.Next = prevNode;    // <=    ....}

А вот пример, где дополнительной проверки как раз не хватает. Метод PreviousSibling как раз может вернуть значение null, а результат этой функции сразу используется без проверки.

Ещё 2 опасных места:

• V3080 Possible null dereference. Consider inspecting 'prevNode'. OpenXmlCompositeElement.cs 489
• V3080 Possible null dereference. Consider inspecting 'prevNode'. OpenXmlCompositeElement.cs 497

Предупреждение 2

V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. UniqueAttributeValueConstraint.cs 60

public override ValidationErrorInfo ValidateCore(ValidationContext context){    ....    foreach (var e in root.Descendants(....))    {        if (e != element & e.GetType() == elementType) // <=        {            var eValue = e.ParsedState.Attributes[_attribute];            if (eValue.HasValue && _comparer.Equals(....))            {                return true;            }        }    }    ....}

Некоторые любят применять оператор '&' к логическим выражениям там, где не надо. В случае этого оператора сначала вычисляется второй операнд, независимо от результата первого. Здесь это не сильно серьёзная ошибка, но такой неаккуратный код после рефакторинга может приводить и к потенциальным исключениям NullReferenceException.

Предупреждение 3

V3097 Possible exception: type marked by [Serializable] contains non-serializable members not marked by [NonSerialized]. OpenXmlPackageValidationEventArgs.cs 15

[Serializable][Obsolete(ObsoleteAttributeMessages.ObsoleteV1ValidationFunctionality, false)][EditorBrowsable(EditorBrowsableState.Never)]public sealed class OpenXmlPackageValidationEventArgs : EventArgs{    private string _message;    [NonSerialized]    private readonly object _sender;    [NonSerialized]    private OpenXmlPart _subPart;    [NonSerialized]    private OpenXmlPart _part;    ....    internal DataPartReferenceRelationship        DataPartReferenceRelationship { get; set; } // <=}

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

Заключение

Мы в компании любим проекты и технологии Microsoft. В разделе, где мы перечисляем Open Source проекты, проверенные с помощью PVS-Studio, мы даже выделили для Microsoft отдельный раздел. Там уже накопился 21 проект, про которые написано 26 статей. Это 27-я.

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


Ещё из новостей, кому интересен анализ кода на C++, C# и Java: мы недавно добавили поддержку стандарта OWASP и активно увеличиваем его покрытие.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing the Code Quality of Microsoft's Open XML SDK.

XMage клиент-серверное приложение для игры в Magic: The Gathering (MTG). XMage начал развиваться еще в начале 2010 года. За это время было выпущено 182 релиза, набралась целая армия контрибьюторов, и проект до сих пор активно развивается. Отличный повод поучаствовать и нам в его развитии! Поэтому сегодня единорог из PVS-Studio проверит кодовую базу XMage, и кто знает, может и схлестнется с кем-нибудь в бою.

Вкратце о проекте

XMage активно развивающееся приложение на протяжении уже 10 лет. Его цель сделать бесплатную, с открытым исходным кодом, онлайновую версию оригинальной карточной игры Magic: the Gathering.

Возможности приложения:

• доступ к ~19 000 уникальных карт, выпущенных за 20-летнюю историю MTG;
• автоматический контроль и применение всех существующих правил игры;
• многопользовательский режим с поиском игроков на общем сервере;
• одиночный режим с игрой против компьютера (AI);
• десятки форматов и режимов игры (Standard, Modern, Vintage, Commander и многое другое);
• возможность проведения как одиночных матчей, так и турниров.

Небольшое отступление

Случайно наткнулся на работу студентов из Делфтского технического университета 2018 года (магистерский курс Software Architecture). Она заключалась в том, что ребята принимали активное участие в open-source проектах, которые должны были быть достаточно сложными и активно развиваться. В течение восьминедельного периода студенты изучали курс и open-source проекты, чтобы понять и описать архитектуру выбранного программного обеспечения.

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

Моё внимание привлекло то, что на момент 2018 года сканирование SonarQube'ом показало 700 дефектов (bugs, vulnerabilities) на 1 000 000 строк кода.

Покопавшись в истории ребят-контрибьюторов, я выявил, что из полученного отчета с предупреждениями они сделали pull-request на исправление примерно 30 дефектов из категории "Blocker" или "Critical". Что с остальными предупреждениями неизвестно, но надеюсь на то, что их не пропустили мимо глаз.

С тех пор прошло уже 2 года и кодовая база подросла примерно на 250 000 строк кода неплохой повод посмотреть, как там дела.

Об анализе

Для анализа я взял релиз XMage 1.4.44V0.

С проектом очень повезло. Собрать XMage при помощи Maven получилось очень просто (как и было написано в документации):

mvn clean install -DskipTests

Больше от меня ничего не потребовалось. Круто же?

С интеграцией плагина PVS-Studio в Maven тоже не возникло проблем -все как в документации.

После анализа было получено 911 предупреждений, из которых 674 приходится на предупреждения 1 и 2 уровня достоверности. В рамках данной статьи я не рассматривал предупреждения 3 уровня достоверности, так как там обычно велик процент ложных срабатываний. Хочу обратить ваше внимание на то, что при использовании статического анализатора в реальном бою игнорировать такие предупреждения нельзя, так как они могут также указывать на значимые дефекты в коде.

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

• V6022, которое ищет неиспользуемые параметры в методах/конструкторах. На их долю пришлось аж 336 срабатываний.
• V6014, которое предупреждает о том, что все ветки выхода из метода возвращают одно и то же значение. 73 срабатывания.
• V6021, которое сигнализирует о том, что в переменную записывается некий результат и про эту переменную забывают. 36 срабатываний.
• V6048, которое предупреждает о том, что выражение можно упростить. 17 срабатываний.

Плюс к этому несколько диагностических правил выдали примерно 20 явных однотипных ложных срабатываний. Записали в todo!

В итоге, если все вычесть, то к рассмотрению ко мне попало примерно 190 срабатываний.

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

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

Давайте глянем, что вышло.

Предупреждение N1

V6003 The use of 'if (card != null) {...} else if (card != null) {...}' pattern was detected. There is a probability of logical error presence. TorrentialGearhulk.java(90), TorrentialGearhulk.java(102)

@Overridepublic boolean apply(Game game, Ability source) {  ....  Card card = game.getCard(....);  if (card != null) {      ....  } else if (card != null) {      ....  }  ....}

Тут все просто: тело второго условного оператора if (card != null) в конструкции if-else-if никогда не выполнится, так как выполнение программы либо не дойдет до этого места, либо card != null будет всегда false.

Предупреждение N2

V6004 The 'then' statement is equivalent to the 'else' statement. AsThoughEffectImpl.java(35), AsThoughEffectImpl.java(37)

@Overridepublic boolean applies(....) {  // affectedControllerId = player to check  if (getAsThoughEffectType().equals(AsThoughEffectType.LOOK_AT_FACE_DOWN)) {    return applies(objectId, source, playerId, game);  } else {    return applies(objectId, source, playerId, game);  }}

Банальная ошибка, которая частенько встречалась на моей практике проверок open-source проектов. Copy-paste? Или я что-то не понимаю? Предположу, что в ветке else всё-таки нужно возвращать false.

P.S. Если что, тут нет рекурсивного вызова applies(....), так как это разные методы.

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

• V6004 The 'then' statement is equivalent to the 'else' statement. GuiDisplayUtil.java(194), GuiDisplayUtil.java(198)

Предупреждение N3

V6007 Expression 'filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")' is always false. SetPowerToughnessAllEffect.java(107)

@Overridepublic String getText(Mode mode) {  StringBuilder sb = new StringBuilder();  ....  if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {    sb.append(" has base power and toughness ");  } else {    sb.append(" have base power and toughness ");  }  ....  return sb.toString();}

Срабатывания диагностического правила V6007 достаточно популярны для каждого проверяемого проекта. XMage не исключение (79 штук). Срабатывания правила, в принципе, все по делу, но много случаев приходится то на debug, то на перестраховывание, то еще на что. В общем, такие срабатывания лучше смотреть автору кода, нежели мне.

Данное срабатывание, однако, точно является ошибкой. В зависимости от начала строки filter.getMessage() к sb добавляется текст " has ...", либо " have ...". Но ошибочка в том, что разработчики проверяют, чтобы строка начиналась с заглавной буквы, преобразовав перед этим эту самую строку в нижний регистр. Упс. В результате добавленной строкой всегда будет " have ...". Результат дефекта не критический, но тоже неприятный: где-то будет фигурировать неграмотно составленный текст.

Срабатывания, которые мне показались наиболее интересными:

• V6007 Expression 't.startsWith("-")' is always false. BoostSourceEffect.java(103)
• V6007 Expression 'setNames.isEmpty()' is always false. DownloadPicturesService.java(300)
• V6007 Expression 'existingBucketName == null' is always false. S3Uploader.java(23)
• V6007 Expression '!lastRule.endsWith(".")' is always true. Effects.java(76)
• V6007 Expression 'subtypesToIgnore::contains' is always false. VerifyCardDataTest.java(893)
• V6007 Expression 'notStartedTables == 1' is always false. MageServerImpl.java(1330)

Предупреждение N4

V6008 Null dereference of 'savedSpecialRares'. DragonsMaze.java(230)

public final class DragonsMaze extends ExpansionSet {  ....  private List<CardInfo> savedSpecialRares = new ArrayList<>();  ....  @Override  public List<CardInfo> getSpecialRare() {    if (savedSpecialRares == null) {                    // <=      CardCriteria criteria = new CardCriteria();      criteria.setCodes("GTC").name("Breeding Pool");      savedSpecialRares.addAll(....);                   // <=      criteria = new CardCriteria();      criteria.setCodes("GTC").name("Godless Shrine");      savedSpecialRares.addAll(....);      ....    }    return new ArrayList<>(savedSpecialRares);  }}

Анализатор ругается на разыменование нулевой ссылки savedSpecialRares, когда выполнение дойдет до первого заполнения коллекции.

Первое, что приходит на ум: просто перепутали savedSpecialRares == null с savedSpecialRares != null. Но в таком случае NPE может произойти в конструкторе ArrayList при возвращении коллекции из метода, так как savedSpecialRares == null по-прежнему не исключено. Исправлять код первым пришедшим в голову решением не очень хороший вариант. Немного разобравшись с кодом, выяснил, что savedSpecialRares сразу определяется пустой коллекцией при объявлении и при этом больше нигде не переприсваивается. Это говорит нам о том, что savedSpecialRares никогдане будет null, и разыменование нулевой ссылки, о котором предупреждает анализатор, так и не произойдет, так как до заполнения коллекции дело так и не дойдет. Как итог, метод всегда будет возвращать пустую коллекцию.

P.S. Для исправления нужно заменить savedSpecialRares == null на savedSpecialRares.isEmpty().

P.P.S. Увы, но пока что, играя в XMage, не получится получить специальные редкие карточки для коллекции Dragon's Maze.

Еще один случай разыменования нулевой ссылки:

• V6008 Null dereference of 'match'. TableController.java(973)

Предупреждение N5

V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value 'table.getCreateTime()'. TableManager.java(418), TableManager.java(418)

private void checkTableHealthState() {  ....  logger.debug(.... + formatter.format(table.getStartTime() == null                                        ? table.getCreateTime()                                        : table.getCreateTime()) + ....);  ....}

Здесь тернарный оператор ?: возвращает одно и тоже значение вне зависимости от условия table.getStartTime() == null. Полагаю, что автодополнение кода сыграло злую шутку с разработчиком. Вариант исправления:

private void checkTableHealthState() {  ....  logger.debug(.... + formatter.format(table.getStartTime() == null                                        ? table.getCreateTime()                                        : table.getStartTime()) + ....);  ....}

Предупреждение N6

V6026 This value is already assigned to the 'this.loseOther' variable. BecomesCreatureTypeTargetEffect.java(54)

publicBecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {  super(effect);  this.subtypes.addAll(effect.subtypes);  this.loseOther = effect.loseOther;  this.loseOther = effect.loseOther;}

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

Предупреждение N7

V6036 The value from the uninitialized 'selectUser' optional is used. Session.java(227)

public String connectUserHandling(String userName, String password){  ....  if (!selectUser.isPresent()) {  // user already exists      selectUser = UserManager.instance.getUserByName(userName);      if (selectUser.isPresent()) {          User user = selectUser.get();            ....      }  }  User user = selectUser.get(); // <=  ....}

По предупреждению анализатора можно сделать вывод, что selectUser.get() может бросить исключение NoSuchElementException.

Давайте рассмотрим более детально что тут происходит.

Если верить комментарию, что user уже существует, то исключения не возникнет:

....if (!selectUser.isPresent()) {  // user already exists  ....}User user = selectUser.get()....

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

А что, если комментарий ничего из себя не представляет?

....if (!selectUser.isPresent()) {  // user already exists    selectUser = UserManager.instance.getUserByName(userName);    if (selectUser.isPresent()) {      ....    }}User user = selectUser.get(); // <=....

Тогда выполнение заходит в тело условного оператора и заново получает пользователя через getUserByName(). Пользователя снова проверяют на валидность, и это наталкивает на мысль, что selectUser может быть неинициализированным. Ветки else на этот случай нет, что далее и приведет к NoSuchElementException на рассматриваемой строке кода.

Предупреждение N8

V6042 The expression is checked for compatibility with type 'A' but is cast to type 'B'. CheckBoxList.java(586)

/** * sets the model - must be an instance of CheckBoxListModel *  * @param model the model to use * @throws IllegalArgumentException if the model is not an instance of *           CheckBoxListModel * @see CheckBoxListModel */@Overridepublic void setModel(ListModel model) {  if (!(model instanceof CheckBoxListModel)) {    if (model instanceof javax.swing.DefaultListModel) {       super.setModel((CheckBoxListModel)model);         // <=    }    else {      throw new IllegalArgumentException(          "Model must be an instance of CheckBoxListModel!");    }  }  else {    super.setModel(model);  }}

Автор кода что-то здесь запутался: сначала убеждается в том, что model не является CheckBoxListModel, а потом в итоге явно приводит объект к этому типу. Из-за этого метод setModel сразу же выбросит ClassCastException, добравшись до этого места.

Файл CheckBoxList.java был добавлен 2 года назад, и эта ошибка живет в коде до сих пор. Тестов на некорректные параметры, видимо, нет, реального использования этого метода c объектами неподходящих типов тоже нет, поэтому и живет.

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

Учитывая документацию, код, скорее всего, должен выглядеть следующим образом:

public void setModel(ListModel model) {  if (!(model instanceof CheckBoxListModel)) {     throw new IllegalArgumentException(        "Model must be an instance of CheckBoxListModel!");    }  else {    super.setModel(model);  }}

Предупреждение N9

V6060 The 'player' reference was utilized before it was verified against null. VigeanIntuition.java(79), VigeanIntuition.java(78)

@Overridepublic boolean apply(Game game, Ability source) {    MageObject sourceObject = game.getObject(source.getSourceId());    Player player = game.getPlayer(source.getControllerId());    Library library = player.getLibrary();                           // <=    if (player != null && sourceObject != null && library != null) { // <=        ....    }}

V6060 предупреждает разработчика о том, что происходит обращение к объекту до того, как производится его проверка на null. Срабатывания этого правила частенько встречаются в статьях о проверках open-source проектов: обычно причиной этого становится неудачный рефакторинг или смена контрактов у методов. Если обратить внимание на объявление метода getPlayer(), то всё сразу станет на свои места:

// Result must be checked for null.// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)Player getPlayer(UUID playerId);

Предупреждение N10

V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SubTypeChangingEffectsTest.java(162), SubTypeChangingEffectsTest.java(158), SubTypeChangingEffectsTest.java(156), SubTypeChangingEffectsTest.java(160)

@Testpublic void testArcaneAdaptationGiveType() {    addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); // Enchantment {2}{U}    addCard(Zone.BATTLEFIELD, playerA, "Island", 3);    addCard(Zone.HAND, playerA, "Silvercoat Lion");    addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=    addCard(Zone.HAND, playerB, "Silvercoat Lion");    addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=    ....    for (Card card : playerB.getGraveyard().getCards(currentGame)) {        if (card.isCreature()) {            Assert.assertEquals(card.getName() + " should not have ORC type",                    false, card.getSubtype(currentGame).contains(SubType.ORC));            Assert.assertEquals(card.getName() + " should have CAT type",                    true, card.getSubtype(currentGame).contains(SubType.CAT));        }    }}

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

Метод testArcaneAdaptationGiveType() тестирует карточку "Arcane Adaptation". Каждому игроку раздаются карты в определенную игровую зону. И благодаря copy-paste в игровую зону "Кладбище" игроку playerА попали 2 одинаковые карточки "Silvercoat Lion", а игроку playerB так ничего и не досталось. Далее какая-то магия и само тестирование.

Когда доходит тестирование до "кладбища" игрока playerB в текущем розыгрыше, то в цикл выполнение теста так и не заходит, ведь в "кладбище" ничего и не было. Это я выяснил старым добрым System.out.println() при запуске теста.

Исправленный вариант copy-paste:

....addCard(Zone.HAND, playerA, "Silvercoat Lion");addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=addCard(Zone.HAND, playerB, "Silvercoat Lion");addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");addCard(Zone.GRAVEYARD, playerB, "Silvercoat Lion");   // <=....

После того как я скорректировал код, при запуске теста проверка существ в кладбище игрока playerB начала работать. Аве, System.out.println()!

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

Такой же copy-paste в других местах:

• V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. PaintersServantTest.java(33), PaintersServantTest.java(29), PaintersServantTest.java(27), PaintersServantTest.java(31)
• V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SubTypeChangingEffectsTest.java(32), SubTypeChangingEffectsTest.java(28), SubTypeChangingEffectsTest.java(26), SubTypeChangingEffectsTest.java(30)

Предупреждение N11

V6086 Suspicious code formatting. 'else' keyword is probably missing. DeckImporter.java(23)

public static DeckImporter getDeckImporter(String file) {  if (file == null) {    return null;  } if (file.toLowerCase(Locale.ENGLISH).endsWith("dec")) {   // <=    return new DecDeckImporter();  } else if (file.toLowerCase(Locale.ENGLISH).endsWith("mwdeck")) {    return new MWSDeckImporter();  } else if (file.toLowerCase(Locale.ENGLISH).endsWith("txt")) {    return new TxtDeckImporter(haveSideboardSection(file));  }  ....  else {    return null;  }}

Диагностическое правило V6086 диагностирует некорректное форматирование if-else-if, подразумевающее пропуск else.

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

Давайте рассмотрим случай, когда пропуск else может привести к неожиданному поведению:

public SomeType smtMethod(SomeType obj) {  ....  if (obj == null) {    obj = getNewObject();  } if (obj.isSomeObject()) {    // some logic  } else if (obj.isOtherSomething()) {    obj = calulateNewObject(obj);    // some logic  }   ....  else {    // some logic  }  return obj;}

Теперь, в случае obj == null, рассматриваемому объекту присвоится какое-то значение, и отсутствующий else приведет к тому, что вновь присвоенный объект начнет проверяться по цепочке if-else-if, в то время как предполагалось, что объект сразу же вернется из метода.

Заключение

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

Как вы поняли, анализаторам свойственны ложные срабатывания (в том числе и PVS-Studio Java). Это может быть результатом как явной недоработки, так и слишком запутанного кода (увы, анализатор не разобрался). Нужно к ним относиться с пониманием и без стеснения сразу же отписываться, а пока ложные срабатывания ждут своего исправления, можно воспользоваться одним из способов подавления предупреждений.

В заключение предлагаю лично "пощупать" анализатор, скачав его с нашего сайта.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Maxim Stefanov. Checking the Code of XMage, and Why You Won't Be Able to Get the Special Rare Cards of the Dragon's Maze Collection.

Хотите увидеть новую порцию ошибок, найденных статическим анализатором PVS-Studio для Java? Тогда присоединяйтесь к прочтению статьи! В этот раз объектом проверки стал проект Bouncy Castle. Самые интересные фрагменты кода, как обычно, ждут вас ниже.

Немного о PVS-Studio

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

Анализатор для Java самое молодое направление PVS-Studio. Несмотря на это, в поиске дефектов в коде он не уступает своим старшим братьям. Это связано с тем, что Java анализатор использует всю мощь механизмов из C++ анализатора. Об этом уникальном союзе Java и С++ можно почитать здесь.

На данный момент для более удобного использования существуют плагины для Gradle, Maven и IntelliJ IDEA. Если вы знакомы с платформой непрерывного контроля качества SonarQube, то, возможно, вам будет интересна идея поиграться с интеграцией результата анализа.

Немного о Bouncy Castle

Bouncy Castle это пакет с реализацией криптографических алгоритмов, написанный на языке программирования Java (также существует реализация на C#, но в данной статье речь пойдет не об этом). Данная библиотека дополняет стандартное криптографическое расширение (JCE) и содержит API, подходящий для использования в любой среде (включая J2ME).

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

Приступаем к проверке

Bouncy Castle довольно серьезный проект, ведь любая ошибка в такой библиотеке может снизить надежность системы шифрования. Поэтому сначала мы даже сомневались, сможем ли мы найти в данной библиотеке хоть что-то интересное, или же все ошибки уже были найдены и исправлены до нас. Скажем сразу, что наш Java анализатор нас не подвел :)

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

А мы приступим к рассмотрению самых интересных фрагментов кода, которые были обнаружены.

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

V6019 Unreachable code detected. It is possible that an error is present. XMSSTest.java(170)

public void testSignSHA256CompleteEvenHeight2() {    ....    int height = 10;    ....    for (int i = 0; i < (1 << height); i++) {        byte[] signature = xmss.sign(new byte[1024]);        switch (i) {            case 0x005b:                assertEquals(signatures[0], Hex.toHexString(signature));                break;            case 0x0822:                assertEquals(signatures[1], Hex.toHexString(signature));                break;            ....        }    }}

Значение переменной height в методе не меняется, поэтому счетчик i в цикле for не может быть больше, чем 1024 (1 << 10). Однако, в операторе switch второй case проверяет i на соответствие значению 0x0822 (2082). Естественно, проверка байта signatures[1] никогда не будет выполнена.

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

Идентичные подвыражения

V6001 There are identical sub-expressions 'tag == PacketTags.SECRET_KEY' to the left and to the right of the '||' operator. PGPUtil.java(212), PGPUtil.java(212)

public static boolean isKeyRing(byte[] blob) throws IOException {    BCPGInputStream bIn = new BCPGInputStream(new ByteArrayInputStream(blob));    int tag = bIn.nextPacketTag();    return tag == PacketTags.PUBLIC_KEY || tag == PacketTags.PUBLIC_SUBKEY        || tag == PacketTags.SECRET_KEY || tag == PacketTags.SECRET_KEY;}

В данном фрагменте кода в операторе return дважды производится проверка tag == PacketTags.SEKRET_KEY. По аналогии с проверкой публичного ключа, последняя проверка должна быть на равенство tag и PacketTags.SECRET_SUBKEY.

Идентичный код в if / else

V6004 The 'then' statement is equivalent to the 'else' statement. BcAsymmetricKeyUnwrapper.java(36), BcAsymmetricKeyUnwrapper.java(40)

public GenericKey generateUnwrappedKey(....) throws OperatorException {    ....    byte[] key = keyCipher.processBlock(....);    if (encryptedKeyAlgorithm.getAlgorithm().equals(....)) {        return new GenericKey(encryptedKeyAlgorithm, key);    } else {        return new GenericKey(encryptedKeyAlgorithm, key);    }}

В данном примере метод возвращает одинаковые экземпляры класса GenericKey вне зависимости от того, выполнено условие в if или нет. Понятно, что код в ветвях if / else должен отличаться, иначе проверка в if вообще не имеет смысла. Здесь программиста явно подвел копипаст.

Выражение всегда ложно

V6007 Expression '!(nGroups < 8)' is always false. CBZip2OutputStream.java(753)

private void sendMTFValues() throws IOException {    ....    int nGroups;    ....    if (nMTF < 200) {        nGroups = 2;    } else if (nMTF < 600) {        nGroups = 3;    } else if (nMTF < 1200) {        nGroups = 4;    } else if (nMTF < 2400) {        nGroups = 5;    } else {        nGroups = 6;    }    ....    if (!(nGroups < 8)) {        panic();    }}

Здесь переменной nGroups в блоках кода if / else присваивается значение, которое используется, но нигде не меняется. Выражение в операторе if всегда будет ложным, т.к. все возможные значения для nGroups: 2, 3, 4, 5 и 6 меньше 8.

Анализатор понимает, что метод panic() никогда не будет выполнен, и поэтому бьет тревогу. Но здесь, скорее всего, было использовано "защитное программирование", и беспокоиться не о чем.

Добавление одинаковых элементов

V6033 An item with the same key 'PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC' has already been added. PKCS12PBEUtils.java(50), PKCS12PBEUtils.java(49)

class PKCS12PBEUtils {    static {        ....        keySizes.put(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC,                     Integers.valueOf(192));        keySizes.put(PKCSObjectIdentifiers.pbeWithSHAAnd2_KeyTripleDES_CBC,                     Integers.valueOf(128));        ....        desAlgs.add(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC);        desAlgs.add(PKCSObjectIdentifiers.pbeWithSHAAnd3_KeyTripleDES_CBC);    }}

Эта ошибка снова из-за копипаста. В контейнер desAlgs добавляются два одинаковых элемента. Разработчик скопировал последнюю строчку кода, но исправить цифру 3 на 2 в имени поля забыл.

Индекс за пределами диапазона

V6025 Possibly index 'i' is out of bounds. HSSTests.java(384)

public void testVectorsFromReference() throws Exception {    List<LMSigParameters> lmsParameters = new ArrayList<LMSigParameters>();    List<LMOtsParameters> lmOtsParameters = new ArrayList<LMOtsParameters>();    ....    for (String line : lines) {                ....        if (line.startsWith("Depth:")) {            ....        } else if (line.startsWith("LMType:")) {            ....            lmsParameters.add(LMSigParameters.getParametersForType(typ));        } else if (line.startsWith("LMOtsType:")) {            ....            lmOtsParameters.add(LMOtsParameters.getParametersForType(typ));        }    }    ....    for (int i = 0; i != lmsParameters.size(); i++) {        lmsParams.add(new LMSParameters(lmsParameters.get(i),                                        lmOtsParameters.get(i)));    }}

Добавление элементов в коллекции lmsParameters и lmOtsParameters производится в первом цикле for, в разных ветках оператора if / else. Затем, во втором цикле for, осуществляется доступ к элементам коллекций по индексу i. При этом проверяется только то, что индекс i меньше, чем размер первой коллекции, а размер второй коллекции в цикле for не проверяется. Если размеры коллекций окажутся разными, то, вполне вероятно, можно получить IndexOutOfBoundsException. Правда, стоит отметить, что это код тестового метода, и особой опасности данное предупреждение не представляет, т.к. коллекции заполняются тестовыми данными из заранее созданного файла и, естественно, по окончанию добавления элементов, коллекции имеют одинаковый размер.

Использование до проверки на null

V6060 The 'params' reference was utilized before it was verified against null. BCDSAPublicKey.java(54), BCDSAPublicKey.java(53)

BCDSAPublicKey(DSAPublicKeyParameters params) {    this.y = params.getY();    if (params != null) {        this.dsaSpec = new DSAParameterSpec(params.getParameters().getP(),                                            params.getParameters().getQ(),                                            params.getParameters().getG());    } else {        this.dsaSpec = null;    }    this.lwKeyParams = params;}

В первой строке метода переменной y присваивается значение params.getY(). Сразу же после присваивания переменная params проверяется на null. Если допускается, что в данном методе params может быть null, следовало сделать данную проверку перед тем, как использовать переменную.

Избыточная проверка в if / else

V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. EnrollExample.java(108), EnrollExample.java(113)

public EnrollExample(String[] args) throws Exception {    ....    for (int t = 0; t < args.length; t++) {        String arg = args[t];        if (arg.equals("-r")) {            reEnroll = true;        } ....        else if (arg.equals("--keyStoreType")) {            keyStoreType = ExampleUtils.nextArgAsString                           ("Keystore type", args, t);            t += 1;        } else if (arg.equals("--keyStoreType")) {            keyStoreType = ExampleUtils.nextArgAsString                           ("Keystore type", args, t);            t += 1;        } ....    }}

В операторе if / else значение строки args дважды проверяется на равенство со строкой "--keyStoreType". Естественно, вторая проверка избыточна, и никакого смысла в ней нет. Однако, на ошибку это не похоже, т.к. в тексте справки по аргументам командной строки нет других параметров, которые бы не были обработаны в блоке if / else. Скорее всего, это избыточный код, который стоит удалить.

Метод возвращает одно и то же значение

V6014 It's odd that this method always returns one and the same value. XMSSSigner.java(129)

public AsymmetricKeyParameter getUpdatedPrivateKey() {    // if we've generated a signature return the last private key generated    // if we've only initialised leave it in place    // and return the next one instead.    synchronized (privateKey) {        if (hasGenerated) {            XMSSPrivateKeyParameters privKey = privateKey;            privateKey = null;            return privKey;        } else {            XMSSPrivateKeyParameters privKey = privateKey;            if (privKey != null) {                privateKey = privateKey.getNextKey();            }            return privKey;        }    }}

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

Подведем итоги

Как вы видите, ошибки в проекте Bouncy Castle нам все же удалось обнаружить, и это только лишний раз подтверждает, что никто из нас не пишет идеальный код. Разные факторы могут сработать: разработчик устал, невнимателен, его кто-то отвлек Пока код пишет человек ошибки будут встречаться всегда.

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

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

В общем, обязательно используйте статический анализ в своих проектах! Мы сами это делаем, и вам рекомендуем :)


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Irina Polynkina. Unicorns on Guard for Your Safety: Exploring the Bouncy Castle Code.

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

За уходящий год мы (Java-команда PVS-Studio) разобрали в наших статьях ошибки из пяти open-source проектов и совсем немного рассказали про нашу внутреннюю кухню:

• АНБ, Ghidra и единороги
• Под капотом PVS-Studio для Java: разработка диагностик
• Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze
• Проверка WildFly сервера JavaEE приложений
• Единороги на страже вашей безопасности: исследуем код Bouncy Castle
• Big/Bug Data: анализируем исходный код Apache Flink

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

Десятое место: "Обманчивое равенство"

Источник: Big/Bug Data: анализируем исходный код Apache Flink

V6001 There are identical sub-expressions 'processedData' to the left and to the right of the '==' operator. CheckpointStatistics.java(229)

@Overridepublic boolean equals(Object o) {  ....  CheckpointStatistics that = (CheckpointStatistics) o;  return id == that.id &&    savepoint == that.savepoint &&    triggerTimestamp == that.triggerTimestamp &&    latestAckTimestamp == that.latestAckTimestamp &&    stateSize == that.stateSize &&    duration == that.duration &&    alignmentBuffered == that.alignmentBuffered &&    processedData == processedData &&                // <=    persistedData == that.persistedData &&    numSubtasks == that.numSubtasks &&    numAckSubtasks == that.numAckSubtasks &&    status == that.status &&    Objects.equals(checkpointType, that.checkpointType) &&    Objects.equals(      checkpointStatisticsPerTask,       that.checkpointStatisticsPerTask);}

Простая и очень обидная ошибка из-за невнимательности: поле processedData сравнивается с самим собой. Из-за этой ошибки сравнение объектов типа CheckpointStatistics иногда будет выдавать ложноположительный результат. Но основная опасность этой опечатки состоит в том, что equals крайне активно используется в коллекциях, и некорректная реализация этого метода может привести к очень странному поведению, на отладку которого уйдёт огромное количество времени.

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

Девятое место: "Недостижимый код"

Источник: Единороги на страже вашей безопасности: исследуем код Bouncy Castle.

V6019 Unreachable code detected. It is possible that an error is present. XMSSTest.java(170)

public void testSignSHA256CompleteEvenHeight2() {    ....    int height = 10;    ....    for (int i = 0; i < (1 << height); i++) {        byte[] signature = xmss.sign(new byte[1024]);        switch (i) {            case 0x005b:                assertEquals(signatures[0], Hex.toHexString(signature));                break;            case 0x0822:                assertEquals(signatures[1], Hex.toHexString(signature));                break;            ....        }    }}

Ветвь switch для значения i == 0x0822(2082) оказалась недостижимой. Как же так получилось?

Если обратить внимание на условие цикла 1 << height, где height всегда равен 10, то всё сразу встанет на свои места. Согласно условию цикла, счётчик i в цикле for не может быть больше, чем 1024 (1 << 10). Естественно, выполнение рассматриваемой ветви switch никогда не произойдет.

Восьмое место: "Проаннотированный метод"

Источник: Под капотом PVS-Studio для Java: разработка диагностик.

V6009 Collection is empty. The call of the 'clear' function is senseless. MetricRepositoryRule.java(90)

protected void after(){  this.metricsById.clear();  this.metricsById.clear();}

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

• Чистый ли это метод,
• Какие накладываются ограничения на аргументы,
• Возвращаемый результат,
• и всякое прочее.

Некоторые аннотации анализатор выводит сам из исходного кода, некоторые мы проставляем вручную (например, для методов стандартной библиотеки). История этой ошибки началась с того, что мы не в полной мере проаннотировали метод Map#clear. После того, как мы это заметили и исправили, на наших тестовых проектах повылезали новые срабатывания, среди которых был и наш интересный случай.

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

private final Map<String, Metric> metricsByKey = new HashMap<>();private final Map<Long, Metric> metricsById = new HashMap<>();

У класса есть два поля с похожими именами metricsById и metricsByKey. Это и наталкивает на мысль, что автор кода хотел очистить оба словаря, но этого не произошло. Таким образом, два словаря, которые хранят связанные данные, будут рассинхронизированы после вызова after.

Седьмое место: "Ожидание / реальность"

Источник: Проверка WildFly сервера JavaEE приложений.

V6058 The 'equals' function compares objects of incompatible types: String, ModelNode. JaxrsIntegrationProcessor.java(563)

// Send value to RESTEasy only if it's not null, empty string, or the // default value.private boolean isTransmittable(AttributeDefinition attribute,                                ModelNode modelNode) {  if (modelNode == null || ModelType      .UNDEFINED.equals(modelNode.getType())) {    return false;  }  String value = modelNode.asString();  if ("".equals(value.trim())) {    return false;  }  return !value.equals(attribute.getDefaultValue());        // <=}

Обратив внимание на предшествующий методу комментарий, можно ожидать, что метод вернет true, если:

• modelNode не null,
• строковое представление modelNode не пустое,
• modelNode не значение по умолчанию.

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

Строковое представление modelNode сравнивается с объектом типа ModelNode, и, как можно догадаться, такое сравнение всегда будет возвращать отрицательный результат из-за несовместимости типов.

Последствия ошибки: непредвиденное разрешение к отправке значения modelNode, когда оно равно значению по умолчанию (attribute.getDefaultValue()).

Шестое место: "Копипаст-ориентированное программирование"

Источник: Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze.

V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SubTypeChangingEffectsTest.java(162), SubTypeChangingEffectsTest.java(158), SubTypeChangingEffectsTest.java(156), SubTypeChangingEffectsTest.java(160)

@Testpublic void testArcaneAdaptationGiveType() {    addCard(Zone.HAND, playerA, "Arcane Adaptation", 1);    addCard(Zone.BATTLEFIELD, playerA, "Island", 3);    addCard(Zone.HAND, playerA, "Silvercoat Lion");    addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=    addCard(Zone.HAND, playerB, "Silvercoat Lion");    addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=    ....    for (Card card : playerB.getGraveyard().getCards(currentGame)) {        if (card.isCreature()) {            Assert.assertEquals(card.getName() + " should not have ORC type",                    false, card.getSubtype(currentGame).contains(SubType.ORC));            Assert.assertEquals(card.getName() + " should have CAT type",                    true, card.getSubtype(currentGame).contains(SubType.CAT));        }    }}

В этом году, как и в прошлом (Топ 10 ошибок за 2019), классная copy-paste ошибка от диагностического правила V6072 заслуживает место в десятке.

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

В данном фрагменте кода так и произошло. Автор теста имитировал игру между игроками, раскидывая между ними одинаковые карты по игровым зонам, но из-за copy-paste игроку playerA дважды досталась одна и та же карта. Из-за этого игровая зона Zone.GRAVEYARD игрока playerB осталась без тестирования. Подробное описание ошибки можно почитать в самой статье.

Пятое место: "Ненормальное распределение"

Источник: Big/Bug Data: анализируем исходный код Apache Flink

V6048 This expression can be simplified. Operand 'index' in the operation equals 0. CollectionUtil.java(76)

public static <T> Collection<List<T>> partition(Collection<T> elements, int numBuckets) {  Map<Integer, List<T>> buckets = new HashMap<>(numBuckets);    int initialCapacity = elements.size() / numBuckets;  int index = 0;  for (T element : elements)   {    int bucket = index % numBuckets;                                 // <=    buckets.computeIfAbsent(bucket,                             key -> new ArrayList<>(initialCapacity))           .add(element);   }  return buckets.values();}

Ошибка была обнаружена в утилитном методе partition, который разбивает переданную коллекцию elements на numBuckets коллекций. Суть ошибки в том, что индекс коллекции bucket, в которую хотят поместить каждый рассматриваемый элемент, имеет константное значение (0). Причиной этому служит то, что разработчик забыл инкрементировать переменную index на каждой итерации цикла.

Вследствие чего метод partition будет всегда возвращать коллекцию elements, обернутую в другую коллекцию. А это вряд ли задуманное поведение.

Четвертое место: "Бомба замедленного действия"

Источник: АНБ, Ghidra и единороги.

V6008 Null dereference of 'selectedNode' in function 'setViewPanel'. OptionsPanel.java(266)

private void processSelection(OptionsTreeNode selectedNode) {  if (selectedNode == null) {    setViewPanel(defaultPanel, selectedNode); // <=    return;  }  ....}private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {  ....  setHelpLocation(component, selectedNode);  ....}private void setHelpLocation(JComponent component, OptionsTreeNode node) {  Options options = node.getOptions();  ....}

В приведенном фрагменте кода явно напортачили. Если вы проследите за selectedNode из processSelection(), когда selectedNode == null, то сразу же обнаружите, что при таком исходе нас ждет неминуемый NullPointerException. О чем и предупреждает нас анализатор.

Но, изучив немного код, автор статьи пришел к выводу, что выполнение программы никогда не встретится с NullPointerException, так как processSelection() вызывается всего в двух местах, перед вызовом которых selectedNode явно проверяется на null.

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

Третье место: "Всегда false"

Источник: Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze.

V6007 Expression 'filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")' is always false. SetPowerToughnessAllEffect.java(107)

@Overridepublic String getText(Mode mode) {  StringBuilder sb = new StringBuilder();  ....  if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {    sb.append(" has base power and toughness ");  } else {    sb.append(" have base power and toughness ");  }  ....  return sb.toString();}

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

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

Второе место: "2-в-1"

Источник: АНБ, Ghidra и единороги.

V6007 Expression 'index >= 0' is always true. ExternalNamesTableModel.java(105)

V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java(109)

public void setValueAt(Object aValue, int row, int column) {  ....  int index = indexOf(newName);  if (index >= 0) {                  // <=    Window window = tool.getActiveWindow();    Msg.showInfo(getClass(), window, "Duplicate Name",                 "Name already exists: " + newName);    return;  }  ExternalPath path = paths.get(row); // <=  ....}private int indexOf(String name) {  for (int i = 0; i < paths.size(); i++) {    ExternalPath path = paths.get(i);    if (path.getName().equals(name)) {      return i;    }  }  return 0;}

Метод indexOf всегда возвращает неотрицательное число. А всё из-за того, что автор метода в случае отсутствия искомого newName по ошибке возвращает 0, а не -1. Такая ошибка приводит к тому, что поток выполнения программы всегда будет заходить в then-ветку условного оператора if (index >= 0), в котором будет выдавать сообщение о существующем newName и успешно выходить из метода, даже тогда, когда в реальности newName не был найден.

Но и это ещё не всё. Так как then-ветка условного оператора прекращает выполнение метода, то до кода после условного оператора дело так и не дойдет.

Об этом и предупреждает нас анализатор.

Первое место: "А то ли мы проверили?"

Источник: Под капотом PVS-Studio для Java: разработка диагностик.

V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition. Menu.java(40)

public class Menu{  private Map<String, List<String>> menus = new HashMap<String, List<String>>();  public void putMenuItem(String menu, String item)  {    List<String> items = menus.get(menu);    if (item == null)                      // <=    {      items = new ArrayList<String>();      menus.put(menu, items);    }    items.add(item);  }  ....}

По задумке автора предполагалось создать коллекцию по ключу menu, если таковой ещё не было. Но проверка не той переменной разрушила всю задумку, прорубив лазеечку для NullPointerException. Метод выбросит исключение, когда в словаре ключ menu будет отсутствовать, и значение item, которое хотели добавить, не будет null.

Заключение

Проверки open-source проектов с помощью PVS-Studio из года в год доказывают, что такой рубеж защиты, как статический анализ кода, должен обязательно присутствовать в разработке. Каким бы вы мастером своего дела ни были, ошибки обязательно найдут лазеечку в ваш проект, и причин этому множество: устали, завал на работе или вовсе отвлеклись на котиков. А если вы работаете в команде, то количество возможностей попасть ошибкам в код вырастает пропорционально количеству коллег.

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


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Maxim Stefanov. Top-10 Bugs in Java Projects in 2020.

2019 был очень насыщенным годом в плане конференций. Наша команда могла уезжать на целые недели в командировки. А как известно, конференция время делиться знаниями. Помимо того, что мы выступали с докладами и много интересного рассказывали на нашем стенде, мы также узнавали много нового от общения с участниками конференции и от докладчиков. Так вот на осенней конференции Joker 2019 доклад от Dalia Abo Sheasha Migrating beyond Java 8 вдохновил нас на реализацию нового диагностического правила, которое позволяет выявлять несовместимости в Java SE API между разными версиями Java. Об этом и пойдет речь.

Актуальность выявления проблем совместимости Java SE

На текущей момент уже вышла Java SE 14. Несмотря на это, многие компании продолжают использовать прежние версии Java (Java SE 6, 7, 8, ...). В связи с тем, что время идет, и Java все время обновляется, то проблема совместимости различных версий Java SE API с каждым годом становится все актуальней.

Когда выходят новые версии Java SE, то они, как правило, обратно совместимы с более ранними версиями, то есть, например, приложение разработанное на основе Java SE 8 должно без проблем запуститься на 11 версии Java. Однако на практике может возникать некоторая несовместимость в ряде классов и методов. Эта несовместимость заключаются в том, что некоторые API претерпевают изменения: удаляются, меняются в поведении, помечаются как устаревшие и многое другое.

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

Думаю, этого вполне достаточно, чтобы заострить внимание!

Существующий инструментарий

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

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

Покопавшись в просторах интернета на тему выявления несовместимости API между разными Java SE, мне встретились только инструменты, которые идут с JDK: javac, jdeps, jdeprscan.

Стороннего инструмента в этом деле так и не нашлось, кроме того, с которым я имел честь познакомиться на докладе Joker 2019 Migration Toolkit for Application Binaries.

javac

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

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

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

jdeps

Если вы уже озадачились вопросом миграции вашего приложения на более свежую версию Java SE, то в помощь вам спешит jdeps.

jdeps инструмент командной строки, производящий статический анализ зависимостей вашего приложения и библиотек, принимая на вход *.class файлы или *.jar. Начиная с Java SE 8, он идет в комплекте с JDK.

И тут нас интересует то, что если вы запустите этот инструмент с опцией --jdk-internals, то он вам сообщит, от какого внутреннего JDK API зависит каждый ваш класс. Это очень важный момент в силу того, что внутренний API не гарантирует, что он не изменится в следующих версиях Java.

Давайте рассмотрим пример. Допустим вы долгое время разрабатывали свое приложение на Java 8 и никогда не озадачивались совместимостью используемого Java SE API с более свежими версиями. Встал вопрос о миграции вашего приложения на, например, Java 11. В таком случае можно пойти по пути минимального сопротивления и сразу же запустить приложение при помощи Java 11. Но что-то пошло не так, и, предположим, запуск вашего приложения закончился падением. В таком случае можно запустить jdeps из Java 11, натравив на файлы *.jar вашего приложения.

Результат примерно будет следующим:



Из вывода видно, что зависимость sun.misc.BASE64Encoder в Java 11 будет удалена. И падение вашего приложения, когда сперва запускали на Java 11, скорее всего связано с ошибкой java.lang.NoClassDefFoundError. Помимо этого информирования, что не может ни радовать, jdeps может предлагать вам альтернативную зависимость, которую можно использовать вместо текущей. В данном случае он предложил удаленную зависимость заменить на java.util.Base64.

Второе предупреждение инструмента сигнализирует о том, что в коде мы еще используем другую внутреннюю зависимость, но пока она корректна. Будут ли изменения этого API в следующих версии Java неизвестно, но это нужно принять во внимание.

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

jdeprscan

Цели у jdeprscan ровно такие же, как и у jdeps, а именно, помощь в поиске нежелательного и проблемного API.

jdeprescan инструмент статического анализа, который сканирует *.jar файл (или некоторую другую совокупность *.class файлов) на предмет использования устаревших элементов API. Использование устаревшего API не является блокирующей проблемой, однако на него следует обращать внимание. Начиная с Java SE 9, он идет в комплекте с JDK.

Также предположим, что стоит вопрос миграции приложения на Java 11. В таком случае, запустив команду

jdeprscan --release 8 app.jar

вы получите список API, который стал нерекомендуемым для Java 8, то есть тот API, который в будущих версиях Java может быть удален. Исправив все предупреждения, можно запустить

jdeprscan --release 11 app.jar

который выдаст список API, устаревший уже для Java 11. Таким образом, можно найти и исправить (если потребуется) весь нерекомендуемый API.

Migration Toolkit for Application Binaries

Этот инструмент ориентирован на помощь в быстром оценивании пользовательского приложения на наличие потенциальных проблем перед развертыванием на различных серверах (JBOSS, WebShere, Tomcat, WebLogic, ...). Помимо всего функционала инструмент также позволяет выявлять различия в Java SE API разных версий.

Вкратце взглянем, что из себя представляет этот инструмент.

Быстрый запуск инструмента выглядит так:

java -jar binaryAppScanner.jar yourApp.jar --analyzeJavaSE --sourceJava=oracle8 --targetJava=java11 ....

Опция analyzeJavaSE подразумевает использование различных параметров, о которых вы можете узнать, вызвав help.

После запуска анализа вам вскоре откроется отчет в веб-браузере:



На скрине не все поместилось =(. А пока вы еще не пробовали запускать у себя этот инструмент, я опишу словами.

В отчете можно увидеть срабатывания правил с 3-мя уровнями серьезности:

• Правила с высоким уровнем серьезности (удаленные API, изменение поведения, которое делает приложение неработоспособным и требует исправления);
• Правила-предупреждения (изменение поведения API, которое может привести к проблемам и их стоит изучить);
• Информационные правила (использование устаревших API, поведение API, которое может незначительно изменить поведение, но на программу не влияет).

Каждое срабатывание можно развернуть и увидеть описание что да как. В информационных сообщениях есть рекомендация, мол запустите jdeps дополнительно помимо нас. Мол мы ориентированы на миграцию приложения, а jdeps дополнительно поможет обнаружить проблему во внутренних пакетах JDK (помимо тех, что находят они).

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

Если вы используете Eclipse IDE, то вы можете воспользоваться плагином. Для более детального изучения стоит посетить их страничку.

Реализация в PVS-Studio

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

Рассмотренные инструменты действительно облегчат задачу миграции приложения или поиска API, который может ломать вам работоспособность приложения на более свежих версиях Java SE (при нежелании мигрировать приложение). Но подумав, что эти инструменты все время нужно запускать в командной строке отдельно от процесса разработки для выявления проблем, пришли к решению, что это не совсем удобно. И отталкиваясь от того, что статический анализ необходим именно для обнаружения проблемного или потенциально проблемного кода на самых ранних этапах разработки, реализовали диагностическое правило V6078, которое и будет вам сигнализировать о проблемном API.

Правило V6078 заранее предупредит вас о том, что ваш код зависим от некоторых функций и классов Java SE API, которые на следующих версиях Java могут доставить вам трудности. И вы еще на самом старте реализации той или иной фичи не будете завязываться на этот API, тем самым уменьшая технические риски в будущем.

Диагностическое правило выдает предупреждения в следующих случаях:

• Если метод/класс/пакет удалены в целевой версии Java;
• Если метод/класс/пакет помечены как устаревшие в целевой версии Java;
• Если у метода изменилась сигнатура.

Правило на данный момент позволяет проанализировать совместимость Oracle Java SE с 8 по 14 версий. Чтобы правило стало активным, его необходимо настроить.

IntelliJ IDEA

В IntelliJ IDEA плагине вам необходимо во вкладке Settings > PVS-Studio > API Compatibility Issue Detection включить правило и указать параметры, а именно:

• Source Java SE версия Java, на которой разработано ваше приложение;
• Target Java SE версия Java, с которой вы хотите проверить совместимость используемого API в вашем приложении (Source Java SE);
• Exclude packages пакеты, которые вы хотите исключить из анализа совместимости (пакеты перечисляются через запятую).

Плагин для Gradle

Используя gradle плагин, вам необходимо сконфигурировать настройки анализатора в build.gardle:

apply plugin: com.pvsstudio.PvsStudioGradlePluginpvsstudio {    ....    compatibility = true    sourceJava = /*version*/      targetJava = /*version*/    excludePackages = [/*pack1, pack2, ...*/]}

Плагин для Maven

Используя maven плагин, вам необходимо сконфигурировать настройки анализатора в pom.xml:

<build>  <plugins>    <plugin>      <groupId>com.pvsstudio</groupId>      <artifactId>pvsstudio-maven-plugin</artifactId>      ....      <configuration>        <analyzer>          ....          <compatibility>true</compatibility>          <sourceJava>/*version*/</sourceJava>          <targetJava>/*version*/</targetJava>          <excludePackages>/*pack1, pack2, ...*/</excludePackages>                </analyzer>      </configuration>    </plugin>  </plugins></build>

Использование ядра напрямую

Если вы используете анализатор напрямую через командную строку, то, чтобы активировать анализ совместимости выбранных Java SE API, необходимо использовать следующие параметры:

java -jar pvs-studio.jar /*other options*/ --compatibility --source-java /*version*/ --target-java /*version*/ --exclude-packages /*pack1 pack2 ... */

Срабатывание анализатора

Давайте предположим, что мы разрабатываем приложение на базе Java SE 8, и у нас есть класс со следующим содержимым:

/* imports */import java.util.jar.Pack200;public class SomeClass{  /* code */  public static void someFunction(Pack200.Packer packer, ...)  {    /* code */    packer.addPropertyChangeListener(evt -> {/* code */});    /* code */  }}

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

• Source Java SE 8, Target Java SE 9
  • The 'addPropertyChangeListener' method will be removed.
  
  
• Source Java SE 8, Target Java SE 11
  • The 'addPropertyChangeListener' method will be removed.
  • The 'Pack200' class will be marked as deprecated.
  
  
• Source Java SE 8, Target Java SE 14
  • The 'Pack200' class will be removed.
  
  

Сначала в Java SE 9 был удален метод 'addPropertyChangeListener' в классе 'Pack200.Packer'. В 11 версии к этому добавился тот факт, что класс 'Pack200' пометили как устаревший. А в 14 версии вовсе этот класс был удален.

Поэтому, запустив приложение на Java 11, вы получите 'java.lang.NoSuchMethodError', а если запустите на Java 14 'java.lang.NoClassDefFoundError'.

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

Идеи дальнейшего развития

В ходе реализации диагностического правила возникли идеи по расширению:

• В связи с тем, что jdeprscan и jdeps не могут предупреждать об использовании рефлексии для доступа к инкапсулированному API, то имеет смысл доработать правило так, чтобы оно пыталось выяснять какой все-таки API пытаются использовать. Результат может получиться далеко не идеальным, но, а почему нет?!
• Существует большое разнообразие реализации JDK (от Oracle, IBM, Red Hat, ...). Как правило, они совместимы между собой. Но как значительно различается внутренний JDK API? Ведь разработчики могут завязываться на него, что может приводить к потенциальным проблемам при миграции приложения с одного JDK на другой.

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

Заключение

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

Диагностика V6078 доступна в анализаторе, начиная с версии 7.08. Скачать и попробовать анализатор на своём проекте можно на странице загрузки.



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Maxim Stefanov. The PVS-Studio analyzer: detecting potential compatibility issues with Java SE API.

Для разнообразия сегодня немного расскажем про процесс разработки и доработки диагностических правил для PVS-Studio Java. Посмотрим, почему старые срабатывания анализатора не слишком сильно плавают от релиза к релизу, а новые не слишком сумасшедшие. А ещё немного заспойлерим "чего там у джавистов в планах" и покажем парочку красивых (и не очень) ошибок, найденных с помощью диагностик из следующего релиза.

Процесс разработки диагностик и SelfTester

Естественно, что каждое новое диагностическое правило начинается с идеи. А так как Java-анализатор самое молодое направление развития PVS-Studio, то в основном эти идеи мы воруем у C/C++ и C# отделов. Но не всё так плохо: правила, придуманные самостоятельно (в том числе и пользователями спасибо!), мы тоже добавляем, чтобы потом те же самые отделы их своровали у нас. Круговорот, как говорится.

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

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

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

• Правило не упадёт на open-source безумии, где частенько встречаются "интересные" решения;
• Нет явно ложных срабатываний (или же их очень-очень мало, и мы с ними не можем ничего поделать по тем или иным соображениям);
• Новые вручную добавленные аннотации для data-flow (механизм анализа потока данных) ничего не сломали и принесли чего-нибудь интересного;
• После изменений в ядре анализатор не развалился на всё том же open-source безумии;
• Новое правило не увеличивает время анализа проектов на over 9000%;
• Мы не потеряли "хороших" срабатываний, которые были раньше;
• Всякое прочее в этом духе.

В общем, здесь, подобно рыцарю на коне (немного хромающем, но мы над этим работаем), на передний план выходит SelfTester. Его главная и единственная задача автоматически проверить пачку проектов и показать, какие срабатывания добавились, исчезли или изменились относительно "эталона" в системе контроля версий. Предоставить диффы по отчёту анализатора и показать соответствующий код в проектах, короче говоря. На данный момент SelfTester для Java проверяет 62 open-source проекта бородатых версий, среди которых числятся, например, DBeaver, Hibernate и Spring. Один полный прогон всех проектов занимает 2-2.5 часа, что, несомненно, больно, но ничего не поделаешь.


На скриншоте выше "зелёные" проекты это те, в которых ничего не изменилось. Каждый дифф в "красных" проектах просматривается вручную и, если он корректен, подтверждается той самой кнопкой "Approve". Дистрибутив анализатора, кстати, соберётся только в случае, если SelfTester даёт чисто зелёный результат. Так мы, в общем-то, и поддерживаем консистентность результатов между разными версиями.

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

• Наиболее общая реализация правила по синтетическим тестам, необязательно полная. Например, "найти все использования паттерна double-checked locking" при поиске некорректных реализаций этого паттерна;
• Полный прогон SelfTester-а, обычно на ночь;
• По диффам отбираются и реализуются несколько признаков ложных срабатываний, которые добавляются в юнит-тесты;
• Прогон SelfTester-а по проектам, в которых ещё остались срабатывания;
• Повтор пунктов 3-4, пока остаются ложные срабатывания;
• Финальный просмотр диффов, их утверждение, подготовка документации (той самой, что на сайте);
• Добавление диагностики, нового эталона и документации в master.

К счастью, полные прогоны SelfTester-а достаточно редки, и "2-2.5 часа" ждать приходится не очень часто. Периодически удача обходит стороной, и срабатывания оказываются на крупных проектах вроде Sakai и Apache Hive самое время попить кофе, попить кофе и попить кофе. Ещё можно позаниматься документацией, но это на любителя.

"А зачем нужны юнит-тесты, раз есть такой волшебный инструмент?"

А затем, что тесты значительно быстрее. Несколько минут и уже есть результат. А ещё они позволяют увидеть, какая конкретно часть правила отвалилась. И, кроме того, не всегда все допустимые срабатывания какого-либо правила отлавливаются в проектах SelfTester-а, но их работоспособность тоже необходимо проверять.

Новые проблемы в старых знакомых

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

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

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

P.S. Сказанное выше не значит, что статический анализ отлавливает только безвредные ошибки в неиспользуемом коде. Мы проверяем релизные (и почти релизные) версии проектов в которых наиболее актуальные баги разработчики и тестировщики (а иногда, к сожалению, и пользователи) находили руками, что долго, дорого и больно. Подробнее про это можно почитать в нашей статье "Ошибки, которые не находит статический анализ кода, потому что он не используется".

Apache Dubbo и пустое меню

GitHub

Диагностика "V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition" уже была зарелизена в версии 7.08, но в наших статьях пока что не появлялась, так что самое время это исправить.

Menu.java:40

public class Menu{  private Map<String, List<String>> menus = new HashMap<String, List<String>>();  public void putMenuItem(String menu, String item)  {    List<String> items = menus.get(menu);    if (item == null)                      // <=    {      items = new ArrayList<String>();      menus.put(menu, items);    }    items.add(item);  }  ....}

Классический пример словаря "ключ-коллекция" и не менее классическая опечатка. Разработчик хотел создать коллекцию, соответствующую ключу, если её ещё не существует, но перепутал название переменной и получил не просто некорректную работу метода, но и NullPointerException в последней строке. Для Java 8 и позже для реализации подобных словарей стоит использовать метод computeIfAbsent:

public class Menu{  private Map<String, List<String>> menus = new HashMap<String, List<String>>();  public void putMenuItem(String menu, String item)  {    List<String> items = menus.computeIfAbsent(menu, key -> new ArrayList<>());    items.add(item);  }  ....}

Glassfish и double-checked locking

GitHub

Одной из диагностик, которые войдут в следующий релиз, является проверка корректной реализации паттерна "double-checked locking". Рекордсменом по срабатываниям из проектов SelfTester-а оказался Glassfish: всего PVS-Studio с помощью этого правила нашёл 10 проблемных мест в проекте. Предлагаю читателю немного развлечься и поискать два из них в отрывке кода ниже. За помощью можно обратиться к документации: "V6082 Unsafe double-checked locking". Ну или, если совсем не хочется, в конец статьи.

EjbComponentAnnotationScanner.java

public class EjbComponentAnnotationScanner{  private Set<String> annotations = null;  public boolean isAnnotation(String value)  {    if (annotations == null)    {      synchronized (EjbComponentAnnotationScanner.class)      {        if (annotations == null)        {          init();        }      }      return annotations.contains(value);    }  }  private void init()  {    annotations = new HashSet();    annotations.add("Ljavax/ejb/Stateless;");    annotations.add("Ljavax/ejb/Stateful;");    annotations.add("Ljavax/ejb/MessageDriven;");    annotations.add("Ljavax/ejb/Singleton;");  }  ....}

SonarQube и data-flow

GitHub

Доработка диагностик заключается не только в прямом изменении их кода с целью отловить больше подозрительных мест или убрать ложные срабатывания. Важную роль в процессе разработки анализатора играет и ручная разметка методов для data-flow например, можно написать, что такой-то библиотечный метод всегда возвращает non-null. При написании новой диагностики мы совершенно случайно обнаружили, что у нас не размечен метод Map#clear(). Не считая явно глупого кода, который начала отлавливать диагностика "V6009 Collection is empty. The call of the 'clear' function is senseless", мы смогли найти отличную опечатку.

MetricRepositoryRule.java:90

protected void after(){  this.metricsById.clear();  this.metricsById.clear();}

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

protected void after(){  this.metricsById.clear();  this.metricsById.clear();}public Metric getByKey(String key){  Metric res = metricsByKey.get(key);  ....}

Именно. У класса есть два поля с похожими именами metricsById и metricsByKey. Я уверен, что в методе after разработчик хотел очистить оба словаря, но либо его подвело автодополнение, либо он по инерции вписал то же имя. Таким образом, два словаря, которые хранят связанные данные, будут рассинхронизированы после вызова after.

Sakai и пустые коллекции

GitHub

Ещё одна новая диагностика, которая войдёт в следующий релиз, "V6084 Suspicious return of an always empty collection". Достаточно легко забыть добавить элементы в коллекцию, а особенно когда каждый элемент надо сначала проинициализировать. Из личного опыта, такие ошибки чаще всего приводят не к падению приложения, а к странному поведению или отсутствию какого-либо функционала.

DateModel.java:361

public List getDaySelectItems(){  List selectDays = new ArrayList();  Integer[] d = this.getDays();  for (int i = 0; i < d.length; i++)  {    SelectItem selectDay = new SelectItem(d[i], d[i].toString());  }  return selectDays;}

В том же классе, кстати, находятся очень похожие методы без такой же ошибки. Например:

public List getMonthSelectItems(){  List selectMonths = new ArrayList();  Integer[] m = this.getMonths();  for (int i = 0; i < m.length; i++)  {    SelectItem selectMonth = new SelectItem(m[i], m[i].toString());    selectMonths.add(selectMonth);  }  return selectMonths;}

Планы на будущее

Не считая разных не очень интересных внутренних вещей, мы думаем насчёт добавления в Java-анализатор диагностик для Spring Framework. Он не только является основным хлебом для джавистов, но и содержит в себе достаточно много неочевидных моментов, на которых можно споткнуться. Мы пока не очень уверены, в каком именно виде эти диагностики в итоге появятся, когда это произойдёт и произойдёт ли вообще. Но зато мы уверены, что нам нужны идеи для них и open-source проекты, использующие Spring, для SelfTester-а. Так что, если есть что-то на примете предлагайте (в комментарии или личные сообщения тоже можно)! А чем больше этого добра мы наберём, тем больше приоритет будет на него смещаться.

Ну и в заключение ошибки в реализации double-checked locking из Glassfish:

• Поле не объявлено как 'volatile'.
• Объект сначала публикуется, а потом инициализируется.

Чем всё это плохо опять же, можно посмотреть в документации.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lazeba. Under the Hood of PVS-Studio for Java: How We Develop Diagnostics.