Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
I sometimes feel quite embarrassed when examining bugs in software projects. Many of these bugs inhabit the code for many years, and you just can't help wondering how the program still manages to run at all with a hundred mistakes and defects. And it does work somehow. And people do manage to use it. It holds true not only for code drawing a video game pockemon, but for math libraries too. Your guess is right - we'll speak about the math library Scilab and its analysis results in this article.
Today we are going to discuss suspicious code fragments in the Scilab mathematical package. Analysis was performed with the PVS-Studio tool.
Scilab is an open source, cross-platform numerical computational package and a high-level, numerically oriented programming language. It can be used for signal processing, statistical analysis, image enhancement, fluid dynamics simulations, numerical optimization, and modeling, simulation of explicit and implicit dynamical systems and (if the corresponding toolbox is installed) symbolic manipulations. Scilab is the most complete open source alternative to MATLAB. [Wikipedia].
Official website: http://www.scilab.org/
The system provides a large number of functionalities:
Get ready: the article is going to be quite a lengthy one. It's not me to blame for so much filth in this code, and I'm just eager to show you an as wide range of various defects as possible.
The bugs I have found have nothing to do with mathematics, of course. Maybe all the algorithms in the library are correct and efficient. But since the developers chose to write their program in C++, they should have kept in mind various possible issues like typos, null pointer dereferencing and other slip-ups besides mistakes in algorithms. After all, it doesn't make any difference to a user if he faces a logic error in a numerical algorithm or it is an uninitialized variable that will cause him much trouble.
Sure, static analysis can only find certain types of errors. But since they are easy to detect, why shouldn't we do that? It's better to fix 10% more of the bugs than not fix anything at all.
So let's see what the PVS-Studio has to tell us about bugs in the Scilab project.
int sci_champ_G(....)
{
....
char * strf = NULL ;
....
if ( isDefStrf( strf ) )
{
char strfl[4];
strcpy(strfl,DEFSTRFN);
strf = strfl;
if ( !isDefRect( rect ) )
{
strf[1]='5';
}
}
(*func)(stk(l1), stk(l2), stk(l3), stk(l4),
&m3, &n3, strf, rect, arfact, 4L);
....
}PVS-Studio's diagnostic message: V507 Pointer to local array 'strfl' is stored outside the scope of this array. Such a pointer will become invalid. sci_champ.c 103
A reference to a temporary array 'strfl' is saved into the 'strf' variable. When leaving the "if () { ... }" block, this array ceases to exist. However, the programmer goes on to work with the 'strf' pointer.
This leads to undefined behavior. One can't work with an already destroyed array. The program may go on running correctly, of course, but only from pure luck. The memory area that used to store that array may be at any moment occupied by other arrays or variables.
Fragments with similar defects:
int C2F(pmatj)
(char *fname, int *lw, int *j, unsigned long fname_len)
{
....
ix1 = il2 + 4;
m2 = Max(m, 1);
ix1 = il + 9 + m * n;
....
}PVS-Studio's diagnostic message: V519 The 'ix1' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2387, 2389. stack1.c 2389
Something is wrong with the 'ix1' variable. I guess there is a typo somewhere in this code.
That's an interesting piece of code: the programmer needed to get some values and then check them. But it all turned just vice versa.
int sci_Playsound (char *fname,unsigned long fname_len)
{
....
int m1 = 0, n1 = 0;
....
if ( (m1 != n1) && (n1 != 1) )
{
Scierror(999,_("%s: Wrong size for input argument #%d: ")
_("A string expected.\n"),fname,1);
return 0;
}
sciErr = getMatrixOfWideString(pvApiCtx, piAddressVarOne,
&m1,&n1,&lenStVarOne, NULL);
....
}PVS-Studio's diagnostic messages: V560 A part of conditional expression is always false: (m1 != n1). sci_playsound.c 66; V560 A part of conditional expression is always true: (n1 != 1). sci_playsound.c 66
The variables m1 and n1 are to take values when calling the getMatrixOfWideString() function and be checked after that. However, the check appears to be executed prior to the function call.
The check finds the variables m1 and n1 equal to 0, so the "if ( (m1 != n1) && (n1 != 1) )" condition will never be true. As a result, this check will in no way affect program execution.
To sum it up - the check of the variables m1 and n1 fails.
void CreCommon(f,var)
FILE *f;
VARPTR var;
{
....
if ( strncmp(var->fexternal, "cintf", 4)==0 )
....
}PVS-Studio's diagnostic message: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. crerhs.c 119
The magic number 4 is used here, and this number is incorrect. There are 5 characters, not 4, in the "cintf" string. Don't use such magic numbers.
I would have implemented a special macro for this code to calculate the length of string literals and use it in the following way:
if ( strncmp(var->fexternal, "cintf", litlen("cintf"))==0 )We won't discuss now how to implement the 'litlen' macro - there are numbers of ways to do that to fit everyone's taste. The main idea is to get rid of the magic number.
Other fragments with incorrect string lengths:
int C2F(run)(void)
{
....
static int *Lpt = C2F(iop).lpt - 1;
....
Lpt[1] = Lin[1 + k];
Lpt[2] = Lin[2 + k];
Lpt[3] = Lin[3 + k];
Lpt[4] = Lin[4 + k];
Lct[4] = Lin[6 + k ];
Lpt[6] = k;
....
}PVS-Studio's diagnostic message: V525 The code containing the collection of similar blocks. Check items '1', '2', '3', '4', '4' in lines 1005, 1006, 1007, 1008, 1009. run.c 1005
There is a typo in a numerical sequence. It results in one array item remaining uninitialized. That can bring you quite a lot of interesting mathematical results.
int write_xml_states(
int nvar, const char * xmlfile, char **ids, double *x)
{
....
FILE *fd = NULL;
....
wcfopen(fd, (char*)xmlfile, "wb");
if (fd < 0)
{
sciprint(_("Error: cannot write to '%s' \n"), xmlfile);
....
}PVS-Studio's diagnostic message: V503 This is a nonsensical comparison: pointer < 0. scicos.c 5826
I'm almost sure that the open function was used in older times in this code to open a file. It must have been replaced with the _wfopen function later - its call is hidden inside the 'wcfopen' macro.
However, the programmer forgot to fix the check of correct file opening. The open() function returns value -1 in case of an error, and checking a pointer for being less than zero doesn't make any sense.
Here's one more fragment with the same background.
void taucs_ccs_genmmd(taucs_ccs_matrix* m,
int** perm, int** invperm)
{
int n, maxint, delta, nofsub;
....
maxint = 32000;
assert(sizeof(int) == 4);
maxint = 2147483647; /* 2**31-1, for 32-bit only! */
....
}PVS-Studio's diagnostic message: V519 The 'maxint' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 154, 157. taucs_scilab.c 157
There's no error here, but the code is pretty funny.
You can notice the line "maxint = 32000;" written long ago. Then a new piece of code was added below it:
assert(sizeof(int) == 4);
maxint = 2147483647; /* 2**31-1, for 32-bit only! */
char *getCommonPart(char **dictionary, int sizeDictionary)
{
....
char *currentstr = dictionary[0];
qsort(dictionary, sizeof dictionary / sizeof dictionary[0],
sizeof dictionary[0], cmp);
....
}PVS-Studio's diagnostic message: V514 Dividing sizeof a pointer 'sizeof dictionary' by another value. There is a probability of logical error presence. getcommonpart.c 76
The second argument of the qsort() function is the number of items in an array. Because of a mistake, this number always makes one.
Have a look at the "sizeof dictionary / sizeof dictionary[0]" expression: the pointer size is divided by the pointer size. This evaluates to one.
I guess the correct code should have looked like this:
qsort(dictionary, sizeDictionary, sizeof dictionary[0], cmp);A similar mistake was found in the following fragment: getfilesdictionary.c 105
void GetenvB(char *name, char *env, int len)
{
int ierr = 0, one = 1;
C2F(getenvc)(&ierr,name,env,&len,&one);
if (ierr == 0)
{
char *last = &env[len-1];
while ( *last == ' ' ) { last = '\0' ; }
last--;
}
....
}V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *last = '\0'. getenvb.c 24
This code is just awful. Or beautiful - if we speak of errors from the viewpoint of how interesting they are.
while ( *last == ' ' ) { last = '\0' ; }If the first character in the string is a space, the pointer will set to zero, and we'll deal with accessing the elements on a null pointer after that.
I suspect that this code was meant to replace all the spaces with '\0'. If so, it should look like this:
while ( *last == ' ' ) { *last++ = '\0' ; }It's a funny thing, but there is one more fragment in the code where spaces are meant to be replaced with zeros - and it is done incorrectly too.
static int msg_101(int *n, int *ierr)
{
....
for (i=0;i<(int)strlen(line);i++)
{
if (line[i]==' ') line[i]='\0';
break;
}
....
}PVS-Studio's diagnostic message: V612 An unconditional 'break' within a loop. msgs.c 1293
It all would be alright but for the 'break' operator. Only one space will be replaced. However, removing 'break' won't help things: the strlen() function will return zero, and the loop will terminate all the same.
Other "one-time" loops:
char **splitLineCSV(....)
{
....
if (retstr[curr_str] == NULL)
{
*toks = 0;
FREE(substitutedstring);
substitutedstring = NULL;
freeArrayOfString(retstr, strlen(substitutedstring));
return NULL;
}
....
}PVS-Studio's diagnostic message: V575 The null pointer is passed into 'strlen' function. Inspect the first argument. splitline.c 107
It's a strange code. The programmer first directly zeroes the 'substitutedstring' pointer and then mercilessly throws it a victim to the strlen() function.
I suspect that the call of the freeArrayOfString() function should have been written before the call of the FREE() function.
That was a warm-up. Now let's investigate a more complex case.
inline static void create(void * pvApiCtx, const int position,
const int rows, const int cols, long long * ptr)
{
int * dataPtr = 0;
alloc(pvApiCtx, position, rows, cols, dataPtr);
for (int i = 0; i < rows * cols; i++)
{
dataPtr[i] = static_cast<int>(ptr[i]);
}
}V522 Dereferencing of the null pointer 'dataPtr' might take place. scilababstractmemoryallocator.hxx 222
The programmer wanted to allocate memory in this function through alloc(). It may seem at first that the function returns a value by reference, the last argument represented by the 'dataPtr' pointer which is to store the pointer to the allocated memory buffer.
But that's wrong. The pointer will remain null. Have a look at the declaration of the alloc() function:
inline static int *alloc(
void * pvApiCtx, const int position, const int rows,
const int cols, int * ptr)As you can see, the last argument is not a reference. By the way, it's not quite clear why it was written here at all. Let's peek inside the alloc() function:
inline static int *alloc(
void * pvApiCtx, const int position, const int rows,
const int cols, int * ptr)
{
int * _ptr = 0;
SciErr err = allocMatrixOfInteger32(
pvApiCtx, position, rows, cols, &_ptr);
checkError(err);
return _ptr;
}The last argument 'ptr' is not used at all.
Anyway, the code for memory allocation is written incorrectly. It should look as follows:
inline static void create(void * pvApiCtx, const int position,
const int rows, const int cols, long long * ptr)
{
int *dataPtr = alloc(pvApiCtx, position, rows, cols, 0);
for (int i = 0; i < rows * cols; i++)
{
dataPtr[i] = static_cast<int>(ptr[i]);
}
}Other similar issues:
PVS-Studio reveals quite a lot of typos in error handlers. These code branches are executed rarely, so errors inside them may stay unnoticed for a long time. I suspect it is because of these errors that we often fail to understand what is wrong with the program: an error message it generates has nothing to do with the real state of things.
Here's an example of incorrect error message preparation:
static SciErr fillCommonSparseMatrixInList(....)
{
....
addErrorMessage(&sciErr, API_ERROR_FILL_SPARSE_IN_LIST,
_("%s: Unable to create list item #%d in Scilab memory"),
_iComplex ? "createComplexSparseMatrixInList" :
"createComplexSparseMatrixInList",
_iItemPos + 1);
....
}PVS-Studio's diagnostic message: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: "createComplexSparseMatrixInList". api_list.cpp 2398
Regardless of the value of the '_iComplex' variable, the "createComplexSparseMatrixInList" message will be printed all the time.
Other similar issues:
Now let's discuss an error handler that will never get control:
#define __GO_FIGURE__ 9
#define __GO_UIMENU__ 21
int sci_uimenu(char *fname, unsigned long fname_len)
{
....
if (iParentType == __GO_FIGURE__ &&
iParentType == __GO_UIMENU__)
{
Scierror(999, _("%s: Wrong type for input argument #%d: ")
_("A '%s' or '%s' handle expected.\n"),
fname, 1, "Figure", "Uimenu");
return FALSE;
}
....
}PVS-Studio's diagnostic message: V547 Expression 'iParentType == 9 && iParentType == 21' is always false. Probably the '||' operator should be used here. sci_uimenu.c 99
The (iParentType == __GO_FIGURE__ && iParentType == __GO_UIMENU__) condition will never be true. The variable cannot equal 9 and 21 at the same time. I think the programmer meant it to be written as follows:
if (iParentType != __GO_FIGURE__ &&
iParentType != __GO_UIMENU__)One more example, especially tasty.
int set_view_property(....)
{
BOOL status = FALSE;
....
status = setGraphicObjectProperty(
pobjUID, __GO_VIEW__, &viewType, jni_int, 1);
if (status = TRUE)
{
return SET_PROPERTY_SUCCEED;
}
else
{
Scierror(999, _("'%s' property does not exist ")
_("for this handle.\n"), "view");
return SET_PROPERTY_ERROR ;
}
....
}PVS-Studio's diagnostic message: V559 Suspicious assignment inside the condition expression of 'if' operator: status = 1. set_view_property.c 61
The error is in this line: "if (status = TRUE)". Assignment is done instead of comparison.
This function can obviously be shortened. It must have been written through the Copy-Paste method, the programmer forgetting to change something in the copied text.
static int uf_union (int* uf, int s, int t) {
if (uf_find(uf,s) < uf_find(uf,t))
{
uf[uf_find(uf,s)] = uf_find(uf,t);
return (uf_find(uf,t));
}
else
{
uf[uf_find(uf,s)] = uf_find(uf,t);
return (uf_find(uf,t));
}
}PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. taucs_scilab.c 700
Regardless of the condition, one and the same algorithm is executed.
Here is another situation where we are dealing with coinciding conditions:
int sci_xset( char *fname, unsigned long fname_len )
{
....
else if ( strcmp(cstk(l1), "mark size") == 0)
....
else if ( strcmp(cstk(l1), "mark") == 0)
....
else if ( strcmp(cstk(l1), "mark") == 0)
....
else if ( strcmp(cstk(l1), "colormap") == 0)
....
}PVS-Studio's diagnostic message: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 175, 398. sci_xset.c 175
A few more incorrect conditions:
I'm sure now I have figured out which mistake C/C++ programmers make most often - dereference a pointer first and only then check it for being null. It doesn't always cause an error, but there is no excuse for such ugly code.
static void appendData(....)
{
....
sco_data *sco = (sco_data *) * (block->work);
int maxNumberOfPoints = sco->internal.maxNumberOfPoints;
int numberOfPoints = sco->internal.numberOfPoints;
if (sco != NULL && numberOfPoints >= maxNumberOfPoints)
....
}PVS-Studio's diagnostic message: V595 The 'sco' pointer was utilized before it was verified against nullptr. Check lines: 305, 311. canimxy3d.c 305
At first, the programmer addressed the members through the 'sco' pointer:
int maxNumberOfPoints = sco->internal.maxNumberOfPoints;
int numberOfPoints = sco->internal.numberOfPoints;Then it occurred to him that the pointer should have been checked:
if (sco != NULL .....The analyzer generated 61 more V595 warnings. I don't find it reasonable to enumerate them all in the article, so here you are a separate list: scilab-v595.txt.
One more widely spread mistake is to use incorrect format specifiers when working with the sprint() function and the like. There hardly were any interesting examples among all the issues of this kind - it's just unsigned values printed as signed ones. That's why I've made another list: scilab-v576.txt.
The only interesting sample I could single out is the following one:
#define FORMAT_SESSION "%s%s%s"
char *getCommentDateSession(BOOL longFormat)
{
....
sprintf(line, FORMAT_SESSION, SESSION_PRAGMA_BEGIN,
STRING_BEGIN_SESSION, time_str, SESSION_PRAGMA_END);
....
}PVS-Studio's diagnostic message: V576 Incorrect format. A different number of actual arguments is expected while calling 'sprintf' function. Expected: 5. Present: 6. getcommentdatesession.c 68
The SESSION_PRAGMA_END string will fail to be printed.
short ezxml_internal_dtd(ezxml_root_t root, char *s, size_t len)
{
....
while (*(n = ++s + strspn(s, EZXML_WS)) && *n != '>') {
....
}PVS-Studio's diagnostic message: V567 Undefined behavior. The 's' variable is modified while being used twice between sequence points. ezxml.c 385
It cannot be said for sure which of the two expressions "++s' or "strspn(s, EZXML_WS)" will be calculated first. Therefore, you may get different results with different compilers, platforms, etc.
Here's another, more interesting, sample. In this code, a typo leads to undefined behavior.
static char **replaceStrings(....)
{
....
int i = 0;
....
for (i = 0; i < nr; i = i++)
....
}V567 Undefined behavior. The 'i' variable is modified while being used twice between sequence points. csvread.c 620
The trouble is in this piece: i = i++.
The code was most likely meant to be written like this:
for (i = 0; i < nr; i++)char *PLD_strtok(....)
{
....
if ((st->start)&&(st->start != '\0'))
....
}PVS-Studio's diagnostic message: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *st->start != '\0'. pldstr.c 303
The programmer wanted to check the string for being nonempty, but actually he compared the pointer to NULL twice. The fixed code should look as follows:
if ((st->start)&&(st->start[0] != '\0'))Another mistake of this kind:
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: ** category == '\0'. sci_xcospalload.cpp 57
The code fragment below seems to be incomplete:
int sci_displaytree(char *fname, unsigned long fname_len)
{
....
string szCurLevel = "";
....
//Add node level
if (szCurLevel != "")
{
szCurLevel + ".";
}
....
}PVS-Studio's warning: V655 The strings was concatenated but are not utilized. Consider inspecting the 'szCurLevel + "."' expression. sci_displaytree.cpp 80
static int sci_toprint_two_rhs(void* _pvCtx,
const char *fname)
{
....
sprintf(lines, "%s%s\n", lines, pStVarOne[i]);
....
}PVS-Studio's warning: V541 It is dangerous to print the string 'lines' into itself. sci_toprint.cpp 314
The sprintf() function saves its return result into the 'lines' buffer. At the same time, this very buffer is also one of the input strings. It's no good doing such things. The code might work, but it is very risky. If you move to another compiler, you may get an unexpected and very unpleasant result.
Another defect of that kind: sci_coserror.c 94
Here's an example of incorrect code that executes well:
typedef struct JavaVMOption {
char *optionString;
void *extraInfo;
} JavaVMOption;
JavaVMOption *options;
BOOL startJVM(char *SCI_PATH)
{
....
fprintf(stderr, "%d: %s\n", j, vm_args.options[j]);
....
}PVS-Studio's diagnostic message: V510 The 'fprintf' function is not expected to receive class-type variable as fourth actual argument. jvm.c 247
The programmer wanted to print the string the 'optionString' pointer refers to. The correct code should look like this:
fprintf(stderr, "%d: %s\n", j, vm_args.options[j].optionString);However, the fprintf() function will actually take an object of the JavaVMOption type as an argument. The code works only thanks to wonderful and lucky coincidence.
Firstly, the 'optionString' member is located in the beginning of the structure. That's why it is this particular member that the fprintf() function will take and handle as a pointer to the string.
Secondly, the function will not print anything after that, therefore no garbage will be printed too (i.e. the contents of the 'extraInfo' variable that will also get into the stack).
Hallelujah!
static void reinitdoit(double *told)
{
int keve = 0, kiwa = 0;
....
kiwa = 0;
....
for (i = 0; i < kiwa; i++)
....
}V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. scicos.c 4432
Something is wrong here. The 'kiwa' variable is always equal to zero. The loop doesn't iterate. Perhaps this code is incomplete.
To be honest, I'm too tired to scan the report and write this article. So I'd better stop right here. I could mention a few more suspicious fragments, but I didn't find them too much significant and gave in to laziness. Besides, I'm sure I must have missed something, for I am not familiar with the project. In this connection, I recommend that the project authors check their code with the PVS-Studio analyzer on their own.
Note. I want to remind those who believe they can use static code analyzer for a one-time check without buying it that it won't make any sense. What static analysis is all about is regular, not one-time, checks. When you make a typo, the analyzer catches it at once, thus reducing the time spent on testing, debugging and fixing bugs from the bug tracker. To find out more about it, see the article "Leo Tolstoy and static code analysis".
Somebody is definitely going to ask which version of Scilab it was that I was checking. Unfortunately, it was not the freshest one. I checked this project and noted down the suspicious code fragments about a month and a half ago... and totally forgot about it as we were very busy with a comparison of analyzers for that time. And just recently I have stumbled upon that file and it has taken me quite a while to recall what it has to do with. You see, I have to check so many projects that they are all mixed up in my head and I can't even remember if I have already checked this or that project.
Well, it's alright though. I will now finish with this article, and the Scilabe authors will notice it and check their project themselves. The main goal of my articles is to show the capabilities of the static analysis methodology, not to find every single bug in the latest project versions.
Be sure to use static analysis regularly - only then will it help you save time on fixing silly mistakes to spend it on something more useful instead.
Medium and large projects that can't do without night checks, additional tool customization, integration with MSBuild, support of Visual Studio 2005/2008, and so on and so forth, are welcome to try the PVS-Studio analyzer.
0
0
0
0