Geekly Articles each Day
📜 ⬆️ ⬇️

In the wake of calculators: Qalculate


Earlier, we did reviews of the code of large mathematical packages, for example, Scilab and Octave, and calculators remained aside as small utilities that make it difficult to make mistakes due to their small amount of code. We made a mistake by not paying attention to them. The case of the publication of the source code of the Windows calculator showed that it is interesting for everyone to discuss what errors are hidden there, and there are more than enough errors to write about this article. My colleagues and I decided to investigate the code of a number of popular calculators and it turned out that the Windows calculator code was not so bad (spoiler).

Introduction


Qalculate! - universal cross-platform calculator. It is easy to use, but it provides the power and versatility typically found in complex math packages, as well as useful tools for everyday needs (such as currency conversion and interest calculation). The project consists of two components: libqalculate (library and CLI) and qalculate-gtk (GTK + UI). Only libqalculate code is involved in the study.

To make it easier to compare the project with the same Windows calculator that we recently researched, I’ll quote the output of the Cloc utility for libqalculate:

Picture 4

Subjectively, there are more errors and they are more critical than in the Windows calculator code. But I recommend to draw conclusions independently, having familiarized with the given review of the code.
')
By the way, here is the link to the article about checking the calculator from Microsoft: " Let's calculate the bugs in the Windows calculator ".

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.

Copy-paste and typos again!


V523 The 'then' statement is equivalent to the 'else' statement. Number.cc 4018

bool Number::square() { .... if(mpfr_cmpabs(i_value->internalLowerFloat(), i_value->internalUpperFloat()) > 0) { mpfr_sqr(f_tmp, i_value->internalLowerFloat(), MPFR_RNDU); mpfr_sub(f_rl, f_rl, f_tmp, MPFR_RNDD); } else { mpfr_sqr(f_tmp, i_value->internalLowerFloat(), MPFR_RNDU); mpfr_sub(f_rl, f_rl, f_tmp, MPFR_RNDD); } .... }

The code in the if and else statements is exactly the same. Neighboring code snippets are very similar to this, but they use different functions: internalLowerFloat () and internalUpperFloat () . It is safe to assume that here the programmer copied the code and forgot to correct the function name.

V501 There are identical sub-expressions '! Mtr2.number (). IsReal ()' operator. BuiltinFunctions.cc 6274

 int IntegrateFunction::calculate(....) { .... if(!mtr2.isNumber() || !mtr2.number().isReal() || !mtr.isNumber() || !mtr2.number().isReal()) b_unknown_precision = true; .... }

Here, duplicate expressions arose from the fact that in one place instead of the name mtr they wrote mtr2 . Thus, in the condition there is no function call mtr.number (). IsReal () .

V501 There are identical sub-expressions 'vargs [1] .representsNonPositive ()' operator. BuiltinFunctions.cc 5785

Picture 6



Finding anomalies in this code manually is unreal! But they are. And in the original file, these fragments are written in one line. The analyzer detected a duplicate expression vargs [1] .representsNonPositive () , which may indicate a typo and, therefore, a potential error.

Here is a list of suspicious places that can hardly be sorted out:


Loop with invalid condition


V534 It is likely that a variable is being compared inside the 'for' operator. Consider reviewing 'i'. MathStructure.cc 28741

 bool MathStructure::isolate_x_sub(....) { .... for(size_t i = 0; i < mvar->size(); i++) { if((*mvar)[i].contains(x_var)) { mvar2 = &(*mvar)[i]; if(mvar->isMultiplication()) { for(size_t i2 = 0; i < mvar2->size(); i2++) { if((*mvar2)[i2].contains(x_var)) {mvar2 = &(*mvar2)[i2]; break;} } } break; } } .... }

In the internal cycle, the counter is the variable i2 , but due to a typo, an error is made - the condition for stopping the cycle is variable i from the external cycle.

Redundancy or error?


V590 Consider inspecting this expression. The expression is misprint. Number.cc 6564

 bool Number::add(const Number &o, MathOperation op) { .... if(i1 >= COMPARISON_RESULT_UNKNOWN && (i2 == COMPARISON_RESULT_UNKNOWN || i2 != COMPARISON_RESULT_LESS)) return false; .... }

Having seen enough of this code, 3 years ago I wrote a note to help myself and other programmers: " Logical expressions in C / C ++. How professionals are mistaken ." Meeting this code, I am convinced that the note has not become any less relevant. You can look at the article, find the error pattern corresponding to the code, and find out all the nuances.

In the case of this example, go to the section "Expression == || ! = "And find out that the expression i2 == COMPARISON_RESULT_UNKNOWN does not affect anything.

Dereferencing unverified pointers


V595 The 'o_data' pointer was used before it was verified against nullptr. Check lines: 1108, 1112. DataSet.cc 1108

 string DataObjectArgument::subprintlong() const { string str = _("an object from"); str += " \""; str += o_data->title(); // <= str += "\""; DataPropertyIter it; DataProperty *o = NULL; if(o_data) { // <= o = o_data->getFirstProperty(&it); } .... }

The o_data pointer in one function is dereferenced without checking and with checking. This may be a redundant code or a potential error. I tend to the last option.

There are two more similar places:


free () or delete []?


V611 The memory was allocated using the 'new' operator. Consider inspecting operation logics behind the 'remcopy' variable. Number.cc 8123

 string Number::print(....) const { .... while(!exact && precision2 > 0) { if(try_infinite_series) { remcopy = new mpz_t[1]; // <= mpz_init_set(*remcopy, remainder); } mpz_mul_si(remainder, remainder, base); mpz_tdiv_qr(remainder, remainder2, remainder, d); exact = (mpz_sgn(remainder2) == 0); if(!started) { started = (mpz_sgn(remainder) != 0); } if(started) { mpz_mul_si(num, num, base); mpz_add(num, num, remainder); } if(try_infinite_series) { if(started && first_rem_check == 0) { remainders.push_back(remcopy); } else { if(started) first_rem_check--; mpz_clear(*remcopy); free(remcopy); // <= } } .... } .... }

Memory for the remcopy array is allocated and released in various ways, which is a serious error.

Lost Changes


 bool expand_partial_fractions(MathStructure &m, ....) { .... if(b_poly && !mquo.isZero()) { MathStructure m = mquo; if(!mrem.isZero()) { m += mrem; m.last() *= mtest[i]; m.childrenUpdated(); } expand_partial_fractions(m, eo, false); return true; } .... }

The variable m is taken as a function by reference, which implies its modification. But the analyzer found that there is a local variable of the same name in the code that overlaps the scope of the parameter of the function, allowing for the loss of changes.

Strange signs


V774 The 'cu' pointer was used after the memory was released. Calculator.cc 3595

 MathStructure Calculator::convertToBestUnit(....) { .... CompositeUnit *cu = new CompositeUnit("", "...."); cu->add(....); Unit *u = getBestUnit(cu, false, eo.local_currency_conversion); if(u == cu) { delete cu; // <= return mstruct_new; } delete cu; // <= if(eo.approximation == APPROXIMATION_EXACT && cu->hasApproximateRelationTo(u, true)) { // <= if(!u->isRegistered()) delete u; return mstruct_new; } .... }

The analyzer warns that the code contains a call to the cu object method after the memory is freed. But if you try to understand the code, it will be even more strange. First, the call cu is always invoked - in the condition and after. Secondly, the code after the condition assumes that the pointers u and cu are not equal, it means that after cleaning the cu object, it is logical to use the object u . Most likely, there was a typo in the code and it was planned to use only the variable u .

Using the find function


V797 The 'find' function. The return value of the function should not be compared with std :: string :: npos. Unit.cc 404

 MathStructure &AliasUnit::convertFromFirstBaseUnit(....) const { if(i_exp != 1) mexp /= i_exp; ParseOptions po; if(isApproximate() && suncertainty.empty() && precision() == -1) { if(sinverse.find(DOT) || svalue.find(DOT)) po.read_precision = READ_PRECISION_WHEN_DECIMALS; else po.read_precision = ALWAYS_READ_PRECISION; } .... }

Although the code successfully compiles, it looks suspicious, since the find function returns a number of type std :: string :: size_type . The condition will be true if the point is found anywhere on the line, unless the point is at the beginning. This is a strange test. I'm not sure, but maybe the code should be rewritten as follows:

 if( sinverse.find(DOT) != std::string::npos || svalue.find(DOT) != std::string::npos) { po.read_precision = READ_PRECISION_WHEN_DECIMALS; }

Potential memory leak


V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'buffer' is lost. Consider assigning realloc () to a temporary pointer. util.cc 703

 char *utf8_strdown(const char *str, int l) { #ifdef HAVE_ICU .... outlength = length + 4; buffer = (char*) realloc(buffer, outlength * sizeof(char)); // <= .... #else return NULL; #endif }

When working with the realloc () function, it is recommended to use an intermediate buffer, since if memory cannot be allocated, the pointer to the old memory location will be irretrievably lost.

Conclusion


Project Qalculate! tops the list of the best free calculators, while containing many serious mistakes. But we have not yet seen its competitors. We will try to go through all the popular calculators.

As for comparison with the quality of the calculator from the Windows world, while the utility from Microsoft looks more reliable and high-quality.

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: Qalculate!

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

More articles:


All Articles