Crash on corrupt MXL import

• May 29, 2021 - 16:29
Reported version
3.6
Type
Functional
Frequency
Once
Severity
S2 - Critical
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project
MuseScore

I run a self-built MuseScore, currently from commit #84f6aa76ca2a9a3b41b5114b1715ab23b09f2887 in the 3.x branch. I came across a MusicML file (produced by Audiveris) that is flagged as corrupt on import. Since those messages can usually be ignored for native project files I decided to ignore the warning and import the file anyway as suggested by the dialog.

A crash ensued, AFAICT as a result of a vulgar nullptr derefence. Fixed as in the patch below the file imports to an empty score - vastly preferable over a crash.

diff --git a/importexport/musicxml/importmxmlpass2.cpp b/importexport/musicxml/importmxmlpass2.cpp
index 35699718..125769af 100644
--- a/importexport/musicxml/importmxmlpass2.cpp
+++ b/importexport/musicxml/importmxmlpass2.cpp
@@ -1579,8 +1579,9 @@ void MusicXMLParserPass2::scorePartwise()
             }
       // set last measure barline to normal or MuseScore will generate light-heavy EndBarline
       // TODO, handle other tracks?
-      if (_score->lastMeasure()->endBarLineType() == BarLineType::NORMAL)
-            _score->lastMeasure()->setEndBarLineType(BarLineType::NORMAL, 0);
+      auto lm = _score->lastMeasure();
+      if (lm && lm->endBarLineType() == BarLineType::NORMAL)
+            lm->setEndBarLineType(BarLineType::NORMAL, 0);
       }
 
 //---------------------------------------------------------

The mxl file: JustinJohnson-BlackwoodLullaby-notab3-p4-crash.mxl

Comments

• May 29, 2021 - 16:43
Frequency ⇒ Once

(if you enclose the code in <cpp>...</cpp> it renders better):

diff --git a/importexport/musicxml/importmxmlpass2.cpp b/importexport/musicxml/importmxmlpass2.cpp
index 1770dad49..99d4010a2 100644
--- a/importexport/musicxml/importmxmlpass2.cpp
+++ b/importexport/musicxml/importmxmlpass2.cpp
@@ -1579,8 +1579,9 @@ void MusicXMLParserPass2::scorePartwise()
             }
       // set last measure barline to normal or MuseScore will generate light-heavy EndBarline
       // TODO, handle other tracks?
-      if (_score->lastMeasure()->endBarLineType() == BarLineType::NORMAL)
-            _score->lastMeasure()->setEndBarLineType(BarLineType::NORMAL, 0);
+      auto lm = _score->lastMeasure();
+      if (lm && lm->endBarLineType() == BarLineType::NORMAL)
+            lm->setEndBarLineType(BarLineType::NORMAL, 0);
       }
 
 //---------------------------------------------------------

I'm not sure whether an empty score really is an improvement though?

Reply

• May 29, 2021 - 16:46

OTOH:
Debug: importMusicXml() file '.../JustinJohnson-BlackwoodLullaby-notab3-p4-crash.mxl' is not a valid MusicXML file (...\importexport\musicxml\importxml.cpp:203, Ms::Score::FileError Ms::doValidate(const QString&, QIODevice*))
Debug: empty score (...\libmscore\layout.cpp:4899, void Ms::Score::doLayoutRange(const Ms::Fraction&, const Ms::Fraction&))

Reply

• May 29, 2021 - 16:47

and looking at the xml:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 3.0.1 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">
<score-partwise version="3.0.1">
  <identification>
    <encoding>
      <software>Audiveris 5.1.1</software>
      <supports type="yes" element="print" attribute="new-system" value="yes"></supports>
      <supports type="yes" element="print" attribute="new-page" value="yes"></supports>
      <software>ProxyMusic 3.0.1.8e1a2c5</software>
      <encoding-date>2021-05-27</encoding-date>
    </encoding>
    <source>/Volumes/Debian/Users/bertin/WinBertin/work/doc/Perso/Music/Partitions/Guitar/JustinJohnson-BlackwoodLullaby-notab2.tiff</source>
    <miscellaneous>
      <miscellaneous-field name="source-file">/Volumes/Debian/Users/bertin/WinBertin/work/doc/Perso/Music/Partitions/Guitar/JustinJohnson-BlackwoodLullaby-notab2.tiff</miscellaneous-field>
      <miscellaneous-field name="source-sheet-4">1 2 3 4</miscellaneous-field>
    </miscellaneous>
  </identification>
  <defaults>
    <scaling>
      <millimeters>9.4827</millimeters>
      <tenths>40</tenths>
    </scaling>
    <page-layout>
      <page-height>1670</page-height>
      <page-width>1180</page-width>
      <page-margins type="both">
        <left-margin>80</left-margin>
        <right-margin>80</right-margin>
        <top-margin>80</top-margin>
        <bottom-margin>80</bottom-margin>
      </page-margins>
    </page-layout>
    <lyric-font font-family="Default" font-size="10"></lyric-font>
  </defaults>
  <part-list/>
</score-partwise>

That score really is empty, so yes, this is an improvement!

Reply

• May 29, 2021 - 17:24

>Are you going to PR it?
>Else I'd do it, just let me know

I was hoping one of the devs would do it indeed, given how simple the change is (and based on another nullptr check on the same property in the same file).

Re: is it an improvement aside from the fact that this was really an empty score; when I import something that is flagged as corrupt or invalid I expect anything up to a definitive failure message, but not really a hard crash.

BTW, is the warning dialog appropriate if this is really an empty score?

Reply

Fix version
4.0.0