Анализ кода ROOT — фреймворка для анализа данных научных исследований |
||
МЕНЮ Искусственный интеллект Поиск Регистрация на сайте Помощь проекту ТЕМЫ Новости ИИ Искусственный интеллект Разработка ИИГолосовой помощник Городские сумасшедшие ИИ в медицине ИИ проекты Искусственные нейросети Слежка за людьми Угроза ИИ ИИ теория Внедрение ИИКомпьютерные науки Машинное обуч. (Ошибки) Машинное обучение Машинный перевод Реализация ИИ Реализация нейросетей Создание беспилотных авто Трезво про ИИ Философия ИИ Big data Работа разума и сознаниеМодель мозгаРобототехника, БПЛАТрансгуманизмОбработка текстаТеория эволюцииДополненная реальностьЖелезоКиберугрозыНаучный мирИТ индустрияРазработка ПОТеория информацииМатематикаЦифровая экономика
Генетические алгоритмы Капсульные нейросети Основы нейронных сетей Распознавание лиц Распознавание образов Распознавание речи Техническое зрение Чат-боты Авторизация |
2019-10-23 02:12 Пока в Стокгольме проходила 118-я Нобелевская неделя, в офисе разработки статического анализатора кода PVS-Studio готовился обзор кода проекта ROOT, используемого в научных исследованиях для обработки больших данных. Премию за такой код, конечно, не дашь, а вот подробный обзор интересных дефектов кода и лицензию для полной проверки проекта разработчики получат. Введение ROOT — набор утилит для работы с данными научных исследований. Он обеспечивает все функциональные возможности, необходимые для обработки больших данных, статистического анализа, визуализации и хранения. В основном написан на языке C++. Разработка началась в CERN (Европейская организация по ядерным исследованиям) для исследований по физике высоких энергий. Каждый день тысячи физиков используют ROOT-приложения для анализа своих данных или для моделирования. PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS и может анализировать код, предназначенный для 32-битных, 64-битных и встраиваемых ARM платформ.Дебют новой диагностики V1046 Unsafe usage of the bool' and 'int' types together in the operation '&='. GSLMultiRootFinder.h 175
Бета-версия анализатора, которая использовалась при проверке, нашла вот такую потрясающую ошибку. Ожидание. Функция SetFunctionList обходит список итераторов. Если хоть один из них будет невалидным, то возвращаемое значение будет false, иначе true. Реальность. Функция SetFunctionList может возвращать значение false даже для валидных итераторов. Разберёмся в ситуации.Функция AddFunction возвращает количество валидных итераторов в списке fFunctions. Т.е. при добавлении ненулевых итераторов, размер этого списка будет последовательно увеличиваться: 1, 2, 3, 4 и т.д. Вот тут и начнёт проявлять себя ошибка в коде:
Т.к. функция возвращает результат типа int, а не bool, то операция '&=' с чётными числами будет давать значение false. Ведь младший бит чётных чисел всегда будет равен нулю. Следовательно, такая неочевидная ошибка будет портить результат функции SetFunctionsList даже для валидных аргументов. Ошибки в условных выражениях V501 There are identical sub-expressions to the left and to the right of the '&&' operator: module && module rootcling_impl.cxx 3650
Начнём с самого безобидного примера. Указатель module проверяется два раза. Скорее всего, одна проверка лишняя. Но код лучше исправить, чтобы не возникало лишних вопросов. V501 There are identical sub-expressions 'strchr(fHostAuth->GetHost(), '*')' to the left and to the right of the '||' operator. TAuthenticate.cxx 300
Тут в строке fHostAuth->GetHost() ищется один и тот же символ — '*'. Возможно, одним из них должен быть символ '?'. Обычно их используют для задания разных масок. V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 163, 165. TProofMonSenderML.cxx 163
Переменная fSummaryVrs дважды сравнивается с нулём. Это приводит к тому, что код в ветви else-if никогда не выполняется. А кода там написано немало… V523 The 'then' statement is equivalent to the 'else' statement. TKDTree.cxx 805
Один и тот же copy-paste-код выполняется при любом условии. Возможно, есть опечатка в словах left и right. Подобного подозрительного кода в проекте немало:
V547 Expression '!file_name_value.empty()' is always false. SelectionRules.cxx 1423
Скорее всего, тут нет ошибки. Анализатор обнаружил код, который можно сократить. Т.к. значение file_name_value.empty() проверяется в начале цикла, то ниже по коду эту проверку можно убрать, заметно сократив количество ненужного кода. V590 Consider inspecting the '!file1 || c <= 0 || c == '*' || c != '('' expression. The expression is excessive or contains a misprint. TTabCom.cxx 840
Рассмотрим сокращённую часть условного выражения, на которое указал анализатор:
Условие не зависит от того, равен символ «звёздочке» или нет. Эта часть условного выражения всегда будет истинна для любого символа, отличного от '('. В этом легко убедиться, если построить таблицу истинности. Ещё два места со странной логикой в условных выражениях:
V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. TProofServ.cxx 1903
Ошибка, которую обнаружил анализатор, проявляет себя только при некорректной работе программы. Переменная ret должна хранить код возврата функции AddWorkers и в случае нештатной ситуации выводить это значение в лог. Но код работает не так. В условии не хватает дополнительных скобок, задающих приоритет операций. В переменную ret сохраняется не код возврата, а результат логического сравнения, т.е. только 0 или 1. Есть ещё одно место с похожим дефектом:
V768 The enumeration constant 'kCostComplexityPruning' is used as a variable of a Boolean-type. MethodDT.cxx 283
Хм… Зачем делать отрицание константного значения kCostComplexityPruning? Скорее всего, символ отрицания случайно добавился и теперь приводит к неправильной логике выполнения кода. Некорректный код с указателями V522 Dereferencing of the null pointer 'pre' might take place. TSynapse.cxx 61
Я попытался разобраться в этом странном коде. Вроде задумка в том, чтобы не выставлять новое значение полю fpre. Тогда тут допустили ошибку, перепутав указатель, который следует проверить в условии. В текущей реализации, если передать значение nullptr в функцию SetPre, то произойдёт разыменование нулевого указателя. Скорее всего, правильно так:
Правда, это всё равно не спасёт функцию от передачи нулевого указателя. Но, по крайней мере, такой код выглядит более логичным, чем первоначальный вариант. Вот ещё одно место, которое скопировано отсюда с небольшими изменениями:
V595 The 'N' pointer was utilized before it was verified against nullptr. Check lines: 484, 488. Scanner.cxx 484
Анализатор обнаружил очень опасный код! Указатель N в первом случае разыменовывается без проверки на нулевое значение. Причём обращение к непроверенному указателю и не разглядишь. Это происходит внутри функции shouldVisitDecl. Традиционно, это диагностическое правильно выдаёт много полезных предупреждений. Вот некоторые из них:
Следующий пример не является ошибкой, но в очередной раз демонстрирует, что макросы поощряют написание неправильного или избыточного кода. V571 Recurring check. The 'if (fCanvasImp)' condition was already verified in line 799. TCanvas.cxx 800
Указатель fCanvasImp проверяется дважды. Одна из проверок уже реализована в макросе SafeDelete. Одна из проблем с макросами в том, что к ним часто затруднена навигация из кода, поэтому многие не изучают их содержимое перед использованием. Ошибки при работе с массивами V519 The 'Line[Cursor]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 352, 353. Editor.cpp 353
Новое значение элемента Line[Cursor] сразу же перезаписывается. Что-то здесь не так… V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 130
Так ошибаться в проверке индекса массива — просто массовая проблема в последнее время. Чуть ли не в каждом третьем проекте встречается. Если при индексации массива в цикле всё просто — почти всегда используется оператор '<' для сравнения индекса с размером массива, то при такой проверке, как здесь, надо использовать оператор '>=', а не '>'. Иначе возможен выход за границу массива на один элемент. Эту ошибку расплодили по файлу несколько раз:
V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. TDataMember.cxx 554
Скорее всего, в цикле for хотели сравнивать переменную dim с dm->fArrayDim, а не fArrayDim. Значение используемой переменной — отрицательное, благодаря условию в начале функции. Такой цикл никогда не выполняется. V767 Suspicious access to element of 'current' array by a constant index inside a loop. TClingUtils.cxx 3082
Этот фрагмент кода парсит некую строку и проверяет её корректность. После того, как нулевой символ строки current определяется как число, выполняется обход всех остальных символов, чтобы убедиться, что до конца строки все символы — числа. Ну как выполняется… счётчик цикла i не используется в цикле. Надо было написать current[i], а не current[0] в условии. Утечка памяти V773 The function was exited without releasing the 'optionlist' pointer. A memory leak is possible. TDataMember.cxx 355
Во время выхода из функции не предусмотрено освобождение памяти по указателю optionList. Нужно ли это в данном конкретном случае — мне сложно сказать. Но обычно такие ошибки исправляют в проектах по отчётам PVS-Studio. Всё зависит от того, должна ли программа пытаться продолжить работать в нештатной ситуации или нет. Таких предупреждений в проекте много, разработчикам лучше перепроверить проект самостоятельно и посмотреть полный отчёт анализатора. Снова про функцию memset V597 The compiler could delete the 'memset' function call, which is used to flush 'x' buffer. The memset_s() function should be used to erase the private data. TMD5.cxx 366
Многие подумают, что когда код скомпилируется, в бинарный файл не попадёт этот комментарий, и будут правы :D. А вот что функция memset тоже будет удалена компилятором, догадываются не все. А это произойдёт. Если очищаемый буфер больше не будет использоваться в коде, то компилятор оптимизирует код и удалит вызов функции. Технически он прав, но если в буфере были секретные данные, то они там и останутся. Это классический дефект безопасности CWE-14. Разные предупреждения V591 Non-void function should return a value. LogLikelihoodFCN.h 108
В перегруженном операторе отсутствует возвращаемое значение. Тоже очень распространённая проблема в последнее время. V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); RTensor.hxx 363
Ошибка заключается в том, что случайно забыто ключевое слово throw. В результате данный код не генерирует исключение в случае ошибочной ситуации. Всего нашлось два таких места. Второе:
V609 Divide by zero. Denominator range [0..100]. TGHtmlImage.cxx 340
Случай, похожий на те, что были описаны ранее про массивы. Здесь переменная n ограничивается диапазоном значений от 0 до 100. В таком случае существует ветвь кода, в которой произойдет деление на переменную n со значением 0. Скорее всего, ограничение значения n следует переписать таким образом:
V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. TProofServ.cxx 729
Анализатор обнаружил странное форматирование. В одном месте отсутствует ключевое слово else. По коду можно предположить, что код действительно стоит исправить. И ещё пару мест исправить заодно:
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. MethodKNN.cxx 602
При работе с классом std::istream недостаточно вызова функции eof() для завершения цикла. В случае возникновения сбоя при чтении данных, вызов функции eof() будет всегда возвращать значение false, а других точек выхода из цикла в этом коде нет. Для завершения цикла в этом случае необходима дополнительная проверка значения, возвращаемого функцией fail():
Или просто написать:
V678 An object is used as an argument to its own method. Consider checking the first actual argument of the 'Copy' function. TFormLeafInfo.cxx 2414
Напоследок вот такая опечаточка. Вместо fSizes надо было передать orig.fSizes в функцию Copy. Заключение Примерно год назад делался обзор кода проекта NCBI Genome Workbench. Этот проект тоже используется в научных исследованиях, но генома. К чему я веду, программное обеспечение в этой сфере очень важно, но его качеству не уделяют должного внимания. Кстати, на днях вышла macOS 10.15 Catalina, в которой отказались от поддержки 32-х битных приложений. И в PVS-Studio есть большой набор правил для выявления проблем при портировании программ на 64-х битные системы. Подробнее об этом в посте блога анализатора. Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing the Code of ROOT, Scientific Data Analysis Framework. Источник: habr.com Комментарии: |
|