Geekly Articles each Day
📜 ⬆️ ⬇️

Headache from using math software

Picture 2


It so happened that in one period of time I discussed on the Internet, it would seem, different topics: free Matlab alternatives for universities and students, and searching for errors in algorithms using static code analysis. All these discussions have combined the terrible quality code of modern programs. In particular, the quality of software for mathematicians and scientists. Here the question arises of confidence in the calculations and research conducted using such programs. Let's try to think about this topic and look for errors.

Introduction


I want to start with the definition of the term "algorithm". The algorithm is a set of instructions describing the procedure for the performer to achieve a certain result ( Wikipedia ). Thus, it is not necessary to divide the source code into algorithms and the rest of the code. For example, sorting algorithms are exactly the same code as opening a file, searching for a character in a string, etc. The code may contain an error and, fortunately, many errors can be identified at the initial stage using the static code analysis tools.

However, for searching for the so-called “algorithmic” errors, I decided to analyze the code of several mathematical packages. In such code there are many functions in which any mathematical formulas are simply programmed. It turns out that there are people who do not even consider such a code. And, accordingly, there may be any errors.

To identify all the code defects presented in the article, we used the PVS-Studio static analyzer version 6.15, which runs on Windows / Linux with the C / C ++ / C # programming languages.
')

Errors from 3rd party


This story began with a search for errors in the Point Cloud Library project (PCL, GitHub ). Without setting goals to find a lot of mistakes and write an article, I simply looked through the report and found a very interesting error:

V533 It is likely that a variable is being incremented inside the 'for' operator. Consider reviewing 'i'. sparsematrix.inl 212
template<class T> SparseMatrix<T>& SparseMatrix<T>::operator *= (const T& V) { for( int i=0 ; i<rows ; i++ ) for( int ii=0 ; ii<rowSizes[i] ; i++ ) m_ppElements[i][ii].Value *= V; return *this; }

In the overloaded operator "* =", they multiplied all the elements of the matrix by a certain value V. The author of the code made a very serious mistake for this algorithm, because of which only the first column of the matrix changes, and an infinite loop is possible with going beyond the array boundaries.

This code was from the mathematical library of Poisson Surface Reconstruction . I made sure that the error is still present in the latest version of the code. It is terrible to think how many more projects include this library.

Here is another strange code snippet:

V607 Ownerless expression 'j <remains'. allocator.h 120
 void rollBack(const AllocatorState& state){ .... if(state.index<index){ .... for(int j=0;j<remains;j++){ memory[index][j].~T(); new(&memory[index][j]) T(); } index=state.index; remains=state.remains; } else{ for(int j=0;j<state.remains;j<remains){ // <= memory[index][j].~T(); new(&memory[index][j]) T(); } remains=state.remains; } .... }

I suspect that this strange loop is not often executed, since it still remains in the code. But someone probably had incomprehensible hangs with the emergency termination of the program. Some idea of ​​the quality of the code has been formed. We now turn to the larger project - Scilab, where a real headache awaits us.

Scilab


about the project


Scilab is a package of applied mathematical programs that provides an open environment for engineering (technical) and scientific calculations. This development environment is one of the publicly available alternatives to Matlab , which is widely used in various institutions and research. GNU Octave is another popular alternative to Matlab, and we have previously paid attention to these projects:
Before writing a new article about Scilab, I read the old one and made only two conclusions:
  1. For 3 years, we have not corrected just a couple of places (“Why correct unspecified behavior, if everything works like this?” - apparently, the developers thought);
  2. There are many new bugs in the project. I decided to put in the article only a couple of tens, so as not to tire the reader too much.

The source file for Scilab immediately contains a project file for Visual Studio, so you could open it and test the project in one click, which I did.

Beautiful typos


V530 sci_mscanf.cpp 274
 types::Function::ReturnValue sci_mscanf(....) { .... std::vector<types::InternalType*> pITTemp = std::vector<...>(); .... case types::InternalType::ScilabString : { .... pITTemp.pop_back(); // <= pITTemp.push_back(pType); } break; case types::InternalType::ScilabDouble : { .... pITTemp.back(); // <= ??? pITTemp.push_back(pType); } break; .... }

It seems that code completion played a cruel joke with the programmer. In the code of the sci_mscanf function, the last element of the vector is always removed before adding a new one, but in one place the programmer made a mistake by calling the back () function instead of pop_back () . Calling the back () function in this way makes no sense.

V595 The 'Block.inptr' pointer was used before it was counted against nullptr. Check lines: 478, 479. sci_model2blk.cpp 478
 types::Function::ReturnValue sci_model2blk(....) { .... Block.inptr[i] = MALLOC(size); if (Block.inptr == nullptr) { freeBlock(&Block); Scierror(888, _("%s : Allocation error.\n"), name.data()); return types::Function::Error; } memset(Block.inptr[i], 0x00, size); .... }

A very interesting case of a typo, because of which the allocation control stopped working. Most likely, the correct code should be like this:
 Block.inptr[i] = MALLOC(size); if (Block.inptr[i] == nullptr) { .... }

V595 The 'pwstLines' pointer was used before it was verified against nullptr. Check lines: 78, 79. mgetl.cpp 78
 int mgetl(int iFileID, int iLineCount, wchar_t ***pwstLines) { *pwstLines = NULL; .... *pwstLines = (wchar_t**)MALLOC(iLineCount * sizeof(wchar_t*)); if (pwstLines == NULL) { return -1; } .... }

Surprisingly similar error. The author of the code did not count the asterisks, therefore the wrong pointer is checked in the condition.

V595 The 'array_size' pointer was used before it was verified against nullptr. Check lines: 67, 68. diary_manager.cpp 67
 wchar_t **getDiaryFilenames(int *array_size) { *array_size = 0; if (SCIDIARY) { std::list<std::wstring> wstringFilenames = SCIDIARY->get.... *array_size = (int)wstringFilenames.size(); if (array_size > 0) { .... } .... }

Stability is a sign of excellence. The programmer again forgot to dereference the pointer, which is why not the size of some array is compared with zero, but the pointer to this variable.

V501 There are identical sub-expressions 'strncmp (tx, "% pi", 3) == 0' to the left and to the right of the '||' operator. stringtocomplex.c 276
 static int ParseNumber(const char* tx) { .... else if (strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0 || strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0 || strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0 || strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0 || strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0 )) { return 4; } else if (strlen(tx) >= 3 && (strncmp(tx, "+%e", 3) == 0 || strncmp(tx, "-%e", 3) == 0 || strncmp(tx, "%pi", 3) == 0 // <= || strncmp(tx, "Nan", 3) == 0 || strncmp(tx, "Inf", 3) == 0 || strncmp(tx, "%pi", 3) == 0)) // <= { return 3; } .... }

This function contains some code for parsing numbers. The analyzer found a suspicious comparison with two identical "% pi" lines. Looking at the neighboring code snippet, we can assume that instead of the duplicated string there could be the string "-% pi" or "-Inf". It is also possible that this is just an extra copied line of code, but then it is better to delete it.

Priorities for operations


V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '==' operator. sci_sparse.cpp 49
 types::Function::ReturnValue sci_sparse(....) { bool isValid = true; .... for (int i = 0 ; isValid && i < in.size() ; i++) { switch (in[i]->getType()) { case types::InternalType::ScilabBool : case types::InternalType::ScilabSparseBool : { isValid = (i == (in.size() > 1) ? 1 : 0); } .... }

Errors with priorities of operations are very common in modern code. (see the article " Logical expressions in C / C ++. How professionals are mistaken ").

In the above code snippet, there is also an error, but due to a lot of luck, the code with the error works as the programmer expected. Just because the elements of the array with indices 0 and 1 participate in the comparison, and the integer representations of truth and false are also the values ​​0 and 1, this piece of code miraculously still works correctly.

You should rewrite the code to the correct priority of operations:
 isValid = (i == (in.size() > 1 ? 1 : 0));

V590 Consider inspecting the 'iType! = - 1 && iType == 8' expression. The expression is misprint. scilabview.cpp 175
 void ScilabView::createObject(int iUID) { int iType = -1; int *piType = &iType; getGraphicObjectProperty(....); if (iType != -1 && iType == __GO_FIGURE__) { m_figureList[iUID] = -1; setCurrentFigure(iUID); } .... }

In this fragment there is an error with the priority of operations, which is also discussed in the article proposed earlier.

The conditional subexpression (iType! = -1) does not affect the result of the entire conditional expression. You can verify the error by building the truth table for this example.

Another such example:

Invalid error messages


In the previous article on errors in Scilab, there was also a small section on errors when printing messages. On a fresh code, there were also quite a lot of errors of this type.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 159, 163. cdfbase.c 159
 void cdf_error(char const* const fname, int status, double bound) { switch (status) { .... case 10: if (strcmp(fname, "cdfchi") == 0) // <= { Scierror(999 _("%s: cumgam returned an error\n"), fname); } else if (strcmp(fname, "cdfchi") == 0) // <= { Scierror(999, _("%s: gamma or inverse gamma routine failed\n"), fname); } break; .... }

Scilab has a large set of cdf functions. The presented code snippet interprets the return codes of these functions. And the trouble is that some kind of error warning is never displayed due to a typo in the function name. A search for this post leads to the cdfgam function. I would like to sympathize with users who worked with this function and could not find out about some of the problems due to a typo of the authors of the mathematical package.

V510 The 'Scierror' function is not expected to receive class-type variable as the third actual argument. sci_winqueryreg.cpp 149
 const std::string fname = "winqueryreg"; types::Function::ReturnValue sci_winqueryreg(....) { .... if (rhs != 2 && rhs != 3) { Scierror(77, _("%s: Wrong number...\n"), fname.data(), 2, 3); return types::Function::Error; } .... else { Scierror(999, _("%s: Cannot open Windows regist..."), fname); return types::Function::Error; } .... }

When printing a line in one place, they forgot to call the data () method.

V746 Type slicing. By reference rather than by value. sci_scinotes.cpp 48
 int sci_scinotes(char * fname, void* pvApiCtx) { .... try { callSciNotesW(NULL, 0); } catch (GiwsException::JniCallMethodException exception) { Scierror(999, "%s: %s\n", fname, exception.getJavaDescription().c_str()); } catch (GiwsException::JniException exception) { Scierror(999, "%s: %s\n", fname, exception.whatStr().c_str()); } .... }

Exception caught by value. This means that a new object will be constructed using the copy constructor and some of the exception information will be lost. The correct option is to intercept exceptions by reference.

There are several such places:

Strange code


Strange code, because it is unclear why so to write and how to fix it better.

V523 The 'then' statement is equivalent to the 'else' statement. data3d.cpp 51
 void Data3D::getDataProperty(int property, void **_pvData) { if (property == UNKNOWN_DATA_PROPERTY) { *_pvData = NULL; } else { *_pvData = NULL; } }

Here is a simple function that always resets the pointer.

V575 The 'memset' function processes '0' elements. Inspect the third argument. win_mem_alloc.c 91
 void *MyHeapAlloc(size_t dwSize, char *file, int line) { LPVOID NewPointer = NULL; if (dwSize > 0) { _try { NewPointer = malloc(dwSize); NewPointer = memset (NewPointer, 0, dwSize); } _except (EXCEPTION_EXECUTE_HANDLER) { } .... } else { _try { NewPointer = malloc(dwSize); NewPointer = memset (NewPointer, 0, dwSize); } _except (EXCEPTION_EXECUTE_HANDLER) { } } return NewPointer; }

Regardless of the value of the dwSize variable , the same code is always executed. So why duplicate it?

V695 Range intersections are possible within conditional expressions. Example: if (A <5) {...} else if (A <2) {...}. Check lines: 438, 442. sci_sorder.c 442
 int sci_sorder(char *fname, void* pvApiCtx) { .... if (iRows * iCols > 0) { dblTol1 = pdblTol[0]; } else if (iRows * iCols > 1) { dblTol2 = pdblTol[1]; } .... }

The second condition is always false, because if EXPR> 0 , then it does not make sense to check EXPR> 1 . There is clearly some kind of error in this code.

Null pointer dereferencing and undefined behavior


V522 Dereferencing of the null pointer 'dataz' might take place. polylinedata_wrap.c 373
 BOOL translatePolyline(int uid, double x, double y, double z, int flagX, int flagY, int flagZ) { double *datax = NULL; double *datay = NULL; double *dataz = NULL; // <= int i = 0; if (x != 0.0) { datax = getDataX(uid); if (datax == NULL) return FALSE; .... if (z != 0 && isZCoordSet(uid)) { if (flagZ) { for (i = 0; i < getDataSize_(uid); ++i) { dataz[i] = pow(10.,log10(dataz[i]) + z); // <= } } else { for (i = 0; i < getDataSize_(uid); ++i) { dataz[i] += z; // <= } } } return TRUE; }

There are datax , datay and dataz arrays . The latter is not initialized anywhere, but is used under certain conditions.

V595 The 'number' pointer was used against nullptr. Check lines: 410, 425. scilab_sscanf.cpp 410
 int scilab_sscanf(....) { .... wchar_t* number = NULL; .... number = (wchar_t*)MALLOC((nbrOfDigit + 1) * sizeof(wchar_t)); memcpy(number, wcsData, nbrOfDigit * sizeof(wchar_t)); number[nbrOfDigit] = L'\0'; iSingleData = wcstoul(number, &number, base); if ((iSingleData == 0) && (number[0] == wcsData[0])) { .... } if (number == NULL) { wcsData += nbrOfDigit; } else { wcsData += (nbrOfDigit - wcslen(number)); } .... }

Under the number line, they allocated memory using the malloc () function, while de-checking it several times before checking the pointer and passing it to the memcpy () function as an argument, which is unacceptable.

V595 The OuputStrings' pointer was used before it was verified against nullptr. Check lines: 271, 272. spawncommand.c 271
 char **CreateOuput(pipeinfo *pipe, BOOL DetachProcess) { char **OuputStrings = NULL; .... OuputStrings = (char**)MALLOC((pipe->NumberOfLines) * ....); memset(OuputStrings, 0x00,sizeof(char*) * pipe->NumberOfLines); if (OuputStrings) { char *line = strtok(buffer, LF_STR); int i = 0; while (line) { OuputStrings[i] = convertLine(line, DetachProcess); .... }

Here, they allocate dynamic memory for the OuputStrings variable, but prior to checking this pointer, the allocated memory is reset with the help of the memset () function, and you cannot do this. Quote from the documentation for the function: " The behavior is undefined if 'dest' is a null pointer ".

Memory leaks and unclosed resources


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

V611 The memory was allocated using the 'new T []' operator. Consider inspecting this code. It's probably better to use 'delete [] piOut;'. sci_grand.cpp 991
 types::Function::ReturnValue sci_grand(....) { .... int* piP = new int[vectpDblInput[0]->getSize()]; int* piOut = new int[pDblOut->getSize()]; .... delete piP; delete piOut; .... }

There were two serious mistakes. After allocating dynamic memory for arrays, it should be cleared by calling the delete [] operator, i.e. with square brackets.

V773 The function of the doc 'pointer. A memory leak is possible. sci_builddoc.cpp 263
 int sci_buildDoc(char *fname, void* pvApiCtx) { .... try { org_scilab_modules_helptools::SciDocMain * doc = new .... if (doc->setOutputDirectory((char *)outputDirectory.c_str())) { .... } else { Scierror(999, _("...."), fname, outputDirectory.c_str()); return FALSE; // <= } if (doc != NULL) { delete doc; } } catch (GiwsException::JniException ex) { Scierror(....); Scierror(....); Scierror(....); return FALSE; } .... }

In some situations, exiting the function without clearing the doc pointer is performed. Also, the comparison of the doc pointer with NULL is not correct, because if the new operator fails to allocate memory, then it throws an exception, and does not return NULL .

This is the most obvious example of a memory leak found in the Scilab project. It is evident that they plan to free up the memory, but in one place they forgot to do it.

In general, a lot of memory leaks have been found in the project: pointers are simply not cleaned and are not saved anywhere. Since I am not a Scilab developer, it is difficult for me to determine where errors are in such cases and where they are not. But I tend to assume that memory leaks very much. Surely my words can confirm users of this math package.

V773 Visibility Scope A resource leak is possible. killscilabprocess.c 35
 void killScilabProcess(int exitCode) { HANDLE hProcess; /* Ouverture de ce Process avec droit pour le tuer */ hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, ....); if (hProcess) { /* Tue ce Process */ TerminateProcess(hProcess, exitCode); } else { MessageBox(NULL, "....", "Warning", MB_ICONWARNING); } }

Resource leak. According to the documentation, after calling the OpenProcess function, you need to call the CloseHandle function.

Conclusion


At the moment, on the official website of Scilab, the stable version is considered to be Scilab 6.0.0, but as we have noticed, stability is far from here. Although the latest version from the repository was checked with the analyzer, errors usually live in the code for a very long time, getting into the supposedly “stable” versions. I myself was also a Scilab user, but long before I could see how many errors there were. I hope that such software does not greatly inhibit the research of people using similar tools for mathematical calculations.

The next proven project, in which there is a lot of mathematics, and it is in demand in various studies, will be the OpenCV library .

Note colleagues Andrei Karpov. The topic of this article strongly intersects with the thoughts that I have stated in the articles:
Perhaps readers will be interested to get acquainted with them.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Headache from using mathematical software

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/331638/

More articles:


All Articles