プログラミングは創造的な活動であるため、開発者の中には、趣味のある才能のある人がたくさんいます。 一般的な信念に反して、これは常にプログラミングではありません（まあ、またはそれだけではありません：D）。 音楽の録音/編集および専門的な活動に対する情熱に基づいて、人気のあるオープンソース音楽プログラムのコードの品質を確認することにしました。 ノートを編集するために選択したプログラムを最初にレビューするのはMuseScoreです。 ポップコーンを買いだめしてください...多くの深刻なバグがあります！
はじめに
MuseScoreは、Windows、Mac OS X、Linuxオペレーティングシステム用のコンピュータープログラム、楽譜エディターです。 MuseScoreを使用すると、コンピューターのキーボードと外部MIDIキーボードの両方からすばやくメモを入力できます。 MIDI、MusicXML、LilyPond形式のデータのインポートとエクスポート、およびMusE、Capella、Band-in-a-Box形式のファイルのインポートをサポートしています。 さらに、プログラムはスコアをPDF、SVG、PNGファイル、またはLilyPondドキュメントにエクスポートして、スコアをさらに微調整できます。
PVS-Studioは、C、C ++、C＃で記述されたプログラムのソースコードのエラーを検出するためのツールです。 WindowsおよびLinuxで動作します。
配列のインデックス作成の問題
V557配列のオーバーランが可能です。 'cidx'インデックスの値は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]); } .... }
このコードフラグメントの作成者は、インデックスを配列の最大サイズと比較するときに重大な間違いを犯しました。 このため、4つのアレイの境界を一度に超えることが可能になりました。
修正されたインデックスチェック条件：
if (cidx >= MAX_STAVES) { setSmall(0, false); }
V557配列のオーバーランが可能です。 「i」インデックスの値は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 .... }
ループでバイパスされる配列要素の数を数える代わりに、そのほぼ4倍の定数を使用しました。 ループでは、配列の境界を超える保証された終了が発生します。
V501 「-」演算子の左右に同じ副次式があります。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[ii].boundingRect(); // <= y += t->lineSpacing(); t->setY(y); bb |= r->translated(0.0, y); } .... }
この場合、インデックス値[i-i]は常にゼロになります。 おそらく間違いがあり、たとえば配列の前の要素を参照したかったのでしょう。
メモリリーク
静的分析はメモリリークも検出できますが、PVS-Studioはこれを行います。 はい、静的アナライザーは動的リークよりもメモリリークの検索に関しては弱いですが、それでも多くの興味深いものを見つけることができます。
なじみのないプロジェクトでは、見つかったすべての警告の信頼性を確認することは困難ですが、一部の場所ではエラーが発生したことを確認できました。
V773 「ビーム」ポインターの可視性スコープは、メモリーを解放せずに終了しました。 メモリリークが発生する可能性があります。 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 「voicePtr」ポインターを解放せずに関数が終了しました。 メモリリークが発生する可能性があります。 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初期化されていない変数「pageWidth」が使用されました。 「doCredits」関数の3番目の実引数を確認することを検討してください。 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 .... } .... }
このようなコードにより、 doCredits（）関数で初期化されていない変数pageWidthおよびpageHeightを使用できます。
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クラスのすべてのメンバーがコンストラクター内で初期化されるわけではありません。 検査を検討してください：_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は、読み取りと書き込みのメソッドを持っていますが、初期化されていません。 readメソッドが最初に呼び出された場合、未定義の値を返します。 約100のそのような場所がプロジェクトコードで見つかったため、開発者が調査するに値します。
継承エラー
V762仮想機能が誤ってオーバーライドされた可能性があります。 派生クラス「PianorollEditor」および基本クラス「MuseScoreView」の関数「adjustCanvasPosition」の3番目の引数を参照してください。 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); .... }
アナライザーは、 MuseScoreView基本クラスのadjustCanvasPosition（）関数をオーバーライドおよびオーバーロードする3つの異なる方法を見つけました。 コードを再確認する必要があります。
到達不能コード
V517 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認してください：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 .... } .... }
非常に大規模な条件のカスケードでは、2つの同一のチェックがあります。 このようなエラーでは、両方の条件が満たされていないか、最初の条件のみが満たされています。 したがって、2番目の条件が満たされることはなく、コードは到達不能のままです。
コード内の2つの類似した場所：
- V517「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認してください：645、726。read114.cpp 645
- V517「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認してください：1740、1811。scoreview.cpp 1740
次のエラーを考慮してください。
V547式 'middleMeasure！= 0'は常に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式 'error == 2'は常に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; } .... }
明らかに、最初のチェックの後、 エラー変数は常にゼロ未満になります。 変数と数値2をさらに比較するため、デバッグ情報は表示されません。
V560条件式の一部は常に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; } .... }
条件式にエラーがある別のケース。 他のコードは常に実行されます。
V779到達不能コードが検出されました。 エラーが存在する可能性があります。 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診断は、到達不能なコードを見つけることに特化しており、その助けを借りてこのような面白い場所がありました。 また、コードには1つではなく、さらに2つあります。
- V779到達不能コードが検出されました。 エラーが存在する可能性があります。 fingering.cpp 165
- V779到達不能コードが検出されました。 エラーが存在する可能性があります。 chordrest.cpp 1127
無効なポインター/イテレーター
V522 nullポインター「customDrumset」の逆参照が行われる場合があります。 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ヌルポインター「セグメント」の逆参照が行われる場合があります。 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 .... } .... }
これは、このプロジェクトでミスが発生する最初の大きなカスケードではありません。 検討する価値があります！ ここで、 セグメントポインターは最初はゼロで、使用前にさまざまな条件下で初期化されます。 あるブランチでは、彼らはそれをするのを忘れていました。
さらに2つの危険な場所：
- V522ヌルポインター「セグメント」の逆参照が行われる場合があります。 read114.cpp 1551
- V522ヌルポインター「セグメント」の逆参照が行われる場合があります。 read206.cpp 1879
V774メモリーが解放された後、「スラー」ポインターが使用されました。 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; } }
スラーポインターは 、 削除演算子を使用してメモリを解放した後に使用されます。 おそらく、ここで線が混同されたでしょう。
範囲ベースのforループで使用される「oldList」コンテナのV789イテレータは、「消去」関数の呼び出し時に無効になります。 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; } } .... }
アナライザーは、範囲ベースのforループでoldListコンテナーの同時読み取りと変更を検出しました。 そのようなコードは誤りです。
算術エラー
V765複合代入式「x + = x + ...」は疑わしいです。 エラーの可能性を調べることを検討してください。 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変数が 2回追加されます。 おそらくこれは間違いです。
V674 「double」タイプの「-2.5」リテラルは、「int」タイプの値と比較されます。 「alter <-2.5」式の検査を検討してください。 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 nullptrに対して検証される前に、「サンプル」ポインターが使用されました。 行を確認：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-> samplerateを除算前のゼロと比較したい場合はどうでしょうか。 次に、重大な間違いがあります。
雑多
V523 「then」ステートメントは「else」ステートメントと同等です。 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 「downLine」関数の本体が「upLine」関数の本体と完全に同等であることは奇妙です。 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同じキー「 "mrcs"」を持つアイテムがすでに追加されています。 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 「ontime」変数が割り当てられていますが、関数の最後まで使用されません。 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式 'runState == 0'は常に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アナライザーを試すのは非常に簡単です。 ダウンロードページに進んでください。
英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 音楽ソフトウェアのコードの欠陥のレビューパート1. MuseScore
記事を読んで質問がありますか？
多くの場合、記事には同じ質問が寄せられます。 ここで回答を収集しました： PVS-Studioバージョン2015に関する記事の読者からの質問への回答 。 リストをご覧ください。