Пока в Стокгольме проходила 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
int AddFunction(const ROOT::Math::IMultiGenFunction & func) {
ROOT::Math::IMultiGenFunction * f = func.Clone();
if (!f) return 0;
fFunctions.push_back(f);
return fFunctions.size();
}
template<class FuncIterator>
bool SetFunctionList( FuncIterator begin, FuncIterator end) {
bool ret = true;
for (FuncIterator itr = begin; itr != end; ++itr) {
const ROOT::Math::IMultiGenFunction * f = *itr;
ret &= AddFunction(*f);
}
return ret;
}Бета-версия анализатора, которая использовалась при проверке, нашла вот такую потрясающую ошибку.
Ожидание. Функция SetFunctionList обходит список итераторов. Если хоть один из них будет невалидным, то возвращаемое значение будет false, иначе true.
Реальность. Функция SetFunctionList может возвращать значение false даже для валидных итераторов. Разберёмся в ситуации. Функция AddFunction возвращает количество валидных итераторов в списке fFunctions. Т.е. при добавлении ненулевых итераторов, размер этого списка будет последовательно увеличиваться: 1, 2, 3, 4 и т.д. Вот тут и начнёт проявлять себя ошибка в коде:
ret &= AddFunction(*f);Т.к. функция возвращает результат типа 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
virtual void HandleDiagnostic(....) override
{
....
bool isROOTSystemModuleDiag = module && ....;
bool isSystemModuleDiag = module && module && module->IsSystem;
if (!isROOTSystemModuleDiag && !isSystemModuleDiag)
fChild->HandleDiagnostic(DiagLevel, Info);
....
}Начнём с самого безобидного примера. Указатель module проверяется два раза. Скорее всего, одна проверка лишняя. Но код лучше исправить, чтобы не возникало лишних вопросов.
V501 There are identical sub-expressions 'strchr(fHostAuth->GetHost(), '*')' to the left and to the right of the '||' operator. TAuthenticate.cxx 300
TAuthenticate::TAuthenticate(TSocket *sock, const char *remote,
const char *proto, const char *user)
{
....
// If generic THostAuth (i.e. with wild card or user == any)
// make a personalized memory copy of this THostAuth
if (strchr(fHostAuth->GetHost(),'*') || strchr(fHostAuth->GetHost(),'*') ||
fHostAuth->GetServer() == -1 ) {
fHostAuth = new THostAuth(*fHostAuth);
fHostAuth->SetHost(fqdn);
fHostAuth->SetUser(checkUser);
fHostAuth->SetServer(servtype);
}
....
}Тут в строке 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
Int_t TProofMonSenderML::SendSummary(TList *recs, const char *id)
{
....
if (fSummaryVrs == 0) {
if ((dsn = recs->FindObject("dataset"))) recs->Remove(dsn);
} else if (fSummaryVrs == 0) {
// Only the first records
xrecs = new TList;
xrecs->SetOwner(kFALSE);
TIter nxr(recs);
TObject *o = 0;
while ((o = nxr())) {
if (!strcmp(o->GetName(), "vmemmxw")) break;
xrecs->Add(o);
}
}
....
}Переменная fSummaryVrs дважды сравнивается с нулём. Это приводит к тому, что код в ветви else-if никогда не выполняется. А кода там написано немало...
V523 The 'then' statement is equivalent to the 'else' statement. TKDTree.cxx 805
template <typename Index, typename Value>
void TKDTree<Index, Value>::UpdateRange(....)
{
....
if (point[fAxis[inode]]<=fValue[inode]){
//first examine the node that contains the point
UpdateRange(GetLeft(inode),point, range, res);
UpdateRange(GetRight(inode),point, range, res);
} else {
UpdateRange(GetLeft(inode),point, range, res);
UpdateRange(GetRight(inode),point, range, res);
}
....
}Один и тот же copy-paste-код выполняется при любом условии. Возможно, есть опечатка в словах left и right.
Подобного подозрительного кода в проекте немало:
V547 Expression '!file_name_value.empty()' is always false. SelectionRules.cxx 1423
bool SelectionRules::AreAllSelectionRulesUsed() const {
for(auto&& rule : fClassSelectionRules){
....
std::string file_name_value;
if (!rule.GetAttributeValue("file_name", file_name_value))
file_name_value.clear();
if (!file_name_value.empty()) { // <=
// don't complain about defined_in rules
continue;
}
const char* attrName = nullptr;
const char* attrVal = nullptr;
if (!file_name_value.empty()) { // <=
attrName = "file name";
attrVal = file_name_value.c_str();
} else {
attrName = "class";
if (!name.empty()) attrVal = name.c_str();
}
ROOT::TMetaUtils::Warning(0,"Unused %s rule: %s\n", attrName, attrVal);
}
....
}Скорее всего, тут нет ошибки. Анализатор обнаружил код, который можно сократить. Т.к. значение 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
TString TTabCom::DetermineClass(const char varName[])
{
....
c = file1.get();
if (!file1 || c <= 0 || c == '*' || c != '(') {
Error("TTabCom::DetermineClass", "variable \"%s\" not defined?",
varName);
goto cleanup;
}
....
}Рассмотрим сокращённую часть условного выражения, на которое указал анализатор:
if (.... || c == '*' || c != '(') {
....
}Условие не зависит от того, равен символ "звёздочке" или нет. Эта часть условного выражения всегда будет истинна для любого символа, отличного от '('. В этом легко убедиться, если построить таблицу истинности.
Ещё два места со странной логикой в условных выражениях:
V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. TProofServ.cxx 1903
Int_t TProofServ::HandleSocketInput(TMessage *mess, Bool_t all)
{
....
if (Int_t ret = fProof->AddWorkers(workerList) < 0) {
Error("HandleSocketInput:kPROOF_GETSLAVEINFO",
"adding a list of worker nodes returned: %d", ret);
}
....
}Ошибка, которую обнаружил анализатор, проявляет себя только при некорректной работе программы. Переменная ret должна хранить код возврата функции AddWorkers и в случае нештатной ситуации выводить это значение в лог. Но код работает не так. В условии не хватает дополнительных скобок, задающих приоритет операций. В переменную ret сохраняется не код возврата, а результат логического сравнения, т.е. только 0 или 1.
Есть ещё одно место с похожим дефектом:
V768 The enumeration constant 'kCostComplexityPruning' is used as a variable of a Boolean-type. MethodDT.cxx 283
enum EPruneMethod {kExpectedErrorPruning=0, kCostComplexityPruning, kNoPruning};
void TMVA::MethodDT::ProcessOptions()
{
....
if (fPruneStrength < 0) fAutomatic = kTRUE;
else fAutomatic = kFALSE;
if (fAutomatic && fPruneMethod==!DecisionTree::kCostComplexityPruning){
Log() << kFATAL
<< "Sorry automatic pruning strength determination is ...." << Endl;
}
....
}Хм... Зачем делать отрицание константного значения kCostComplexityPruning? Скорее всего, символ отрицания случайно добавился и теперь приводит к неправильной логике выполнения кода.
V522 Dereferencing of the null pointer 'pre' might take place. TSynapse.cxx 61
void TSynapse::SetPre(TNeuron * pre)
{
if (pre) {
Error("SetPre","this synapse is already assigned to a pre-neuron.");
return;
}
fpre = pre;
pre->AddPost(this);
}Я попытался разобраться в этом странном коде. Вроде задумка в том, чтобы не выставлять новое значение полю fpre. Тогда тут допустили ошибку, перепутав указатель, который следует проверить в условии. В текущей реализации, если передать значение nullptr в функцию SetPre, то произойдёт разыменование нулевого указателя.
Скорее всего, правильно так:
void TSynapse::SetPre(TNeuron * pre)
{
if (fpre) {
Error("SetPre","this synapse is already assigned to a pre-neuron.");
return;
}
fpre = pre;
pre->AddPost(this);
}Правда, это всё равно не спасёт функцию от передачи нулевого указателя. Но, по крайней мере, такой код выглядит более логичным, чем первоначальный вариант.
Вот ещё одно место, которое скопировано отсюда с небольшими изменениями:
V595 The 'N' pointer was utilized before it was verified against nullptr. Check lines: 484, 488. Scanner.cxx 484
bool RScanner::shouldVisitDecl(clang::NamedDecl *D)
{
if (auto M = D->getOwningModule()) { // <= 2
return fInterpreter.getSema().isModuleVisible(M);
}
return true;
}
bool RScanner::VisitNamespaceDecl(clang::NamespaceDecl* N)
{
if (fScanType == EScanType::kOnePCM)
return true;
if (!shouldVisitDecl(N)) // <= 1
return true;
if((N && N->isImplicit()) || !N){ // <= 3
return true;
}
....
}Анализатор обнаружил очень опасный код! Указатель N в первом случае разыменовывается без проверки на нулевое значение. Причём обращение к непроверенному указателю и не разглядишь. Это происходит внутри функции shouldVisitDecl.
Традиционно, это диагностическое правило выдаёт много полезных предупреждений. Вот некоторые из них:
Следующий пример не является ошибкой, но в очередной раз демонстрирует, что макросы поощряют написание неправильного или избыточного кода.
V571 Recurring check. The 'if (fCanvasImp)' condition was already verified in line 799. TCanvas.cxx 800
#define SafeDelete(p) { if (p) { delete p; p = 0; } }
void TCanvas::Close(Option_t *option)
{
....
if (fCanvasImp)
SafeDelete(fCanvasImp);
....
}Указатель fCanvasImp проверяется дважды. Одна из проверок уже реализована в макросе SafeDelete. Одна из проблем с макросами в том, что к ним часто затруднена навигация из кода, поэтому многие не изучают их содержимое перед использованием.
V519 The 'Line[Cursor]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 352, 353. Editor.cpp 353
size_t find_last_non_alnum(const std::string &str,
std::string::size_type index = std::string::npos) {
....
char tmp = Line.GetText()[Cursor];
Line[Cursor] = Line[Cursor - 1];
Line[Cursor] = tmp;
....
}Новое значение элемента Line[Cursor] сразу же перезаписывается. Что-то здесь не так...
V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 130
bool BasicMinimizer::SetVariableValue(unsigned int ivar, double val) {
if (ivar > fValues.size() ) return false;
fValues[ivar] = val;
return true;
}Так ошибаться в проверке индекса массива - просто массовая проблема в последнее время. Чуть ли не в каждом третьем проекте встречается. Если при индексации массива в цикле всё просто - почти всегда используется оператор '<' для сравнения индекса с размером массива, то при такой проверке, как здесь, надо использовать оператор '>=', а не '>'. Иначе возможен выход за границу массива на один элемент.
Эту ошибку расплодили по файлу несколько раз:
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
Int_t TDataMember::GetArrayDim() const
{
if (fArrayDim<0 && fInfo) {
R__LOCKGUARD(gInterpreterMutex);
TDataMember *dm = const_cast<TDataMember*>(this);
dm->fArrayDim = gCling->DataMemberInfo_ArrayDim(fInfo);
// fArrayMaxIndex should be zero
if (dm->fArrayDim) {
dm->fArrayMaxIndex = new Int_t[fArrayDim];
for(Int_t dim = 0; dim < fArrayDim; ++dim) {
dm->fArrayMaxIndex[dim] = gCling->DataMemberInfo_MaxIndex(fInfo,dim);
}
}
}
return fArrayDim;
}Скорее всего, в цикле for хотели сравнивать переменную dim с dm->fArrayDim, а не fArrayDim. Значение используемой переменной - отрицательное, благодаря условию в начале функции. Такой цикл никогда не выполняется.
V767 Suspicious access to element of 'current' array by a constant index inside a loop. TClingUtils.cxx 3082
llvm::StringRef ROOT::TMetaUtils::DataMemberInfo__ValidArrayIndex(....)
{
....
while (current!=0) {
// Check the token
if (isdigit(current[0])) {
for(i=0;i<strlen(current);i++) {
if (!isdigit(current[0])) {
if (errstr) *errstr = current;
if (errnum) *errnum = NOT_INT;
return llvm::StringRef();
}
}
} else { // current token is not a digit
....
}
....
}
....
}Этот фрагмент кода парсит некую строку и проверяет её корректность. После того, как нулевой символ строки current определяется как число, выполняется обход всех остальных символов, чтобы убедиться, что до конца строки все символы - числа. Ну как выполняется... счётчик цикла i не используется в цикле. Надо было написать current[i], а не current[0] в условии.
V773 The function was exited without releasing the 'optionlist' pointer. A memory leak is possible. TDataMember.cxx 355
void TDataMember::Init(bool afterReading)
{
....
TList *optionlist = new TList(); //storage for options strings
for (i=0;i<token_cnt;i++) {
if (strstr(tokens[i],"Items")) {
ptr1 = R__STRTOK_R(tokens[i], "()", &rest);
if (ptr1 == 0) {
Fatal("TDataMember","Internal error, found \"Items....",GetTitle());
return;
}
ptr1 = R__STRTOK_R(nullptr, "()", &rest);
if (ptr1 == 0) {
Fatal("TDataMember","Internal error, found \"Items....",GetTitle());
return;
}
....
}
....
}
....
// dispose of temporary option list...
delete optionlist;
....
}Во время выхода из функции не предусмотрено освобождение памяти по указателю optionList. Нужно ли это в данном конкретном случае - мне сложно сказать. Но обычно такие ошибки исправляют в проектах по отчётам PVS-Studio. Всё зависит от того, должна ли программа пытаться продолжить работать в нештатной ситуации или нет. Таких предупреждений в проекте много, разработчикам лучше перепроверить проект самостоятельно и посмотреть полный отчёт анализатора.
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
void TMD5::Transform(UInt_t buf[4], const UChar_t in[64])
{
UInt_t a, b, c, d, x[16];
....
// Zero out sensitive information
memset(x, 0, sizeof(x));
}Многие подумают, что когда код скомпилируется, в бинарный файл не попадёт этот комментарий, и будут правы :D. А вот что функция memset тоже будет удалена компилятором, догадываются не все. А это произойдёт. Если очищаемый буфер больше не будет использоваться в коде, то компилятор оптимизирует код и удалит вызов функции. Технически он прав, но если в буфере были секретные данные, то они там и останутся. Это классический дефект безопасности CWE-14.
V591 Non-void function should return a value. LogLikelihoodFCN.h 108
LogLikelihoodFCN & operator = (const LogLikelihoodFCN & rhs) {
SetData(rhs.DataPtr() );
SetModelFunction(rhs.ModelFunctionPtr() );
fNEffPoints = rhs.fNEffPoints;
fGrad = rhs.fGrad;
fIsExtended = rhs.fIsExtended;
fWeight = rhs.fWeight;
fExecutionPolicy = rhs.fExecutionPolicy;
}В перегруженном операторе отсутствует возвращаемое значение. Тоже очень распространённая проблема в последнее время.
V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); RTensor.hxx 363
template <typename Value_t, typename Container_t>
inline RTensor<Value_t, Container_t> RTensor<Value_t, Container_t>::Transpose()
{
if (fLayout == MemoryLayout::RowMajor) {
fLayout = MemoryLayout::ColumnMajor;
} else if (fLayout == MemoryLayout::ColumnMajor) {
fLayout = MemoryLayout::RowMajor;
} else {
std::runtime_error("Memory layout is not known.");
}
....
}Ошибка заключается в том, что случайно забыто ключевое слово throw. В результате данный код не генерирует исключение в случае ошибочной ситуации.
Всего нашлось два таких места. Второе:
V609 Divide by zero. Denominator range [0..100]. TGHtmlImage.cxx 340
const char *TGHtml::GetPctWidth(TGHtmlElement *p, char *opt, char *ret)
{
int n, m, val;
....
if (n < 0 || n > 100) return z;
if (opt[0] == 'h') {
val = fCanvas->GetHeight() * 100;
} else {
val = fCanvas->GetWidth() * 100;
}
if (!fInTd) {
snprintf(ret, 15, "%d", val / n); // <=
} else {
....
}
....
}Случай, похожий на те, что были описаны ранее про массивы. Здесь переменная n ограничивается диапазоном значений от 0 до 100. В таком случае существует ветвь кода, в которой произойдет деление на переменную n со значением 0. Скорее всего, ограничение значения n следует переписать таким образом:
if (n <= 0 || n > 100) return z;V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. TProofServ.cxx 729
TProofServ::TProofServ(Int_t *argc, char **argv, FILE *flog)
: TApplication("proofserv", argc, argv, 0, -1)
{
....
if (!logmx.IsDigit()) {
if (logmx.EndsWith("K")) {
xf = 1024;
logmx.Remove(TString::kTrailing, 'K');
} else if (logmx.EndsWith("M")) {
xf = 1024*1024;
logmx.Remove(TString::kTrailing, 'M');
} if (logmx.EndsWith("G")) {
xf = 1024*1024*1024;
logmx.Remove(TString::kTrailing, 'G');
}
}
....
}Анализатор обнаружил странное форматирование. В одном месте отсутствует ключевое слово 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
void TMVA::MethodKNN::ReadWeightsFromStream(std::istream& is)
{
....
while (!is.eof()) {
std::string line;
std::getline(is, line);
if (line.empty() || line.find("#") != std::string::npos) {
continue;
}
....
}
....
}При работе с классом std::istream недостаточно вызова функции eof() для завершения цикла. В случае возникновения сбоя при чтении данных, вызов функции eof() будет всегда возвращать значение false, а других точек выхода из цикла в этом коде нет. Для завершения цикла в этом случае необходима дополнительная проверка значения, возвращаемого функцией fail():
while (!is.eof() && !is.fail())
{
....
}Или просто написать:
while (is)
{
....
}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
TFormLeafInfoMultiVarDim::TFormLeafInfoMultiVarDim(
const TFormLeafInfoMultiVarDim& orig) : TFormLeafInfo(orig)
{
fNsize = orig.fNsize;
fSizes.Copy(fSizes); // <=
fCounter2 = orig.fCounter2?orig.fCounter2->DeepCopy():0;
fSumOfSizes = orig.fSumOfSizes;
fDim = orig.fDim;
fVirtDim = orig.fVirtDim;
fPrimaryIndex = orig.fPrimaryIndex;
fSecondaryIndex = orig.fSecondaryIndex;
}Напоследок вот такая опечаточка. Вместо fSizes надо было передать orig.fSizes в функцию Copy.
Примерно год назад делался обзор кода проекта NCBI Genome Workbench. Этот проект тоже используется в научных исследованиях, но генома. К чему я веду, программное обеспечение в этой сфере очень важно, но его качеству не уделяют должного внимания.
Кстати, на днях вышла macOS 10.15 Catalina, в которой отказались от поддержки 32-х битных приложений. И в PVS-Studio есть большой набор правил для выявления проблем при портировании программ на 64-х битные системы. Подробнее об этом в посте блога анализатора.