In the wake of calculators: SpeedCrunch

Research code calculators continues! This review will review the project SpeedCrunch - the second most popular among free calculators.

Introduction

SpeedCrunch is a high-precision scientific calculator with a fast user interface, controlled from the keyboard. This is free open source software available on Windows, Linux and macOS.

Source code is hosted on BitBucket . I didn’t like the assembly documentation, which, in my opinion, would be worth writing in more detail. The requirements specify “Qt 5.2 or later”, although it took several specific packages that were difficult to learn from the CMake log. By the way, now it is considered good practice to apply the Dockerfile to the project to quickly set up the desired developer environment.

For comparison with other calculators, I quote the output of the Cloc utility:
')

Reviews of errors in other projects:

• Calculate bugs in the calculator Windows
• In the wake of calculators: Qalculate!

PVS-Studio was used as a static analysis tool. This is a set of solutions for code quality control, searching for errors and potential vulnerabilities. Supported languages ​​include: C, C ++, C # and Java. Analyzer launch is possible on Windows, Linux and macOS.

Strange logic in the loop

V560 A part of the conditional expression is always true:! RuleFound. evaluator.cpp 1410

void Evaluator::compile(const Tokens& tokens) { .... while (!syntaxStack.hasError()) { bool ruleFound = false; // <= // Rule for function last argument: id (arg) -> arg. if (!ruleFound && syntaxStack.itemCount() >= 4) { // <= Token par2 = syntaxStack.top(); Token arg = syntaxStack.top(1); Token par1 = syntaxStack.top(2); Token id = syntaxStack.top(3); if (par2.asOperator() == Token::AssociationEnd && arg.isOperand() && par1.asOperator() == Token::AssociationStart && id.isIdentifier()) { ruleFound = true; // <= syntaxStack.reduce(4, MAX_PRECEDENCE); m_codes.append(Opcode(Opcode::Function, argCount)); #ifdef EVALUATOR_DEBUG dbg << "\tRule for function last argument " << argCount << " \n"; #endif argCount = argStack.empty() ? 0 : argStack.pop(); } } .... } .... } 

Notice the ruleFound variable. At each iteration, it is set to false. But if you look at the body of the whole cycle, then under certain conditions this variable is set to true, but it will not be taken into account in the new iteration of the cycle. Most likely, the variable ruleFound needed to be declared before the loop.

Suspicious comparisons

V560 A part of the conditional expression is always true: m_scrollDirection! = 0. resultdisplay.cpp 242

 void ResultDisplay::fullContentScrollEvent() { QScrollBar* bar = verticalScrollBar(); int value = bar->value(); bool shouldStop = (m_scrollDirection == -1 && value <= 0) || (m_scrollDirection == 1 && value >= bar->maximum()); if (shouldStop && m_scrollDirection != 0) { // <= stopActiveScrollingAnimation(); return; } scrollLines(m_scrollDirection * 10); } 

If the variable shouldStop is true , then the m_scrollDirection variable will have one of two values: -1 or 1. Therefore, in the following conditional operator, the value of m_scrollDirection will not be exactly zero, which is what the analyzer warns about.

V668 allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated The exception will be generated in the case of memory allocation error. editor.cpp 998

 void EditorCompletion::showCompletion(const QStringList& choices) { .... for (int i = 0; i < choices.count(); ++i) { QStringList pair = choices.at(i).split(':'); QTreeWidgetItem* item = new QTreeWidgetItem(m_popup, pair); if (item && m_editor->layoutDirection() == Qt::RightToLeft) item->setTextAlignment(0, Qt::AlignRight); .... } .... } 

Memory for an object of type QTreeWidgetItem is allocated using the new operator. This means that if it is impossible to allocate dynamic memory, the exception std :: bad_alloc () will be thrown. Therefore, checking the item pointer is redundant and can be deleted.

Potential NULL Dereference

V595 The 'ioparams' pointer was used before it was verified against nullptr. Check lines: 969, 983. floatio.c 969

 int cattokens(....) { .... if (printexp) { if (expbase < 2) expbase = ioparams->expbase; // <= .... } dot = '.'; expbegin = "("; expend = ")"; if (ioparams != NULL) // <= { dot = ioparams->dot; expbegin = ioparams->expbegin; expend = ioparams->expend; } .... } 

The ioparams pointer is dereferenced before being checked for validity. Most likely, a potential error crept into the code. Since dereferencing is under several conditions, the problem may manifest itself rarely, but neatly.

Division by zero

V609 Divide by zero. Denominator range [0..4]. floatconvert.c 266

 static int lgbase( signed char base) { switch(base) { case 2: return 1; case 8: return 3; case 16: return 4; } return 0; // <= } static void _setlongintdesc( p_ext_seq_desc n, t_longint* l, signed char base) { int lg; n->seq.base = base; lg = lgbase(base); // <= n->seq.digits = (_bitlength(l) + lg - 1) / lg; // <= n->seq.leadingSignDigits = 0; n->seq.trailing0 = _lastnonzerobit(l) / lg; // <= n->seq.param = l; n->getdigit = _getlongintdigit; } 

The lgbase function allows the return of a zero value, which is then divided. Potentially, anything other than 2, 8, and 16 can be passed to the function.

Undefined behavior

V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~ 0)' is negative. floatlogic.c 64

 static char _signextend( t_longint* longint) { unsigned mask; signed char sign; sign = _signof(longint); mask = (~0) << SIGNBIT; // <= if (sign < 0) longint->value[MAXIDX] |= mask; else longint->value[MAXIDX] &= ~mask; return sign; } 

The result of the zero inversion is placed in the sign type int , so the result will be a negative number, for which then a shift is performed. Shifting a negative number to the left is an undefined behavior.

The entire list of dangerous places:

• V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 1)' is negative. floatnum.c 289
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 1)' is negative. floatnum.c 325
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 1)' is negative. floatnum.c 344
• V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 1)' is negative. floatnum.c 351

HTML unclosed tags

V735 Possibly an incorrect HTML. The "</ body>" tag was expected, while the "</ div>" tag was expected. book.cpp 127

 static QString makeAlgebraLogBaseConversionPage() { return BEGIN INDEX_LINK TITLE(Book::tr("Logarithmic Base Conversion")) FORMULA(y = log(x) / log(a), log<sub>a</sub>x = log(x) / log(a)) END; } 

As is often the case with C / C ++ code - nothing is clear from the source, so let's turn to the preprocessed code for this fragment:

The analyzer detected an unclosed div tag. There are many fragments of html-code in this file and now it should be additionally checked by the developers.

Here are some more suspicious places that were found using PVS-Studio:

• V735 Possibly an incorrect HTML. The tag was expected. book.cpp 344
• V735 Possibly an incorrect HTML. The tag was expected. book.cpp 347

Assignment operator

V794 The assignment operator should not be protected from the case of this == & other. quantity.cpp 373

 Quantity& Quantity::operator=(const Quantity& other) { m_numericValue = other.m_numericValue; m_dimension = other.m_dimension; m_format = other.m_format; stripUnits(); if(other.hasUnit()) { m_unit = new CNumber(*other.m_unit); m_unitName = other.m_unitName; } cleanDimension(); return *this; } 

It is recommended to consider the situation when the object is assigned to itself by comparing pointers.

In other words, the following two lines of code should be added to the beginning of the function body:

 if (this == &other) return *this; 

Reminder

V601 The ' fake ' value is an implicitly cast to the integer type. cmath.cpp 318

 /** * Returns -1, 0, 1 if n1 is less than, equal to, or more than n2. * Only valid for real numbers, since complex ones are not an ordered field. */ int CNumber::compare(const CNumber& other) const { if (isReal() && other.isReal()) return real.compare(other.real); else return false; // FIXME: Return something better. } 

Sometimes in the comments to our articles suggest that some warnings are issued on the unfinished code. Yes, it happens, but when it really is, it is directly written about it.

Conclusion

Already available reviews of three calculators: Windows Calculator, Qalculate! and SpeedCrunch. We are ready to continue to explore the code of popular calculators. You can suggest projects for verification, as software ratings do not always reflect the real picture.

Check your “Calculator” by downloading PVS-Studio and trying it on your project :-)

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Following in the Footsteps of Calculators: SpeedCrunch