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


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

    Введение


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

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

    Picture 4

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

    Кстати, вот ссылка на статью про проверку калькулятора от Microsoft: "Подсчитаем баги в калькуляторе 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

    Picture 6



    Найти аномалии в этом коде вручную нереально! Но они есть. Причём в оригинальном файле эти фрагменты записаны в одну строку. Анализатор обнаружил дублирующееся выражение 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 выделяется и освобождается разными способами, что является серьёзной ошибкой.

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


    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 и попробовав на своём проекте. :-)



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Following in the Footsteps of Calculators: Qalculate!
    PVS-Studio
    774,00
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 13

      0
      The issue
      Fixed.

      Most listed errors were harmless or affected very unusual use cases. The most critical error was only present in unreleased code and had already been fixed. The example above in Number.cc resulted in a potentially too narrow interval when calculating the square of a complex interval with different sign for upper and lower endpoint.
        +5
        Как обычно, разработчики исправили очень много кода, но сказали, что там ничего интересного не было) Но результаты анализа позволяют делать такой вывод: программисты допускают ошибки во всём коде, но прикладывают много усилий, чтобы исправить критические из них. Но если использовать статический анализатор регулярно — большинство проблем во всём коде были бы выявлены на раннем этапе.
          0
          Разработчица из Швеции…
          Надо было эксплоит написать :) Шутка конечно.
        0
        Следующий: KCalc из Mageia 6 пожалуйста. :)
          0
          На днях выложу обзор на ещё один популярный калькулятор. Его упоминали в комментариях к калькулятору Windows, но он не из вашего списка) KCalc гляну… но вы можете тоже в этом поучаствовать, воспользовавшись PVS-Studio :)
            0
            но вы можете тоже в этом поучаствовать, воспользовавшись PVS-Studio

            А не идет ли это вразрез со словами вашего коллеги отсюда, раз KCalc часть проекта KDE?
              0
              Нет. Там речь про предоставление бесплатной лицензии для регулярного использования. Мы же проверяем любые открытые проекты, в том числе KDE (2014 год).
          +1

          SpeedCrunch посмотрите, если можно.

          0
          Спасибо, очень интересно. Надеюсь, что разработчики починят все проблемы и можно будет и дальше пользоваться этим замечательным ПО.
            0
            Чтобы починить все проблемы, нужно пользоваться им постоянно.
            0
            Про KmPlot почитал бы с интересом, пусть и строилка графиков. Ещё KAlgebra есть из интересного, но там функционал пообширней калькулятора будет.
              0
              Вы настоящая скорая помощь :)

              Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

              Самое читаемое