Программы для работы с музыкой имеют маленький объём кода и, поначалу, я сомневался в возможности находить достаточное количество ошибок для статей. Тематику музыкального софта всё равно хотелось затронуть, поэтому я был готов объединять несколько проектов в статье. И вот я пишу уже третью статью, стараясь хоть как-то вместить интересные ошибки в одну статью. Третьим проектом для анализа выбран MIDI-секвенсор и нотный редактор - Rosegarden. Внимание! Прочтение статьи вызывает "Facepalm"!
Rosegarden — свободный MIDI-секвенсор, нотный редактор для Linux, использующий ALSA и JACK, программа для создания и редактирования музыки наподобие Apple Logic Pro, Cakewalk Sonar и Steinberg Cubase.
В статью вошли только наиболее интересные ошибки, найденные мною с помощью PVS-Studio. Для просмотра полного отчёта авторы могут самостоятельно проверить проект, запросив в поддержке временный ключ.
PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в среде Windows и Linux.
Ложные предупреждения всегда составляют некую часть отчёта профессионального статического анализатора кода. Обидно, когда люди просто не хотят написать код лучше и просто списывают предупреждения как ложные. Иногда код написан так запутанно, что другому программисту не под силу разобраться без отладки. Так или иначе, мы стараемся учитывать такие ситуации, чтобы анализатор не выдавал ложные предупреждения. Для этого сейчас активно развивается Data-flow analysis, что, кроме уменьшения количества ложных предупреждений, позволяет находить интересные ошибки.
V560 A part of conditional expression is always false: singleStaff. NotationScene.cpp 1707
void NotationScene::layout(....)
{
....
bool full = (singleStaff == 0 && startTime == endTime);
m_hlayout->setViewSegmentCount(m_staffs.size());
if (full) {
Profiler profiler("....", true);
m_hlayout->reset();
m_vlayout->reset();
bool first = true;
for (unsigned int i = 0; i < m_segments.size(); ++i) {
if (singleStaff && // <= Always False
m_segments[i] != &singleStaff->getSegment()) {
continue;
}
timeT thisStart = m_segments[i]->getClippedStartTime();
timeT thisEnd = m_segments[i]->getEndMarkerTime();
if (first || thisStart < startTime) startTime = thisStart;
if (first || thisEnd > endTime) endTime = thisEnd;
first = false;
}
}
....
}Из-за логической ошибки в цикле for никогда не выполняется оператор continue, что, вероятно, приводит к выполнению лишних итераций цикла. Причиной этого является проверка указателя singleStaff в условии с оператором '&&'. Значение указателя singleStff всегда равно нулю. Весь этот код находится под условием "if (full)", при вычислении которого анализатор увидел зависимость от переменной singleStaff:
bool full = (singleStaff == 0 && startTime == endTime);Значение переменной full будет равно true только в том случае, если указатель singleStaff будет нулевым.
В этом разделе я собрал разные примеры ошибок, так или иначе приводящих к невыполнению кода. Всё это относится к CWE-571: Expression is Always True, CWE-570: Expression is Always False, CWE-561: Dead Code и их вариациям.
V547 Expression '!beamedSomething' is always true. SegmentNotationHelper.cpp 1405
void SegmentNotationHelper::makeBeamedGroupAux(....)
{
int groupId = segment().getNextId();
bool beamedSomething = false; // <=
for (iterator i = from; i != to; ++i) {
....
if ((*i)->isa(Note::EventType) &&
(*i)->getNotationDuration() >= Note(....).getDuration()) {
if (!beamedSomething) continue; // <=
iterator j = i;
bool somethingLeft = false;
while (++j != to) {
if ((*j)->getType() == Note::EventType &&
(*j)->getNotationAbsoluteTime() > (*i)->get....() &&
(*j)->getNotationDuration() < Note(....).getDuration()) {
somethingLeft = true;
break;
}
}
if (!somethingLeft) continue;
}
....
}Этот пример очень похож на код, приведённый в предыдущем разделе, но немного проще. Переменная beamedSomething инициализируется значением false и больше не изменяется. Вследствие чего в цикле for всегда выполняется оператор continue, из-за чего никогда не выполняется большой фрагмент кода.
V547 Expression 'i > 5' is always false. SegmentParameterBox.cpp 323
void SegmentParameterBox::initBox()
{
....
for (int i = 0; i < 6; i++) {
timeT time = 0;
if (i > 0 && i < 6) {
time = Note(Note::Hemidemisemiquaver).get.... << (i - 1);
} else if (i > 5) {
time = Note(Note::Crotchet).getDuration() * (i - 4);
}
....
}Счётчик цикла принимает значения в диапазоне от 0 до 5. Первое условное выражение выполняется для всех значений счётчика, кроме нулевого, в то время как второе условное выражение не выполняется никогда, т.к. ожидает, что переменная i будет иметь значение от 6 и более.
V547 Expression 'adjustedOctave < 8' is always false. NotePixmapFactory.cpp 1920
QGraphicsPixmapItem* NotePixmapFactory::makeClef(....)
{
....
int oct = clef.getOctaveOffset();
if (oct == 0) return plain.makeItem();
int adjustedOctave = (8 * (oct < 0 ? -oct : oct));
if (adjustedOctave > 8)
adjustedOctave--;
else if (adjustedOctave < 8)
adjustedOctave++;
....
}Начнём разбирать пример по порядку. Переменная oct сначала инициализируется неким значением знакового типа, а потом из этого диапазона исключается нулевое значение. Далее вычисляется модуль переменной oct и умножается на 8. Результирующее значение в adjustedOctave будет иметь диапазон [8..N), что делает проверку (adjustedOctave < 8) бессмысленной.
V547 Expression '""' is always true. LilyPondOptionsDialog.cpp 64
LilyPondOptionsDialog::LilyPondOptionsDialog(....)
{
setModal(true);
setWindowTitle((windowCaption = "" ?
tr("LilyPond Export/Preview") : windowCaption));
....
}Интересная ошибка с формированием заголовка модального окна. Видимо хотели задавать новый заголовок окна, если текущее значение отсутствует, но допустили ошибку в операторе.
Для тех, кто сразу не заметил опечатку, подскажу. Должен был использоваться оператор '==', а не оператор '='.
Такой же код используется при показе другого окна:
Примечание. Если автор кода хотел в одной строке задать новый заголовок и стереть старый таким образом, то нет — так писать не круто.
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 223, 239. IntervalDialog.cpp 223
QString IntervalDialog::getIntervalName(....)
{
....
if (deviation == -1)
textIntervalDeviated += tr("a minor");
else if (deviation == 0) // <=
textIntervalDeviated += tr("a major");
else if (deviation == -2)
textIntervalDeviated += tr("a diminished");
else if (deviation == 1)
textIntervalDeviated += tr("an augmented");
else if (deviation == -3)
textIntervalDeviated += tr("a doubly diminished");
else if (deviation == 2)
textIntervalDeviated += tr("a doubly augmented");
else if (deviation == -4)
textIntervalDeviated += tr("a triply diminished");
else if (deviation == 3)
textIntervalDeviated += tr("a triply augmented");
else if (deviation == 4)
textIntervalDeviated += tr("a quadruply augmented");
else if (deviation == 0) // <=
textIntervalDeviated += tr("a perfect");
....
}Одно из условий лишнее или записано с ошибкой. Значение 0 уже обработано в самом начале.
В этом разделе я приведу несколько интересных фрагментов кода для работы с файлами. Похоже, программист вдохновлялся такими языками программирования, как C# и Java. В противном случае непонятно, почему не создать экземпляр типа ifstream просто как переменную на стеке. Динамическое выделение памяти здесь явно избыточно и, вдобавок, стало поводом для ошибки.
V773 The function was exited without releasing the 'testFile' pointer. A memory leak is possible. RIFFAudioFile.cpp 561
AudioFileType
RIFFAudioFile::identifySubType(const QString &filename)
{
std::ifstream *testFile =
new std::ifstream(filename.toLocal8Bit(),
std::ios::in | std::ios::binary);
if (!(*testFile))
return UNKNOWN;
....
testFile->close();
delete testFile;
delete [] bytes;
return type;
}Если с файлом возникают проблемы, то указатель testFile не освобождается при выходе из функции. Это распространённый паттерн, приводящий к утечке памяти.
V773 The function was exited without releasing the 'midiFile' pointer. A memory leak is possible. MidiFile.cpp 1531
bool
MidiFile::write(const QString &filename)
{
std::ofstream *midiFile =
new std::ofstream(filename.toLocal8Bit(),
std::ios::out | std::ios::binary);
if (!(*midiFile)) {
RG_WARNING << "write() - can't write file";
m_format = MIDI_FILE_NOT_LOADED;
return false;
}
....
midiFile->close();
return true;
}Вы могли подумать, что этот фрагмент кода такой же, как предыдущий, но это не совсем так. В отличии от первого примера, в этой функции вообще нет освобождения памяти. Утечка памяти происходит всегда.
V668 There is no sense in testing the 'file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SF2PatchExtractor.cpp 94
SF2PatchExtractor::Device
SF2PatchExtractor::read(string fileName)
{
Device device;
ifstream *file = new ifstream(fileName.c_str(), ios::in |....);
if (!file)
throw FileNotFoundException();
....
}Вот список проблем этого фрагмента кода:
Причём такое место не одно:
V601 The integer type is implicitly cast to the char type. MidiEvent.cpp 181
QDebug &
operator<<(QDebug &dbg, const MidiEvent &midiEvent)
{
timeT tempo;
int tonality;
std::string sharpflat;
....
tonality = (int)midiEvent.m_metaMessage[0];
if (tonality < 0) {
sharpflat = -tonality + " flat"; // <=
} else {
sharpflat = tonality; // <=
sharpflat += " sharp";
}
....
}Предположим, значение переменной tonality было '42', тогда в указанных в коде местах хотели получить такие строки: "42 flat" или "42 sharp". Но это работает совсем не так, как ожидает программист. Конвертации числа в строку не происходит, вместо этого в строку сохраняется смещённый указатель, образуя мусор в буфере. Или случится access violation. Или вообще произойдет что угодно, так как доступ за границу массива приводит к неопределённому поведению программы.
Исправить ошибку можно следующим образом:
if (tonality < 0) {
sharpflat = to_string(-tonality) + " flat";
} else {
sharpflat = to_string(tonality);
sharpflat += " sharp";
}V674 The '0.1' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'm_connectingLineLength > 0.1' expression. StaffLayout.cpp 1028
class StaffLayout
{
....
protected:
int m_connectingLineLength;
....
}
int m_connectingLineLength;
void
StaffLayout::resizeStaffLineRow(int row, double x, double length)
{
....
if (m_pageMode != LinearMode && m_connectingLineLength > 0.1) {
....
}Сравнение переменой типа int со значением 0.1 выполнять бессмысленно. Возможно, здесь задумывалось что-то другое. Авторам проекта стоит внимательно изучить этот код.
V601 The string literal is implicitly cast to the bool type. FileSource.cpp 902
bool
FileSource::createCacheFile()
{
{
QMutexLocker locker(&m_mapMutex);
#ifdef DEBUG_FILE_SOURCE
std::cerr << "...." << m_refCountMap[m_url] << std::endl;
#endif
if (m_refCountMap[m_url] > 0) {
m_refCountMap[m_url]++;
m_localFilename = m_remoteLocalMap[m_url];
#ifdef DEBUG_FILE_SOURCE
std::cerr << "...." << m_refCountMap[m_url] << std::endl;
#endif
m_refCounted = true;
return true;
}
}
QDir dir;
try {
dir = TempDirectory::getInstance()->....;
} catch (DirectoryCreationFailed f) {
#ifdef DEBUG_FILE_SOURCE
std::cerr << "...." << f.what() << std::endl;
#endif
return ""; // <=
}
....
}В одном месте, вместо значений true/false, функция возвращает пустую строку, что всегда интерпретируется как true.
Использование итераторов в этом проекте выглядит не менее странным, чем работа с файлами.
V783 Dereferencing of the invalid iterator 'i' might take place. IconStackedWidget.cpp 126
void
IconStackedWidget::slotPageSelect()
{
iconbuttons::iterator i = m_iconButtons.begin();
int index = 0;
while (((*i)->isChecked() == false) &&
(i != m_iconButtons.end())) {
++i;
index++;
}
m_pagePanel->setCurrentIndex(index);
}В цикле while перепутан порядок проверки итератора i. В этом коде ничего необычного нет, классическая ошибка.
V783 Dereferencing of the invalid iterator 'beatTimeTs.end()' might take place. CreateTempoMapFromSegmentCommand.cpp 119
void
CreateTempoMapFromSegmentCommand::initialise(Segment *s)
{
....
std::vector<timeT> beatTimeTs;
....
for (int i = m_composition->...At(*beatTimeTs.begin() - 1) + 1;
i <= m_composition->...At(*beatTimeTs.end() - 1); ++i){
....
}Анализатор обнаружил ещё одно обращение к итератору end(). Возможно, хотели получить примерно такой код:
...At(*(beatTimeTs.end() - 1))но забыли скобочки.
Похожий код есть и в другом файле:
V1004 The 'track' pointer was used unsafely after it was verified against nullptr. Check lines: 319, 329. MatrixView.cpp 329
void
MatrixView::slotUpdateWindowTitle(bool m)
{
....
Track *track =
m_segments[0]->getComposition()->getTrackById(trackId);
int trackPosition = -1;
if (track)
trackPosition = track->getPosition(); // <=
QString segLabel = strtoqstr(m_segments[0]->getLabel());
if (segLabel.isEmpty()) {
segLabel = " ";
} else {
segLabel = QString(" \"%1\" ").arg(segLabel);
}
QString trkLabel = strtoqstr(track->getLabel()); // <=
....
}Стрелочками я отметил два места, в которых разыменовывается указатель track. Первое место является безопасным, т.к. указатель точно ненулевой. Второе место может приводить к неопределённому поведению программы. В приведённом фрагменте кода нет косвенных проверок. Код выполняется последовательно и содержит потенциальную ошибку.
Другие опасные разыменования указателей:
V595 The 'm_scene' pointer was utilized before it was verified against nullptr. Check lines: 1001, 1002. NotationWidget.cpp 1001
void
NotationWidget::slotEnsureTimeVisible(timeT t)
{
m_inMove = true;
QPointF pos = m_view->mapToScene(0,m_view->height()/2);
pos.setX(m_scene->getRulerScale()->getXForTime(t)); // <=
if (m_scene) m_scene->constrainToSegmentArea(pos); // <=
m_view->ensureVisible(QRectF(pos, pos));
m_inMove = false;
}Диагностика V595 находит похожий тип ошибок. Здесь указатель s_scene разыменовывается в одной строке, а сразу на следующей проверяется, является ли он валидным.
V595 The 'm_hideSignatureButton' pointer was utilized before it was verified against nullptr. Check lines: 248, 258. TimeSignatureDialog.cpp 248
TimeSignature
TimeSignatureDialog::getTimeSignature() const
{
QSettings settings;
settings.beginGroup( GeneralOptionsConfigGroup );
settings.setValue("timesigdialogmakehidden",
m_hideSignatureButton->isChecked()); // <=
settings.setValue("timesigdialogmakehiddenbars",
m_hideBarsButton->isChecked()); // <=
settings.setValue("timesigdialogshowcommon",
m_commonTimeButton->isChecked()); // <=
settings.setValue("timesigdialognormalize",
m_normalizeRestsButton->isChecked());
TimeSignature ts(m_timeSignature.getNumerator(),
m_timeSignature.getDenominator(),
(m_commonTimeButton &&
m_commonTimeButton->isEnabled() &&
m_commonTimeButton->isChecked()),
(m_hideSignatureButton && // <=
m_hideSignatureButton->isEnabled() &&
m_hideSignatureButton->isChecked()),
(m_hideBarsButton &&
m_hideBarsButton->isEnabled() &&
m_hideBarsButton->isChecked()));
settings.endGroup();
return ts;
}Похожая на предыдущий пример ошибка, но я решил всё равно привести этот фрагмент кода. Здесь выполняется сразу три разыменования потенциально нулевых указателей.
Все остальные похожие места приведу списком:
Ошибка с порядком инициализации членов класса является очень редкой. В нашей базе ошибок есть всего двенадцать упоминаний об этой ошибке.
V670 The uninitialized class member 'm_intervals' is used to initialize the 'm_size' member. Remember that members are initialized in the order of their declarations inside a class. Tuning.cpp 394
class Tuning {
....
int m_size; // line 138
const IntervalList *m_intervals; // line 139
....
}
Tuning::Tuning(const Tuning *tuning) :
m_name(tuning->getName()),
m_rootPitch(tuning->getRootPitch()),
m_refPitch(tuning->getRefPitch()),
m_size(m_intervals->size()),
m_intervals(tuning->getIntervalList()),
m_spellings(tuning->getSpellingList())
{
....
}Поля класса инициализируются в том порядке, в каком определены в классе. В приведённом примере кода поле m_size проинициализируется первым и будет иметь некорректное значение.
V557 Array overrun is possible. The value of 'submaster' index could reach 64. SequencerDataBlock.cpp 325
#define SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS 64
class SequencerDataBlock
{
....
protected:
int m_submasterLevelUpdateIndices[64];
....
}
bool
SequencerDataBlock::getSubmasterLevel(int submaster, ....) const
{
....int lastUpdateIndex[SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS];
if (submaster < 0 ||
submaster > SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS) {
info.level = info.levelRight = 0;
return false;
}
int currentUpdateIndex=m_submasterLevelUpdateIndices[submaster];
info = m_submasterLevels[submaster];
if (lastUpdateIndex[submaster] != currentUpdateIndex) {
lastUpdateIndex[submaster] = currentUpdateIndex;
return true;
} else {
return false; // no change
}
}Эта ошибка уже стала классикой. При сравнении индекса массива с максимальным значением, почему-то всегда путают оператор '>' и '>='. Такая ошибка приводит к обращению за пределы массива. В данном случае, даже к двум массивам.
Правильная проверка должна выглядеть так:
if (submaster < 0 ||
submaster >= SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS) {Такой код скопирован в ещё две функции:
V612 An unconditional 'break' within a loop. Fingering.cpp 83
Fingering::Barre
Fingering::getBarre() const
{
int lastStringStatus = m_strings[getNbStrings() - 1];
Barre res;
res.fret = lastStringStatus;
for(unsigned int i = 0; i < 3; ++i) {
if (m_strings[i] > OPEN && m_strings[i] == lastStringStatus)
res.start = i;
break;
}
res.end = 5;
return res;
}Я уже приводил примеры кода, стили которых были похожи на C# или Java. Вот тут явное сходство с языком Python. К сожалению (для автора кода), в C++ это так не работает. Оператор break не находится в условии, а выполняется всегда на первой итерации цикла.
V746 Object slicing. An exception should be caught by reference rather than by value. MupExporter.cpp 197
timeT MupExporter::writeBar(....)
{
....
try {
// tuplet compensation, etc
Note::Type type = e->get<Int>(NOTE_TYPE);
int dots = e->get
<Int>(NOTE_DOTS);
duration = Note(type, dots).getDuration();
} catch (Exception e) { // no properties
RG_WARNING << "WARNING: ...: " << e.getMessage();
}
....
}Перехват исключения по значению может приводить к нескольким типам шибок. В коде проекта я нашёл такой класс:
class BadSoundFileException : public ExceptionПри перехвате исключения по значению, будет создан новый объект класса Exception, а информация об унаследованном классе BadSoundFileException будет утеряна.
Таких мест в проекте около пятидесяти.
V523 The 'then' statement is equivalent to the 'else' statement. HydrogenXMLHandler.cpp 476
bool
HydrogenXMLHandler::characters(const QString& chars)
{
bool rc=false;
if (m_version=="") {
/* no version yet, use 093 */
rc=characters_093(chars);
}
else {
/* select version dependant function */
rc=characters_093(chars);
}
return rc;
}Подозрительное место. Наличие разных комментариев предполагает разный код, но здесь это не так.
Два похожих предупреждения:
Пока это проект с самым низким качеством кода из всех в обзоре. Будем продолжать исследование дальше.
Другие обзоры музыкального софта:
Если вы знаете интересный софт для работы с музыкой и хотите увидеть его в обзоре, то присылайте названия мне на почту.
Попробовать анализатор PVS-Studio на своём проекте очень легко, достаточно перейти на страницу загрузки.