Ранее мы делали обзоры кода крупных математических пакетов, например, 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.
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(), что может свидетельствовать об опечатке и, следовательно, о потенциальной ошибке.
Вот весь список подозрительных мест, в которых едва ли можно разобраться:
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 в одной функции разыменовывается без проверки и с проверкой. Это может быть избыточный код, либо потенциальная ошибка. Я склоняюсь к последнему варианту.
Есть ещё два похожих места:
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.
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 и попробовав на своём проекте. :-)