Топ 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(&inter);
if (ve) {
result = ve->qi();
return 0;
}
FEdge *fe = dynamic_cast(&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 ss(new int[decks * 52]);
....
} По умолчанию умный указатель типа shared_ptr для уничтожения объекта вызовет оператор delete без квадратных скобок []. В данном случае, это неправильно.
Корректный вариант кода должен быть таким:
std::shared_ptr ss(new int[decks * 52],
std::default_delete()); Третье место
Источник: Проверка исходного кода игрового движка 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
