Головная боль от использования математического софта

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

Введение

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

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

Для выявления всех дефектов кода, представленных в статье, использовался статический анализатор PVS-Studio версии 6.15, работающий в Windows/Linux с языками программирования C/C++/C#.

Ошибки из 3rd party

Эта история началась с поиска ошибок в проекте Point Cloud Library (PCL, GitHub). Не ставив себе цели найти много ошибок и написать статью, я просто просматривал отчёт и нашёл очень интересную ошибку:

V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. sparsematrix.inl 212

В перегруженном операторе "* https://github.com/mkazhdan/PoissonRecon" target="_blank" rel="nofollow">Poisson Surface Reconstruction. Я убедился, что ошибка до сих по присутствует в последней версии кода. Страшно подумать, сколько ещё проектов включают в себя эту библиотеку.

Вот ещё один странный фрагмент кода:

V607 Ownerless expression 'j < remains'. allocator.h 120

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

Scilab

Scilab - пакет прикладных математических программ, предоставляющий открытое окружение для инженерных (технических) и научных расчётов. Эта среда разработки является одной из общедоступных альтернатив Matlab, которая широко используется в разных учреждениях и научных исследованиях. Ещё одной популярной альтернативой Matlab является GNU Octave, и ранее мы уже обращали внимание на эти проекты:

(март 2014); (август 2015).

Перед написанием новой статьи о Scilab я прочитал старую и сделал всего два вывода:

• За 3 года не исправили всего парочку мест ("Зачем исправлять неопределённое поведение, если и так всё работает?" - видимо, подумали разработчики);
• В проекте появилось много новых ошибок. Я решил поместить в статью только пару десятков, чтобы не слишком утомить читателя.

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

V530 The return value of function 'back' is required to be utilized. sci_mscanf.cpp 274

Похоже, автодополнение кода сыграло с программистом злую шутку. В коде функции sci_mscanf всегда удаляют последний элемент вектора перед добавлением нового, но в одном месте программист допустил ошибку, позвав функцию back(), вместо pop_back(). Вызывать функцию back() таким образом нет никакого смысла.

V595 The 'Block.inptr' pointer was utilized before it was verified against nullptr. Check lines: 478, 479. sci_model2blk.cpp 478

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

V595 The 'pwstLines' pointer was utilized before it was verified against nullptr. Check lines: 78, 79. mgetl.cpp 78

На удивление похожая ошибка. Автор кода не досчитался звёздочек, поэтому в условии проверяется не тот указатель.

V595 The 'array_size' pointer was utilized before it was verified against nullptr. Check lines: 67, 68. diary_manager.cpp 67

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

V501 There are identical sub-expressions 'strncmp(tx, "%pi", 3) == 0' to the left and to the right of the '||' operator. stringtocomplex.c 276

Эта функция содержит некий код для парсинга чисел. Анализатор нашёл подозрительное сравнение с двумя одинаковыми строками "%pi". Глядя на соседний фрагмент кода, можно предположить, что вместо продублированной строки могла быть строка "-%pi" или "-Inf". Также не исключено, что это просто лишняя скопированная строчка кода, но тогда её лучше удалить.

V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. sci_sparse.cpp 49

Ошибки с приоритетами операций очень распространены в современном коде. (см. статью "Логические выражения в C/C++. Как ошибаются профессионалы").

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

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

V590 Consider inspecting the 'iType != - 1 && iType == 8' expression. The expression is excessive or contains a misprint. scilabview.cpp 175

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

Условное подвыражение (iType != -1) никак не влияет на результат всего условного выражения. Убедиться в ошибке можно с помощью построения таблицы истинности для этого примера.

Ещё один такой пример:

• V590 Consider inspecting the 'iObjectType != - 1 && iObjectType == 5' expression. The expression is excessive or contains a misprint. sci_unglue.c 90

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

V517 The use of 'if (A) else if (A) ' pattern was detected. There is a probability of logical error presence. Check lines: 159, 163. cdfbase.c 159

В Scilab есть большой набор cdf функций. В представленном фрагменте кода выполняется интерпретация кодов возврата этих функций. И вот беда - некое предупреждение об ошибке никогда не выводится из-за опечатки в имени функции. Поиск по этому сообщению приводит к функции cdfgam. Хочется посочувствовать пользователям, которые работали с этой функцией и не могли узнать о некоторых проблемах из-за опечатки авторов математического пакета.

V510 The 'Scierror' function is not expected to receive class-type variable as third actual argument. sci_winqueryreg.cpp 149

При печати строки в одном месте забыли вызвать метод data().

V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 48

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

Таких мест найдено несколько:

• V746 Type slicing. An exception should be caught by reference rather than by value. sci_builddoc.cpp 270
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_closescinotesfromscilab.cpp 45
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_closescinotesfromscilab.cpp 50
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 52
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 263
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 272
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 349
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 353
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 365
• V746 Type slicing. An exception should be caught by reference rather than by value. sci_scinotes.cpp 369
• V746 Type slicing. An exception should be caught by reference rather than by value. visitor_common.cpp 1743
• V746 Type slicing. An exception should be caught by reference rather than by value. overload.cpp 135

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

V523 The 'then' statement is equivalent to the 'else' statement. data3d.cpp 51

Вот такая нехитрая функция, которая всегда обнуляет указатель.

V575 The 'memset' function processes '0' elements. Inspect the third argument. win_mem_alloc.c 91

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

V695 Range intersections are possible within conditional expressions. Example: if (A < 5) < . >else if (A < 2) < . >. Check lines: 438, 442. sci_sorder.c 442

Второе условие всегда ложно, так как если EXPR > 0, то проверять EXPR > 1 уже не имеет смысла. В этом коде явно присутствует какая-то ошибка.

V522 Dereferencing of the null pointer 'dataz' might take place. polylinedata_wrap.c 373

Есть массивы datax, datay и dataz. Последний нигде не инициализируется, но при определённых условиях используется.

V595 The 'number' pointer was utilized before it was verified against nullptr. Check lines: 410, 425. scilab_sscanf.cpp 410

Под строку number выделили память с помощью функции malloc(), при этом до проверки указателя его несколько раз разыменовывают и передают в функцию memcpy() в качестве аргумента, что недопустимо.

V595 The 'OuputStrings' pointer was utilized before it was verified against nullptr. Check lines: 271, 272. spawncommand.c 271

Здесь выделяют динамическую память для переменной OuputStrings, но до проверки этого указателя, выделенную память обнуляют с помощью функции memset(), а так делать нельзя. Цитата из документации к функции: "The behavior is undefined if 'dest' is a null pointer".

V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] piP;'. sci_grand.cpp 990

V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] piOut;'. sci_grand.cpp 991

Тут были допущены две серьезные ошибки. После выделения динамической памяти под массивы, очищать ее следует с помощью вызова оператора delete[], т.е. с квадратными скобками.

V773 The function was exited without releasing the 'doc' pointer. A memory leak is possible. sci_builddoc.cpp 263

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

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

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

V773 Visibility scope of the 'hProcess' handle was exited without releasing the resource. A resource leak is possible. killscilabprocess.c 35

Утечка ресурса. Согласно документации, после вызова функции OpenProcess необходимо звать функцию CloseHandle.

Заключение

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

Следующим проверенным проектом, в котором много математики, и он востребован в разных исследованиях, будет OpenCV library.

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