Топ 10 ошибок в открытых проектах С++ за 2016 год

Пока во всём мире обсуждают 89-ю церемонию вручения наград премии «Оскар» и составляют различные рейтинги актёров и их костюмов, мы решили подготовить обзорную статью в IT-сфере. Речь пойдёт о самых интересных ошибках, допущенных в проектах с открытым исходным кодом в 2016 году. Этот год был примечателен тем, что наш анализатор PVS-Studio стал доступен и в операционных системах, основанных на Linux. Представленные ошибки наверняка уже исправлены, и каждый читатель может убедиться в серьёзности ошибок, которые допускают разработчики.

Итак, рассмотрим, какие интересные ошибки нашел анализатор PVS-Studio в 2016 году. Помимо кода, будет приведена диагностика, с помощью которой ошибка была выявлена, а также статья, в которой данная ошибка была впервые нами описана.

Разделы отсортированы в соответствии с моими представлениями о красоте багов :).

Десятое место

Источник: Находим ошибки в коде компилятора GCC с помощью анализатора PVS-Studio

V519 The 'bb_copy' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1076, 1078. cfg.c 1078

void
free_original_copy_tables (void)
{
  gcc_assert (original_copy_bb_pool);
  delete bb_copy;
  bb_copy = NULL;       // <=
  delete bb_original;   // <=
  bb_copy = NULL;       // <=
  delete loop_copy;
  loop_copy = NULL;
  delete original_copy_bb_pool;
  original_copy_bb_pool = NULL;
}

Случайно дважды обнуляется указатель bb_copy, а указатель bb_original остался необнулённым.

Девятое место

Источник: Долгожданная проверка CryEngine V

V519 The 'BlendFactor[2]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266

void CCryDXGLDeviceContext::
OMGetBlendState(...., FLOAT BlendFactor[4], ....)
{
  CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);
  if ((*ppBlendState) != NULL)
    (*ppBlendState)->AddRef();
  BlendFactor[0] = m_auBlendFactor[0];
  BlendFactor[1] = m_auBlendFactor[1];
  BlendFactor[2] = m_auBlendFactor[2]; // <=
  BlendFactor[2] = m_auBlendFactor[3]; // <=
  *pSampleMask = m_uSampleMask;
}

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

Восьмое место

Источник: GDB оказался крепким орешком

V579 The read_memory function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. jv-valprint.c 111

extern void
read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);

void
java_value_print (....)
{
  ....
  gdb_byte *buf;
  buf = ((gdb_byte *)
    alloca (gdbarch_ptr_bit (gdbarch) / HOST_CHAR_BIT));
  ....
  read_memory (address, buf, sizeof (buf));
  ....
}

Оператор sizeof(buf) вычисляет не размер буфера, а размер указателя. Следовательно, из памяти извлекается недостаточное количество байт данных.

Седьмое место

Источник: Команда PVS-Studio готовит технический прорыв, ну а пока перепроверим Blender

V522 Dereferencing of the null pointer 've' might take place. functions1d.cpp 107

int QuantitativeInvisibilityF1D::operator()(....)
{
  ViewEdge *ve = dynamic_cast<ViewEdge*>(&inter);
  if (ve) {
    result = ve->qi();
    return 0;
  }
  FEdge *fe = dynamic_cast<FEdge*>(&inter);
  if (fe) {
    result = ve->qi(); // <=
    return 0;
  }
  ....
}

Здесь опечатка в наименовании привела к более серьёзной ошибке. По всей видимости, второй фрагмент кода писался с помощью Copy-Paste. И случайно в одном месте забыли поменять имя переменной ve на fe. Как результат, возникнет неопределённое поведение программы, которое может, например, привести к аварийному завершению программы.

Шестое место

Источник: Плохой код пакета для создания 2D-анимаций Toonz

V546 Member of a class is initialized by itself: 'm_subId(m_subId)'. tfarmcontroller.cpp 572

class TaskId
{
  int m_id;
  int m_subId;

public:
  TaskId(int id, int subId = -1) : m_id(id), m_subId(m_subId){};

Интересная ошибка в списке инициализации класса. Поле m_subld инициализируется само собой, хотя, скорее всего, хотели написать m_subId(subId).

Пятое место

Источник: PVS-Studio спешит на помощь CERN: проверка проекта Geant4

V603 The object was created but it is not being used. If you wish to call constructor, 'this->G4PhysicsModelCatalog::G4PhysicsModelCatalog(....)' should be used. g4physicsmodelcatalog.cc 51

class G4PhysicsModelCatalog
{
  private:  
  ....
    G4PhysicsModelCatalog();
  ....
  static modelCatalog* catalog;
  ....
};

G4PhysicsModelCatalog::G4PhysicsModelCatalog()
{ if(!catalog) { 
    static modelCatalog catal;
    catalog = &catal; 
  } 
}

G4int G4PhysicsModelCatalog::Register(const G4String& name)
{
  G4PhysicsModelCatalog();
  .... 
}

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

Четвертое место

Источник: Casablanca: Единорог, который смог

V554 Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. BlackJack_Server140 table.cpp 471

void DealerTable::FillShoe(size_t decks)
{
  std::shared_ptr<int> ss(new int[decks * 52]);
  ....
}

По умолчанию умный указатель типа shared_ptr для уничтожения объекта вызовет оператор delete без квадратных скобок []. В данном случае, это неправильно.

Корректный вариант кода должен быть таким:

std::shared_ptr<int> ss(new int[decks * 52],
                        std::default_delete<int[]>());

Третье место

Источник: Проверка исходного кода игрового движка Serious Engine v.1.10 к юбилею шутера Serious Sam

V541 It is dangerous to print the string 'achrDefaultScript' into itself. dlgcreateanimatedtexture.cpp 359

BOOL CDlgCreateAnimatedTexture::OnInitDialog() 
{
  ....
  // allocate 16k for script
  char achrDefaultScript[ 16384];
  // default script into edit control
  sprintf( achrDefaultScript, ....); // <=
  ....
  // add finishing part of script
  sprintf( achrDefaultScript,        // <=
           "%sANIM_END\r\nEND\r\n",  // <=
           achrDefaultScript);       // <=
  ....
}

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

Для объяснения, почему здесь может получиться неожиданный результат, я процитирую простой пример из документации к диагностике V541:

char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s);

В результате работы этого кода хочется получить строку:

N = 123, S = test

Но на практике в буфере будет сформирована строка:

N = 123, S = N = 123, S =

Что произведёт в нашем случае, предсказать затруднительно, так как это зависит от реализации функции sprintf. Есть шанс, что код отработает так, как ожидал программист. А можно и получить некорректный вариант или падение программы. Код может быть исправлен, если использовать для сохранения результата новый буфер.

Второе место

Источник: PVS-Studio покопался в ядре FreeBSD

V733 It is possible that macro expansion resulted in incorrect evaluation order. Check expression: chan — 1 * 20. isp.c 2301

static void
isp_fibre_init_2400(ispsoftc_t *isp)
....
  if (ISP_CAP_VP0(isp))
    off += ICB2400_VPINFO_PORT_OFF(chan);
  else
    off += ICB2400_VPINFO_PORT_OFF(chan - 1); // <=
  ....
}

На первый взгляд, в этом фрагменте кода нет ничего подозрительного. Иногда используется значение 'chan', иногда на единицу меньше: 'chan — 1', но посмотрим на определение макроса:

#define ICB2400_VPOPT_WRITE_SIZE 20

#define  ICB2400_VPINFO_PORT_OFF(chan) \
  (ICB2400_VPINFO_OFF +                \
   sizeof (isp_icb_2400_vpinfo_t) +    \
  (chan * ICB2400_VPOPT_WRITE_SIZE))          // <=

При передаче в макрос бинарного выражения, там кардинально меняется логика вычислений. Предполагаемое выражение "(chan — 1) * 20" превращается в «chan — 1 *20», т.е. в «chan — 20», и далее в программе используется неверно вычисленный размер.

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

Первое место

Источник: Свежий взгляд на код Oracle VM VirtualBox

V547 Expression is always false. Unsigned type value is never < 0. dt_subr.c 715

#define vsnprintf RTStrPrintfV

int
dt_printf(dtrace_hdl_t *dtp, FILE *fp, const char *format, ...)
{
  ....
  if (vsnprintf(&dtp->dt_buffered_buf[dtp->dt_buffered_offs], // <=
        avail, format, ap) < 0) {
      rval = dt_set_errno(dtp, errno);
      va_end(ap);
      return (rval);
    }
  ....
}

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

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

size_t  RTStrPrintfV(char *, size_t, const char *, va_list args);
int     vsnprintf   (char *, size_t, const char *, va_list arg );

Заключение

В заключение хочу привести самую популярную картинку к статье, которая получила множество восторженных комментариев. Картинка из статьи "Проверка проекта OpenJDK с помощью PVS-Studio ":

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

Скачать и попробовать PVS-Studio можно по этой ссылке.

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

Желаю всем безбажного кода!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Top 10 bugs in C++ open source projects, checked in 2016