Программирование - занятие творческое, поэтому среди разработчиков встречается много талантливых людей, имеющих своеобразное хобби. Вопреки распространённому мнению, это не всегда программирование (ну или не только оно :D). На основе своего увлечения записью/обработкой музыки и профессиональной деятельности, я решил проверить качество кода популярных музыкальных программ с открытым исходным кодом. Первой для обзора выбрана программа для редактирования нот - MuseScore. Запасайтесь попкорном... серьёзных багов будет много!
MuseScore — компьютерная программа, редактор нотных партитур для операционных систем Windows, macOS X и Linux. MuseScore позволяет быстро вводить ноты как с клавиатуры компьютера, так и с внешней MIDI-клавиатуры. Поддерживается импорт и экспорт данных в форматах MIDI, MusicXML, LilyPond, а также импорт файлов в форматах MusE, Capella и Band-in-a-Box. Кроме того, программа может экспортировать партитуры в файлы PDF, SVG и PNG, либо в документы LilyPond для дальнейшей точной доработки партитуры.
PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в среде Windows и Linux.
V557 Array overrun is possible. The value of 'cidx' index could reach 4. staff.cpp 1029
ClefTypeList clefTypes[MAX_STAVES];
int staffLines[MAX_STAVES];
BracketType bracket[MAX_STAVES];
int bracketSpan[MAX_STAVES];
int barlineSpan[MAX_STAVES];
bool smallStaff[MAX_STAVES];
void Staff::init(...., const StaffType* staffType, int cidx)
{
if (cidx > MAX_STAVES) { // <=
setSmall(0, false);
}
else {
setSmall(0, t->smallStaff[cidx]);
setBracketType(0, t->bracket[cidx]);
setBracketSpan(0, t->bracketSpan[cidx]);
setBarLineSpan(t->barlineSpan[cidx]);
}
....
}Автор этого фрагмента кода допустил серьёзную ошибку, сравнивая индекс с максимальным размером массива. По этой причине стал возможен выход за пределы границ сразу четырёх массивов.
Исправленное условие проверки индекса:
if (cidx >= MAX_STAVES) {
setSmall(0, false);
}V557 Array overrun is possible. The value of 'i' index could reach 59. inspectorAmbitus.cpp 70
class NoteHead : public Symbol {
....
public:
enum class Group : signed char {
HEAD_NORMAL = 0,
HEAD_CROSS,
HEAD_PLUS,
....
HEAD_GROUPS, // <= 59
HEAD_INVALID = -1
};
....
}
InspectorAmbitus::InspectorAmbitus(QWidget* parent)
: InspectorElementBase(parent)
{
r.setupUi(addWidget());
s.setupUi(addWidget());
static const NoteHead::Group heads[] = {
NoteHead::Group::HEAD_NORMAL,
NoteHead::Group::HEAD_CROSS,
NoteHead::Group::HEAD_DIAMOND,
NoteHead::Group::HEAD_TRIANGLE_DOWN,
NoteHead::Group::HEAD_SLASH,
NoteHead::Group::HEAD_XCIRCLE,
NoteHead::Group::HEAD_DO,
NoteHead::Group::HEAD_RE,
NoteHead::Group::HEAD_MI,
NoteHead::Group::HEAD_FA,
NoteHead::Group::HEAD_SOL,
NoteHead::Group::HEAD_LA,
NoteHead::Group::HEAD_TI,
NoteHead::Group::HEAD_BREVIS_ALT
};
....
for (int i = 0; i < int(NoteHead::Group::HEAD_GROUPS); ++i)
r.noteHeadGroup->setItemData(i, int(heads[i]));//out of bound
....
}Вместо того, чтобы посчитать количество элементов массива, которые обходятся в цикле, здесь воспользовались константой, которая почти в четыре раза больше этого количества. В цикле происходит гарантированный выход за границу массива.
V501 There are identical sub-expressions to the left and to the right of the '-' operator: i - i text.cpp 1429
void Text::layout1()
{
....
for (int i = 0; i < rows(); ++i) {
TextBlock* t = &_layout[i];
t->layout(this);
const QRectF* r = &t->boundingRect();
if (r->height() == 0)
r = &_layout[i-i].boundingRect(); // <=
y += t->lineSpacing();
t->setY(y);
bb |= r->translated(0.0, y);
}
....
}Значение индекса [i - i], в данном случае, всегда будет равно нулю. Возможно, тут ошибка и хотели, к примеру, обращаться к предыдущему элементу массива.
С помощью статического анализа тоже можно находить утечки памяти и PVS-Studio это делает. Да, статические анализаторы в плане поиска утечек памяти более слабы, чем динамические, но всё равно они могут найти много интересного.
В незнакомом проекте мне сложно проверить достоверность всех найденных предупреждений, но в некоторых местах мне удалось убедиться в наличии ошибки.
V773 Visibility scope of the 'beam' pointer was exited without releasing the memory. A memory leak is possible. read114.cpp 2334
Score::FileError MasterScore::read114(XmlReader& e)
{
....
else if (tag == "Excerpt") {
if (MScore::noExcerpts)
e.skipCurrentElement();
else {
Excerpt* ex = new Excerpt(this);
ex->read(e);
_excerpts.append(ex);
}
}
else if (tag == "Beam") {
Beam* beam = new Beam(this);
beam->read(e);
beam->setParent(0);
// _beams.append(beam); // <=
}
....
}В большом каскаде условий выполняется аллокация памяти. В каждом блоке кода создаётся объект и сохраняется указатель на него. В приведённом фрагменте кода сохранение указателя закомментировали, добавив в код ошибку, приводящую к утечке памяти.
V773 The function was exited without releasing the 'voicePtr' pointer. A memory leak is possible. ove.cpp 3967
bool TrackParse::parse() {
....
Track* oveTrack = new Track();
....
QList<Voice*> voices;
for( i=0; i<8; ++i ) {
Voice* voicePtr = new Voice();
if( !jump(5) ) { return false; } // <=
// channel
if( !readBuffer(placeHolder, 1) ) { return false; } // <=
voicePtr->setChannel(placeHolder.toUnsignedInt());
// volume
if( !readBuffer(placeHolder, 1) ) { return false; } // <=
voicePtr->setVolume(placeHolder.toInt());
// pitch shift
if( !readBuffer(placeHolder, 1) ) { return false; } // <=
voicePtr->setPitchShift(placeHolder.toInt());
// pan
if( !readBuffer(placeHolder, 1) ) { return false; } // <=
voicePtr->setPan(placeHolder.toInt());
if( !jump(6) ) { return false; } // <=
// patch
if( !readBuffer(placeHolder, 1) ) { return false; } // <=
voicePtr->setPatch(placeHolder.toInt());
voices.push_back(voicePtr); //SAVE 1
}
// stem type
for( i=0; i<8; ++i ) {
if( !readBuffer(placeHolder, 1) ) { return false; } // <=
voices[i]->setStemType(placeHolder.toUnsignedInt());
oveTrack->addVoice(voices[i]); //SAVE 2
}
....
}Достаточно большой фрагмент, но в наличии ошибки легко убедиться. Каждый отмеченный оператор return приводит к потере указателя voicePtr. Если всё же программа выполняется до строки кода с комментарием "SAVE 2", то указатель сохраняется в класс Track. В деструкторе этого класса и будет выполнено освобождение указателей. В остальных случаях будет утечка памяти. Вот как выглядит реализация класса Track:
class Track{
....
QList<Voice*> voices_;
....
}
void Track::addVoice(Voice* voice) {
voices_.push_back(voice);
}
Track::~Track() {
clear();
}
void Track::clear(void) {
....
for(int i=0; i<voices_.size(); ++i){
delete voices_[i];
}
voices_.clear();
}Другие аналогичные предупреждения анализатора лучше посмотреть разработчикам проекта.
V614 Uninitialized variable 'pageWidth' used. Consider checking the third actual argument of the 'doCredits' function. importmxmlpass1.cpp 944
void MusicXMLParserPass1::scorePartwise()
{
....
int pageWidth;
int pageHeight;
while (_e.readNextStartElement()) {
if (_e.name() == "part")
part();
else if (_e.name() == "part-list") {
doCredits(_score, credits, pageWidth, pageHeight);// <= USE
partList(partGroupList);
}
....
else if (_e.name() == "defaults")
defaults(pageWidth, pageHeight); // <= INIT
....
}
....
}Такой код допускает использование неинициализированных переменных pageWidth и pageHeight в функции doCredits():
static
void doCredits(Score* score,const CreditWordsList& credits,
const int pageWidth, const int pageHeight)
{
....
const int pw1 = pageWidth / 3;
const int pw2 = pageWidth * 2 / 3;
const int ph2 = pageHeight / 2;
....
}Использование неинициализированных переменных приводит к неопределённому поведению, что долгое время может создавать видимость правильной работы программы.
V730 Not all members of a class are initialized inside the constructor. Consider inspecting: _dclickValue1, _dclickValue2. aslider.cpp 30
AbstractSlider::AbstractSlider(QWidget* parent)
: QWidget(parent), _scaleColor(Qt::darkGray),
_scaleValueColor(QColor("#2456aa"))
{
_id = 0;
_value = 0.5;
_minValue = 0.0;
_maxValue = 1.0;
_lineStep = 0.1;
_pageStep = 0.2;
_center = false;
_invert = false;
_scaleWidth = 4;
_log = false;
_useActualValue = false;
setFocusPolicy(Qt::StrongFocus);
}
double lineStep() const { return _lineStep; }
void setLineStep(double v) { _lineStep = v; }
double pageStep() const { return _pageStep; }
void setPageStep(double f) { _pageStep = f; }
double dclickValue1() const { return _dclickValue1; }
double dclickValue2() const { return _dclickValue2; }
void setDclickValue1(double val) { _dclickValue1 = val; }
void setDclickValue2(double val) { _dclickValue2 = val; }
....К неопределённому поведению может приводить использование неинициализированного поля класса. В этом классе большинство полей инициализируются в конструкторе и имеют методы для доступа к ним. А вот переменные _dclickValue1 и_dclickValue2 остаются не инициализированными, хотя имеют методы для чтения и записи. Если первым будет вызван метод для чтения, то он вернёт неопределённое значение. В коде проекта найдено около ста таких мест, и они заслуживают изучения разработчиками.
V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'adjustCanvasPosition' in derived class 'PianorollEditor' and base class 'MuseScoreView'. pianoroll.h 92
class MuseScoreView {
....
virtual void adjustCanvasPosition(const Element*,
bool /*playBack*/, int /*staffIdx*/ = 0) {};
....
}
class PianorollEditor : public QMainWindow, public MuseScoreView{
....
virtual void adjustCanvasPosition(const Element*, bool);
....
}
class ScoreView : public QWidget, public MuseScoreView {
....
virtual void adjustCanvasPosition(const Element* el,
bool playBack, int staff = -1) override;
....
}
class ExampleView : public QFrame, public MuseScoreView {
....
virtual void adjustCanvasPosition(const Element* el,
bool playBack);
....
}Анализатор нашёл целых три разных способа переопределить и перегрузить функцию adjustCanvasPosition() у базового класса MuseScoreView. Необходимо перепроверить код.
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1740, 1811. scoreview.cpp 1740
static void readNote(Note* note, XmlReader& e)
{
....
while (e.readNextStartElement()) {
const QStringRef& tag(e.name());
if (tag == "Accidental") {
....
}
....
else if (tag == "offTimeType") { // <= line 651
if (e.readElementText() == "offset")
note->setOffTimeType(2);
else
note->setOffTimeType(1);
}
....
else if (tag == "offTimeType") // <= line 728
e.skipCurrentElement(); // <= Dead code
....
}
....
}В очень большом каскаде условий присутствуют две одинаковые проверки. При такой ошибке либо не выполняются оба условия, либо выполнится только первое. Таким образом, второе условие никогда не выполняется и код остаётся недостижим.
Ещё два похожих места в коде:
Рассмотрим следующую ошибку.
V547 Expression 'middleMeasure != 0' is always false. ove.cpp 7852
bool
getMiddleUnit(...., Measure* middleMeasure, int& middleUnit) {
....
}
void OveOrganizer::organizeWedge(....) {
....
Measure* middleMeasure = NULL;
int middleUnit = 0;
getMiddleUnit(
ove_, part, track,
measure, ove_->getMeasure(bar2Index),
wedge->start()->getOffset(), wedge->stop()->getOffset(),
middleMeasure, middleUnit);
if( middleMeasure != 0 ) { // <=
WedgeEndPoint* midStopPoint = new WedgeEndPoint();
measureData->addMusicData(midStopPoint);
midStopPoint->setTick(wedge->getTick());
midStopPoint->start()->setOffset(middleUnit);
midStopPoint->setWedgeStart(false);
midStopPoint->setWedgeType(WedgeType::Cres_Line);
midStopPoint->setHeight(wedge->getHeight());
WedgeEndPoint* midStartPoint = new WedgeEndPoint();
measureData->addMusicData(midStartPoint);
midStartPoint->setTick(wedge->getTick());
midStartPoint->start()->setOffset(middleUnit);
midStartPoint->setWedgeStart(true);
midStartPoint->setWedgeType(WedgeType::Decresc_Line);
midStartPoint->setHeight(wedge->getHeight());
}
}
....
}Ещё очень большой фрагмент кода, который никогда не выполняется. Причиной является условие, которое всегда ложно. В условии сравнивается с нулём указатель, который изначально инициализирован нулём. При внимательном рассмотрении можно увидеть опечатку: перепутаны переменные middleMeasure и middleUnit. Обратите внимание на функцию getMiddleUnit(). По названию и последнему аргументу (передан по ссылке) видно, что модифицируется переменная middleUnit, которую и надо было проверять в условии.
V547 Expression 'error == 2' is always false. mididriver.cpp 126
#define ENOENT 2
bool AlsaMidiDriver::init()
{
int error = snd_seq_open(&alsaSeq, "hw", ....);
if (error < 0) {
if (error == ENOENT)
qDebug("open ALSA sequencer failed: %s",
snd_strerror(error));
return false;
}
....
}Очевидно, что после первой проверки переменная error всегда будет меньше нуля. Из-за дальнейшего сравнения переменной с числом 2 никогда не выводится отладочная информация.
V560 A part of conditional expression is always false: strack > - 1. edit.cpp 3669
void Score::undoAddElement(Element* element)
{
QList<Staff* > staffList;
Staff* ostaff = element->staff();
int strack = -1;
if (ostaff) {
if (ostaff->score()->excerpt() && strack > -1)
strack = ostaff->score()->excerpt()->tracks().key(...);
else
strack = ostaff->idx() * VOICES + element->track() % VOICES;
}
....
}Ещё один случай с ошибкой в условном выражении. Всегда выполняется код из else.
V779 Unreachable code detected. It is possible that an error is present. figuredbass.cpp 1377
bool FiguredBass::setProperty(P_ID propertyId, const QVariant& v)
{
score()->addRefresh(canvasBoundingRect());
switch(propertyId) {
default:
return Text::setProperty(propertyId, v);
}
score()->setLayoutAll();
return true;
}Диагностика V779 специализирована на поиске недостижимого кода, с ее помощью нашлось вот такое забавное место. И оно не одно в коде, есть ещё два:
V522 Dereferencing of the null pointer 'customDrumset' might take place. instrument.cpp 328
bool Instrument::readProperties(XmlReader& e, Part* part,
bool* customDrumset)
{
....
else if (tag == "Drum") {
// if we see on of this tags, a custom drumset will
// be created
if (!_drumset)
_drumset = new Drumset(*smDrumset);
if (!customDrumset) { // <=
const_cast<Drumset*>(_drumset)->clear();
*customDrumset = true; // <=
}
const_cast<Drumset*>(_drumset)->load(e);
}
....
}Тут допущена ошибка в условии. Скорее всего, автор хотел иначе проверить указатель customDrumset перед разыменованием, но сделал опечатку.
V522 Dereferencing of the null pointer 'segment' might take place. measure.cpp 2220
void Measure::read(XmlReader& e, int staffIdx)
{
Segment* segment = 0;
....
while (e.readNextStartElement()) {
const QStringRef& tag(e.name());
if (tag == "move")
e.initTick(e.readFraction().ticks() + tick());
....
else if (tag == "sysInitBarLineType") {
const QString& val(e.readElementText());
BarLine* barLine = new BarLine(score());
barLine->setTrack(e.track());
barLine->setBarLineType(val);
segment = getSegmentR(SegmentType::BeginBarLine, 0); //!!!
segment->add(barLine); // <= OK
}
....
else if (tag == "Segment")
segment->read(e); // <= ERR
....
}
....
}Это уже не первый большой каскад условий в этом проекте, где допускают ошибки. Стоит задуматься! Здесь указатель segment изначально равен нулю, а перед использованием инициализируется при разных условиях. В одной ветви это сделать забыли.
Ещё два опасных места:
V774 The 'slur' pointer was used after the memory was released. importgtp-gp6.cpp 2072
void GuitarPro6::readGpif(QByteArray* data)
{
if (c) {
slur->setTick2(c->tick());
score->addElement(slur);
legatos[slur->track()] = 0;
}
else {
delete slur;
legatos[slur->track()] = 0;
}
}Указатель slur используется уже после освобождения памяти с помощью оператора delete. Вероятно, тут перепутали строки местами.
V789 Iterators for the 'oldList' container, used in the range-based for loop, become invalid upon the call of the 'erase' function. layout.cpp 1760
void Score::createMMRest(....)
{
ElementList oldList = mmr->takeElements();
for (Element* ee : oldList) { // <=
if (ee->type() == e->type()) {
mmr->add(ee);
auto i = std::find(oldList.begin(), oldList.end(), ee);
if (i != oldList.end())
oldList.erase(i); // <=
found = true;
break;
}
}
....
}Анализатор обнаружил одновременное чтение и модификацию контейнера oldList в range-based for цикле. Такой код является ошибочным.
V765 A compound assignment expression 'x += x + ...' is suspicious. Consider inspecting it for a possible error. tremolo.cpp 321
void Tremolo::layout()
{
....
if (_chord1->up() != _chord2->up()) {
beamYOffset += beamYOffset + beamHalfLineWidth; // <=
}
else if (!_chord1->up() && !_chord2->up()) {
beamYOffset = -beamYOffset;
}
....
}Вот такой код нашёл анализатор. Указанное выражение равносильно такому:
beamYOffset = beamYOffset + beamYOffset + beamHalfLineWidth;Переменная beamYOffset складывается два раза. Возможно, это является ошибкой.
V674 The '-2.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'alter < - 2.5' expression. importmxmlpass2.cpp 5253
void MusicXMLParserPass2::pitch(int& step, int& alter ....)
{
....
alter = MxmlSupport::stringToInt(strAlter, &ok);
if (!ok || alter < -2.5 || alter > 2.5) {
logError(QString("invalid alter '%1'").arg(strAlter));
....
alter = 0;
}
....
}Переменная alter имеет целочисленный тип int. И сравнение с числами 2.5 и -2.5 выглядит очень странным.
V595 The 'sample' pointer was utilized before it was verified against nullptr. Check lines: 926, 929. voice.cpp 926
void Voice::update_param(int _gen)
{
....
if (gen[GEN_OVERRIDEROOTKEY].val > -1) {
root_pitch = gen[GEN_OVERRIDEROOTKEY].val * 100.0f - ....
}
else {
root_pitch = sample->origpitch * 100.0f - sample->pitchadj;
}
root_pitch = _fluid->ct2hz(root_pitch);
if (sample != 0)
root_pitch *= (float) _fluid->sample_rate / sample->samplerate;
break;
....
}Анализатор ругается на разыменование непроверенного указателя sample, когда ниже по коду проверка присутствует. Но что если указатель sample не планировали проверять в этой функции, а с нулём хотели сравнить переменную sample->samplerate перед делением? Тогда здесь имеет место серьёзная ошибка.
V523 The 'then' statement is equivalent to the 'else' statement. pluginCreator.cpp 84
PluginCreator::PluginCreator(QWidget* parent)
: QMainWindow(parent)
{
....
if (qApp->layoutDirection() == Qt::LayoutDirection::....) {
editTools->addAction(actionUndo);
editTools->addAction(actionRedo);
}
else {
editTools->addAction(actionUndo);
editTools->addAction(actionRedo);
}
....
}Анализатор обнаружил выполнение одинакового кода при разных условиях. Тут следует исправить ошибку, либо сократить код вдвое, убрав условие.
V524 It is odd that the body of 'downLine' function is fully equivalent to the body of 'upLine' function. rest.cpp 667
int Rest::upLine() const
{
qreal _spatium = spatium();
return lrint((pos().y() + bbox().top() +
_spatium) * 2 / _spatium);
}
int Rest::downLine() const
{
qreal _spatium = spatium();
return lrint((pos().y() + bbox().top() +
_spatium) * 2 / _spatium);
}Функции upLine() и downLine() имеют противоположные по смыслу названия, но при этом реализованы одинаково. Стоит проверить это подозрительное место.
V766 An item with the same key '"mrcs"' has already been added. importgtp-gp6.cpp 100
const static std::map<QString, QString> instrumentMapping = {
....
{"e-piano-gs", "electric-piano"},
{"e-piano-ss", "electric-piano"},
{"hrpch-gs", "harpsichord"},
{"hrpch-ss", "harpsichord"},
{"mrcs", "maracas"}, // <=
{"mrcs", "oboe"}, // <=
{"mrcs", "oboe"}, // <= явный Copy-Paste
....
};Похоже автор этого фрагмента кода торопился и насоздавал пар с одинаковыми ключами, но разными значениями.
V1001 The 'ontime' variable is assigned but is not used until the end of the function. rendermidi.cpp 1176
bool renderNoteArticulation(....)
{
int ontime = 0;
....
// render the suffix
for (int j = 0; j < s; j++)
ontime = makeEvent(suffix[j], ontime, tieForward(j,suffix));
// render graceNotesAfter
ontime = graceExtend(note->pitch(), ...., ontime);
return true;
}В коде модифицируется переменная ontime, но при этом не используется при выходе из функции. Возможно, тут допущена ошибка.
V547 Expression 'runState == 0' is always false. pulseaudio.cpp 206
class PulseAudio : public Driver {
Transport state;
int runState; // <=
....
}
bool PulseAudio::stop()
{
if (runState == 2) {
runState = 1;
int i = 0;
for (;i < 4; ++i) {
if (runState == 0) // <=
break;
sleep(1);
}
pthread_cancel(thread);
pthread_join(thread, 0);
}
return true;
}Анализатор обнаружил всегда ложное условие, но функция stop() вызывается в параллельном коде и срабатывания быть не должно. Причина появления предупреждения заключается в том, что автор кода воспользовался для синхронизации простой переменой типа int, являющейся полем класса. А это приводит к ошибкам синхронизации. После исправления кода, диагностика V547 перестанет выдавать ложное срабатывание, т.е. в ней сработает исключение на тему параллельного кода.
В небольшом проекте нашлось много разных ошибок. Надеемся, авторы программы обратят внимание на мой обзор и проведут исправительные работы. Я проверю код ещё нескольких программ, которыми пользуюсь. Если вы знаете интересный софт для работы с музыкой и хотите увидеть его в обзоре, то присылайте названия мне на почту.
Другие обзоры музыкального софта:
Попробовать анализатор PVS-Studio на своём проекте очень легко, достаточно перейти на страницу загрузки.