Following the trail of calculators: SpeedCrunch

    Picture 4

    The study of calculator code continues! In this review, the SpeedCrunch project will be reviewed - the second most popular among free calculators.

    Introduction


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

    The source code is hosted on BitBucket . I didn’t really like the assembly documentation, which, in my opinion, should be written in more detail. The requirements specify “Qt 5.2 or later”, although several specific packages were needed, which were not easy to learn from the CMake log. By the way, now it’s good practice to apply a Dockerfile to a project to quickly configure the desired developer environment.

    For comparison with other calculators, I bring the output of the Cloc utility:

    Picture 2


    Reviews of bugs in other projects:


    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.

    Strange logic in a loop


    V560 A part of 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();
            }
        }
        ....
      }
      ....
    }

    Note 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 at the new iteration of the cycle. Most likely, the ruleFound variable needed to be declared before the loop.

    Suspicious comparisons


    V560 A part of 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 shouldStop variable is true , then the m_scrollDirection variable will have one of two values: -1 or 1. Therefore, in the following conditional statement, the value of the m_scrollDirection variable will definitely not be zero, which the analyzer warns about.

    V668 There is no sense in testing the 'item' pointer against null, as the memory was allocated using the 'new' operator. 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 not possible to allocate dynamic memory, an exception std :: bad_alloc () will be thrown . Therefore, checking the item pointer is superfluous and can be deleted.

    Potential NULL Dereference


    V595 The 'ioparams' pointer was utilized 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 it is checked for validity. Most likely, a potential error crept into the code. Since dereferencing is under several conditions, the problem can rarely manifest itself, but accurately.

    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 a null value to be returned, by which division is then performed. Potentially, anything other than values ​​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 inverse of zero is placed in the signed type int , so the result will be a negative number, for which a shift is then performed. Shifting a negative number to the left is undefined behavior.

    The whole 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

    Unclosed HTML Tags


    V735 Possibly an incorrect HTML. The "" closing tag was encountered, while the "
    "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), logax = log(x) / log(a))
        END;
    }

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

    Picture 3



    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 developers.

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

    • V735 Possibly an incorrect HTML. The "" closing tag was encountered, while the "" tag was expected. book.cpp 344
    • V735 Possibly an incorrect HTML. The "" closing tag was encountered, while the "" tag was expected. book.cpp 347

    Assignment operator


    V794 The assignment operator should 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 the 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 'false' value is 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 they suggest that some warnings are issued on an incomplete 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 research the code of popular calculators. You can offer projects for verification, since the ratings of the software do not always reflect the real picture.

    Check your “Calculator” by downloading PVS-Studio and trying 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

    Also popular now: