Святослав Размыслов

14 Мар 2019

Теги:

#Cpp

По следам калькуляторов: Qalculate!

14 Мар 2019

Автор: Святослав Размыслов

Введение
Снова copy-paste и опечатки!
Цикл с неверным условием
Избыточность или ошибка?
Разыменование непроверенных указателей
free() или delete []?
Потерянные изменения
Странные указатели
Использование функции find
Потенциальная утечка памяти
Заключение

Ранее мы делали обзоры кода крупных математических пакетов, например, Scilab и Octave, а калькуляторы оставались в стороне как небольшие утилиты, в которых сложно допустить ошибки из-за их малого объёма кода. Мы ошиблись, не уделив им внимания. Случай с публикацией исходного кода калькулятора Windows показал, что всем интересно пообсуждать, какие ошибки там прячутся, а ошибок там более чем достаточно, чтобы написать про это статью. Мы с коллегами решили исследовать код ряда популярных калькуляторов и оказалось, что код калькулятора Windows был не так уж и плох (спойлер).

Введение

Qalculate! - универсальный кроссплатформенный калькулятор. Он прост в использовании, но обеспечивает мощь и универсальность, обычно характерную для сложных математических пакетов, а также полезные инструменты для повседневных нужд (таких как конвертация валюты и расчет процентов). Проект состоит из двух компонентов: libqalculate (library and CLI) и qalculate-gtk (GTK+ UI). В исследовании участвует только код libqalculate.

Чтобы удобнее сравнить проект с тем же калькулятором Windows, который мы недавно исследовали, привожу вывод утилиты Cloc для libqalculate:

Субъективно, ошибок больше, и они более критичные, чем в коде калькулятора Windows. Но рекомендую сделать выводы самостоятельно, ознакомившись с данным обзором кода.

Обзоры ошибок в других проектах:

В качестве инструмента статического анализа использовался PVS-Studio. Это комплекс решений для контроля качества кода, поиска ошибок и потенциальных уязвимостей. В поддерживаемые языки входят: C, C++, C# и Java. Запуск анализатора возможен на Windows, Linux и macOS.

Снова copy-paste и опечатки!

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);
  }
  ....
}

Код в операторе if и else абсолютно одинаковый. Соседние фрагменты кода очень похожи на этот, но в них используются разные функции: internalLowerFloat() и internalUpperFloat(). Можно с уверенностью предположить, что здесь программист скопировал код и забыл поправить имя функции.

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;
  ....
}

Здесь дублирующиеся выражения возникли из-за того, что в одном месте вместо имени mtr написали mtr2. Таким образом, в условии отсутствует вызов функции mtr.number().isReal().

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

Найти аномалии в этом коде вручную нереально! Но они есть. Причём в оригинальном файле эти фрагменты записаны в одну строку. Анализатор обнаружил дублирующееся выражение vargs[1].representsNonPositive(), что может свидетельствовать об опечатке и, следовательно, о потенциальной ошибке.

Вот весь список подозрительных мест, в которых едва ли можно разобраться:

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

Цикл с неверным условием

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;
    }
  }
  ....
}

Во внутреннем цикле счётчиком является переменная i2, но из-за опечатки допущена ошибка - в условии остановки цикла используется переменная i от внешнего цикла.

Избыточность или ошибка?

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;
  ....
}

Насмотревшись на подобный код, 3 года назад я написал заметку для помощи себе и другим программистам: "Логические выражения в C/C++. Как ошибаются профессионалы". Встречая такой код, я убеждаюсь, что заметка ничуть не стала менее актуальной. Вы можете заглянуть в статью, найти паттерн ошибки, соответствующий коду, и узнать все нюансы.

В случае этого примера переходим в раздел "Выражение == || !=" и узнаём, что выражение i2 == COMPARISON_RESULT_UNKNOWN ни на что не влияет.

Разыменование непроверенных указателей

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);
  }
  ....
}

Указатель o_data в одной функции разыменовывается без проверки и с проверкой. Это может быть избыточный код, либо потенциальная ошибка. Я склоняюсь к последнему варианту.

Есть ещё два похожих места:

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() или 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);                                 // <=
      }
    }
    ....
  }
  ....
}

Память под массив remcopy выделяется и освобождается разными способами, что является серьёзной ошибкой.

Потерянные изменения

V672 There is probably no need in creating the new 'm' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 25600, 25626. MathStructure.cc 25626

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;
  }
  ....
}

Переменная m принимается в функции по ссылке, что подразумевает её модификацию. Но анализатор обнаружил, что в коде присутствует одноимённая локальная переменная, которая перекрывает область видимости параметра функции, допуская потерю изменений.

Странные указатели

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;
  }
  ....
}

Анализатор предупреждает, что в коде присутствует обращение к методу объекта cu уже после освобождения памяти. Но если попытаться разобраться в коде, то он окажется ещё более странным. Во-первых, вызов delete cu происходит всегда - в условии и после. Во-вторых, код после условия предполагает, что указатели u и cu не равны, значит после очистки объекта cu логично использовать объект u. Скорее всего, в коде была допущена опечатка и планировалось использовать только переменную u.

Использование функции find

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
}

При работе с функцией realloc() рекомендуется использовать промежуточный буфер, так как в случае невозможности выделения памяти, указатель на старый участок памяти будет безвозвратно утерян.

Заключение

Проект Qalculate! возглавляет список лучших бесплатных калькуляторов, при этом содержит много серьёзных ошибок. Но мы ещё не видели его конкурентов. Постараемся пройтись по всем популярным калькуляторам.

Что касается сравнения с качеством калькулятора из мира Windows, пока утилита от Microsoft выглядит более надёжной и качественной.

Проверь свой "Калькулятор", скачав PVS-Studio и попробовав на своём проекте. :-)

#Cpp