Following the trail of calculators: Qalculate / PVS-Studio Blog

SvyatoslavMC March 14, 2019 at 08:54

Following the trail of calculators: Qalculate

Earlier we did code reviews of large mathematical packages, for example, Scilab and Octave, and calculators remained aside as small utilities in which it is difficult to make mistakes due to their small code size. We were mistaken in not paying attention to them. The case with the publication of the source code of the Windows calculator showed that everyone is interested in discussing what errors are hidden there, and there are more than enough errors there to write an article about it. My colleagues and I decided to examine the code of a number of popular calculators, and it turned out that the code of the Windows calculator was not so bad (spoiler).

Introduction

Qalculate! - universal cross-platform calculator. It is easy to use, but 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 more conveniently compare the project with the same Windows calculator that we recently examined, I give the output of the Cloc utility for libqalculate:

Subjectively, there are more errors, and they are more critical than in the code of the Windows calculator. But I recommend that you draw conclusions yourself by reading this code review.

By the way, here is a link to an article about checking a calculator from Microsoft: " Counting bugs in a Windows calculator. " PVS-Studio was

used as a static analysis tool . This is a set of solutions for code quality control, search for errors and potential vulnerabilities. Supported languages include: C, C ++, C #, and Java. The analyzer can be launched 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 statement is exactly the same. The neighboring code fragments 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 ()' to the left and to the right of the '||' 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 because in one place instead of the name mtr they wrote mtr2 . Thus, in the condition there is no call to the mtr.number (). IsReal () function .

V501 There are identical sub-expressions 'vargs [1] .representsNonPositive ()' to the left and to the right of the '||' operator. BuiltinFunctions.cc 5785

To find anomalies in this code manually is unreal! But they are. Moreover, 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 the whole list of suspicious places that you can hardly figure out:

V501 There are identical sub-expressions 'vargs [1] .representsNonPositive ()' to the left and to the right of the '||' operator. BuiltinFunctions.cc 5788
V501 There are identical sub-expressions 'append' to the left and to the right of the '&&' operator. MathStructure.cc 1780
V501 There are identical sub-expressions 'append' to the left and to the right of the '&&' operator. MathStructure.cc 2043
V501 There are identical sub-expressions '(* v_subs [v_order [1]]). RepresentsNegative (true)' to the left and to the right of the '&&' operator. MathStructure.cc 5569

Invalid Loop

V534 It is likely that a wrong 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 inner loop, the counter is the variable i2 , but due to a typo, an error was made - in the condition of stopping the loop, the variable i from the outer loop is used.

Redundancy or mistake?

V590 Consider inspecting this expression. The expression is excessive or contains a 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 looked at such code, 3 years ago I wrote a note to help myself and other programmers: " Logical expressions in C / C ++. How professionals are wrong ." Upon encountering such a code, I am convinced that the note has not become less relevant at all. You can look in 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 we learn that the expression i2 == COMPARISON_RESULT_UNKNOWN does not affect anything.

Dereferencing unverified pointers

V595 The 'o_data' pointer was utilized 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 redundant code, or a potential error. I am inclined to the last option.

There are two more similar places:

V595 The 'o_assumption' pointer was utilized before it was verified against nullptr. Check lines: 229, 230. Variable.cc 229
V595 The 'i_value' pointer was utilized before it was verified against nullptr. Check lines: 3412, 3427. Number.cc 3412

free () or delete []?

V611 The memory was allocated using 'new' operator but was released using the 'free' function. 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 freed in various ways, which is a serious mistake.

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 accepted in the function by reference, which implies its modification. But the analyzer found that the code contains the local variable of the same name, which overlaps the scope of the function parameter, allowing the loss of changes.

Strange pointers

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 method of the cu object after freeing memory. But if you try to understand the code, then it will be even more strange. Firstly, the call to delete cu always occurs - in the condition and after. Secondly, the code after the condition assumes that the pointers u and cu are not equal, so after clearing the cu object, it is logical to use the u object . Most likely, a typo was made in the code and it was planned to use only the variable u .

Using the find function

V797 The 'find' function is used as if it returned a bool type. The return value of the function should probably 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;
  }
  ....
}

Хотя код успешно компилируется, он выглядит подозрительным, так как функция find возвращает число типа std::string::size_type. Условие будет истинно, если точка будет найдена в любом месте строки, кроме случая, если точка стоит в начале. Это странная проверка. Я не уверен, но возможно, код следует переписать следующим образом:

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

Потенциальная утечка памяти

V701 realloc() possible leak: when realloc() fails in allocating memory, 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 it is impossible to allocate memory, the pointer to the old memory will be irretrievably lost.

Conclusion

Qalculate project! tops the list of the best free calculators, while it contains many serious errors. But we have not seen his competitors. We will try to go through all the popular calculators.

As for the 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!

Tags:

Following the trail of calculators: Qalculate / PVS-Studio Blog