В этой статье я хочу рассказать об опыте проверки такого проекта как Octave. Этот проект довольно известен, особенно среди студентов, которые нуждаются в проверке своих решений математических задач и не хотят платить за Matlab.
Octave - система для математических вычислений и основная альтернатива Matlab от мира open-source. Она способна на решение самых различных задач, таких как работа с матрицами, решение дифференциальных уравнений или интегрирование функций на бесконечных интервалах. Функционал Octave широк и может быть дополнен с помощью динамически подгружаемых модулей, написанных на С, С++ или Fortran. Сам Octave написан на С++ и имеет собственный Си-подобный язык высокого уровня, синтаксис которого похож на MATLAB, что дает возможность запускать грамотно написанные скрипты не только на Octave, но и на MATLAB. Octave можно использовать в GUI режиме, который обеспечен библиотекой Qt.
Сборка системы производилась с использованием MinGW. Так как я занимался этим впервые, компилирование проекта доставило мне небольшие затруднения. Octave имеет приличное количество зависимостей, поэтому большое количество времени было потрачено на установку сторонних библиотек. По ходу сборки пришлось пересобрать и обновить некоторые уже найденные библиотеки. Например, обнаруженная на моем компьютере библиотека BLAS оказалась несовместима с настройками встроенного в MinGW компилятора gfortran.
Проверка производилась с помощью статического анализатора PVS-Studio (точнее модуля Standalone), который обладает функцией мониторинга, позволяющей перехватывать вызовы компилятора и получать препроцессированные файлы для проверки. Для запуска проверки достаточно нажать "Analyze your files" в PVS-Studio и начать сборку проекта в консоли командой make.
Большая часть предупреждений пришлась на конструкции, призванные сделать код кроссплатформенным. Возможность отсортировать предупреждения анализатора по типу ошибки или уровню её важности, а также пометить группу предупреждений, ссылающихся на кроссплатформенные решения, как ложные срабатывания, позволяют работать только с ошибками, которые предоставляют интерес. Для удобства я разделил все предупреждения в этой на три категории: предупреждения, касающиеся неправильной работы с указателями, предупреждения, относящиеся к условиям, и прочие. Предлагаю взглянуть на некоторые примеры предупреждений, найденных в проекте.
Предупреждение PVS-Studio: V507 Pointer to local array 'dirbuf' is stored outside the scope of this array. Such a pointer will become invalid. tmpdir.c 128
#define PATH_MAX 260
int path_search(const char *dir, ....)
{
....
if (....)
{
char dirbuf[PATH_MAX];
....
dir = dirbuf;
}
....
dlen = strlen (dir);
}В этом примере адрес локального буфера "dirbuf", время существования которого соответствует телу "if", сохраняется в указателе, переданном функции. После выхода из тела блока "if" указатель на уничтоженный массив используется для получения длины строки при помощи функции "strlen". Так как область памяти, на которую ссылается указатель "dir" уже недействительна, эти действия являются ошибочными.
Предупреждение PVS-Studio: V595 The 'Pinv' pointer was utilized before it was verified against nullptr. Check lines: 66, 79. colamd.cc 66
static void symetree(const octave_idx_type *ridx,
octave_idx_type *P, ....)
{
....
for (octave_idx_type k = 0 ; k < n ; k++)
Pinv[P[k]] = k;
....
octave_idx_type i = (Pinv) ? (Pinv[ridx[p]]) : (ridx[p]);
....
}В этом участке кода программист забыл проверить указатель "Pinv" перед первым использованием. Однако проверка на равенство нулю происходит в теле тернарного оператора. Так как никакие действия, которые могли бы изменить данный указатель, не предпринимались, понятно, что он так и останется ненулевым. Если бы указатель изначально был равен нулю, то мы бы получили ошибку при первом его использовании. Немного непонятен такой порядок работы с указателем.
Предупреждение PVS-Studio: V668 There is no sense in testing the 'instance' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. oct-spparms.cc 45
octave_sparse_params *octave_sparse_params::instance = 0;
bool octave_sparse_params::instance_ok(void)
{
....
instance = new octave_sparse_params();
if (instance)
....
}Этот код содержит лишнюю проверку. Эта проверка встречается после каждого использования оператора "new" в коде несколько десятков раз. Как известно, современные компиляторы согласно стандарту генерируют исключение "bad_alloc" в случае неудачного выделения памяти оператором "new". Но это не всегда было так. Например, уже устаревший компилятор VC6 в обход стандарту возвращал "NULL", вместо генерирования исключения. На данный момент эти проверки являются пережитками прошлого и более не нужны.
Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1956, 1962. cellfun.cc 1956
DEFUN(....)
{
....
octave_value array = args(0);
....
if (....)
else if (array.is_object())
retval = do_object2cell(array, dimv);
else if (array.is_map())
retval = do_num2cell(array.map_value (), dimv);
else if (array.is_cell())
retval = do_num2cell(array.cell_value (), dimv);
else if (array.is_object())
retval = do_num2cell(array.cell_value (), dimv);
....
}Здесь анализатор обнаружил повторяющееся условие в конструкции if {} else if {}. Трудно сказать, какой метод должен использоваться вместо второго вызова "array.is_object", так как класс octave_value содержит большое количество подобных методов. То, что в теле повторяющегося условия используется тот же вызов функции, что и при if (array.is_cell()) также кажется подозрительным.
Предупреждение PVS-Studio: V560 A part of conditional expression is always true: brace_level == 0. kpse.cc 490
class kpse_path_iterator
{
....
private:
size_t e;
size_t len;
void set_end(void)
{
....
int brace_level = 0;
while (e < len && !(brace_level == 0 && ...))
e++;
....
}
....
}Условие "while" в методе класса содержит лишнюю проверку brace_level == 0. Это условие выполняется всегда, поскольку переменная "brace_level" была инициализирована нулем перед циклом и не изменялась в процессе его выполнения. Возможно, когда-то тело цикла содержало операции с "brace_level". Потом эти операции убрали, а условие изменить забыли. Но это всего лишь предположение.
Предупреждение PVS-Studio: V560 A part of conditional expression is always true: !error_state. load-save.cc 403
octave_value do_load(std::istream& stream, ....)
{
....
std::string name;
....
if (error_state || stream.eof() || name.empty())
break;
else if (!error_state && !name.empty())
{
....
}
....
}В этой конструкции попадание в блок "else" возможно лишь тогда, когда каждое из условий "error_state", "stream.eof()" и "name.empty()" будет ложным. Если хотя бы одно из этих условий будет истинным, то сработает блок "if". Таким образом, попадание в блок "else" гарантирует нам то, что "error_state" и "name.empty()" будут ложны, делая вторую проверку ненужной.
Предупреждение PVS-Studio: V571 Recurring check. The 'nargin > 0' condition was already verified in line 51. __dispatch__.cc 53
DEFUN(....)
{
int nargin = args.length();
....
if (nargin > 0 && nargin < 4)
{
if (nargin > 0)
....
}
....
}Здесь мы имеем похожую ситуацию с лишней проверкой переменной "nargin". Лишние проверки не являются ошибками и слабо влияют на скорость выполнения программы, особенно если они не расположены в телах циклов, но их наличие делает код немного более громоздким и трудночитаемым.
Предупреждение PVS-Studio: V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. ls-mat-ascii.cc 75
static std::string get_mat_data_input_line(std::istream& is)
{
....
do
{
while (is.get(c))
....
}
while (!(have_data || is.eof()));
....
}В данной ситуации возможно невыполнение условия выхода из цикла. В случае неудачного считывания информации из потока "is" будет установлен флаг "is.fail()". При этом флаг "is.eof()" не будет изменен и функция продолжит работу с некорректной информацией. Правильное условие выхода должно выглядеть так:
while (!(have_data || is.eof() || is.fail()));Предупреждение PVS-Studio: V519 The 'x_normrender' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 5521, 5621. graphics.cc 5621
void axes::properties::update_camera(void)
{
....
Matrix x_normrender = xform_matrix();
....
x_normrender = x_viewport * x_projection * x_view;
....
}Довольно странно, что результат первого присваивания переменной "x_normrender" нигде не используется, а позднее заменяется произведением других параметров. В теле функции "xform_matrix()" используется конструктор для создания объекта - матрицы, и небольшой цикл по ее заполнению. Эти операции могут снизить производительность в данном участке кода. Возможно, компилятор увидит, что результат функции нигде не используется, и уберет ее вызов. Но, как говорится, на компилятор надейся, а сам не плошай.
Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. matrix_type.cc 312
DEFUN(....)
{
....
if (str_typ == "upper")
mattyp.mark_as_permuted(len, p);
else
mattyp.mark_as_permuted(len, p);
....
}Нет нужды напоминать, что при использовании copy-paste метода для написания похожих конструкций, нужно быть крайне внимательным, чтобы не допустить такую ошибку. Вряд ли блоку if-else должны соответствовать одинаковые действия. Скорее всего при срабатывании условия else должен был быть вызван метод "mark_as_unpermuted". Более того, программист скопировал часть кода, содержащую этот блок, для повторного использования в функции позднее, повторив ошибку.
В итоге вторую такую же конструкцию можно найти тут:
V523 The 'then' statement is equivalent to the 'else' statement. matrix_type.cc 485
Перейдем к следующему предупреждению.
Предупреждение PVS-Studio: V570 The 'i' variable is assigned to itself. sparse.cc 144
template <class T>
void Sparse<T>::SparseRep::maybe_compress(bool remove_zeros)
{
....
octave_idx_type i = 0;
for (octave_idx_type j = 1; j <= ncols; j++)
{
octave_idx_type u = c[j];
for (i = i; i < u; i++)
if (d[i] != T())
{
d[k] = d[i];
r[k++] = r[i];
}
c[j] = k;
}
....
}Подобную ошибку довольно трудно обнаружить, особенно бегло пробегая глазами по коду, поскольку в этих циклах используется большое количество однобуквенных переменных. Я специально не стал вырезать часть кода, чтобы вы могли увидеть, как сильно замыливают глаза такие названия. Внимательные читатели уже обнаружили странное присваивание переменной в инициализирующей части цикла for (i = i; i < u; i++). Трудно сказать, хотел ли программист присвоить "i" значение "j" или 1, так как визуально эти символы очень схожи.
Любопытно, но спустя 800 строчек кода эта конструкция повторяется уже с переименованными переменными "d" и "k" и немного другими условиями, но с той же ошибкой.
Подводя итог, хочется сказать, что код Octave показался мне качественным. Большинство подозрительных мест, обнаруживаемых анализатором, объяснялись кроссплатформенными решениями. В статье были опущены некоторые предупреждения, как использование классов без перегруженного оператора присваивания, использование глобальных переменных с короткими именами и т.п. Эти предупреждения низкого уровня не являются ошибками и носят строго рекомендательный характер. В общем хочется похвалить разработчиков, ведь для такого крупного проекта Octave содержит довольно мало ошибок, что, возможно, является следствие его солидного возраста. Тем не менее PVS-Studio удалось найти что-то интересное. Предлагаю попробовать его бесплатно на вашем проекте: http://www.viva64.com/ru/pvs-studio/download/