work on multiple possible causes of crashes or audible artefacts
authormirabilos <tg@debian.org>
Sun, 14 Mar 2021 18:29:49 +0000 (19:29 +0100)
committermirabilos <mirabilos@evolvis.org>
Sun, 14 Mar 2021 18:29:49 +0000 (19:29 +0100)
debian/changelog
debian/patches/experiments/valid-soundfont.diff [new file with mode: 0644]
debian/patches/series

index d99593a..8201a66 100644 (file)
@@ -1,3 +1,9 @@
+musescore2 (2.3.2+dfsg4-15) unstable; urgency=medium
+
+  * Fix soundfont-related crashes or audible artefacts (Closes: #985129)
+
+ -- Thorsten Glaser <tg@mirbsd.de>  Sun, 14 Mar 2021 18:17:41 +0100
+
 musescore2 (2.3.2+dfsg4-14) unstable; urgency=medium
 
   * Fix possible error cause in m-common.prerm (Closes: #984592)
diff --git a/debian/patches/experiments/valid-soundfont.diff b/debian/patches/experiments/valid-soundfont.diff
new file mode 100644 (file)
index 0000000..1d607d3
--- /dev/null
@@ -0,0 +1,245 @@
+Description: Fix multiple possible causes of crashes or audible artefacts
+ - Track sample name so we can issue proper warning messages, show filename
+ - On read errors, issue an error message and mark sample as invalid
+ - Mark sample as invalid if Ogg Vorbis decompression (SF3) fails
+ - Do all sanity checks on {,loop}{start,end} with SF2 semantics for end;
+   only switch end to point to the last sample afterwards in only one place
+ - Add sanity check provided by the SoundFont spec as extra warning
+ - Do not crash if there is no data[]
+ - Issue diagnostics if disabling a sample
+ - Swap two members to improve structure packing/alignment while there
+ - Use unsigned integers for SoundFont element sizes properly
+Author: mirabilos <m@mirbsd.org>
+Bug: https://musescore.org/en/node/89216 (and probably others)
+Bug-Debian: https://bugs.debian.org/985129
+Forwarded: tbd
+
+--- a/fluid/sfont.cpp
++++ b/fluid/sfont.cpp
+@@ -589,6 +589,7 @@ Sample::Sample(SFont* s)
+       data        = 0;
+       amplitude_that_reaches_noise_floor_is_valid = false;
+       amplitude_that_reaches_noise_floor = 0.0;
++      name[0]     = 0;
+       }
+ //---------------------------------------------------------
+@@ -625,11 +626,12 @@ void Sample::load()
+ #ifdef SOUNDFONT3
+             char* p = new char[size];
+             if (fd.read(p, size) != size) {
+-                  printf("  read %d failed\n", size);
+                   delete[] p;
++                  qWarning("SoundFont(%s) Sample(%s) read %u bytes failed", qPrintable(sf->get_name()), name, size);
++                  setValid(false);
+                   return;
+                   }
+-            decompressOggVorbis(p, size);
++            setValid(decompressOggVorbis(p, size));
+             delete[] p;
+ #endif
+             }
+@@ -637,8 +639,11 @@ void Sample::load()
+             data = new short[size];
+             size *= sizeof(short);
+-            if (fd.read((char*)data, size) != size)
++            if (fd.read((char*)data, size) != size) {
++                  qWarning("SoundFont(%s) Sample(%s) read %u bytes failed", qPrintable(sf->get_name()), name, size);
++                  setValid(false);
+                   return;
++                  }
+             if (QSysInfo::ByteOrder == QSysInfo::BigEndian) {
+                   unsigned char hi, lo;
+@@ -652,11 +657,34 @@ void Sample::load()
+                         data[i] = s;
+                         }
+                   }
+-            end       -= (start + 1);       // marks last sample, contrary to SF spec.
++            end       -= start;
+             loopstart -= start;
+             loopend   -= start;
+             start      = 0;
+             }
++
++      // sanity checks:
++      // - start < end in SFont::load_shdr(int)
++      // - start < loopstart < loopend < end for !SF3 ibidem
++      // - … for SF3 in Sample::decompressOggVorbis(char *, unsigned int) in sfont3.cpp
++      // - most importantly, they are done *before* start is normalised to 0, which it is at this point
++      //
++      // now add some extra sanity checks from the SF2 spec (biased so they work with unsigned int);
++      // just a warning, they probably work with Fluid, possibly with audible artefacts though...
++      if (!(start + 7 < loopstart) || !(loopstart + 31 < loopend) || !(loopend + 7 < end))
++            qWarning("SoundFont(%s) Sample(%s) start(%u) startloop(%u) endloop(%u) end(%u) smaller than SoundFont 2.04 spec chapter 7.10 recommendation",
++                     qPrintable(sf->get_name()), name, start, loopstart, loopend, end);
++
++      // this used to cause a crash
++      if (!data) {
++            qWarning("SoundFont(%s) Sample(%s) data is nil", qPrintable(sf->get_name()), name);
++            setValid(false);
++            }
++
++      // from here, end marks the last sample, not one past as in the SF2 spec
++      if (end > 0)
++            end -= 1;
++
+       optimize();
+       }
+@@ -725,6 +753,8 @@ bool SFont::load()
+             }
+       SFChunk chunk;
++      // so that any subsequent errors can be attributed to the right file
++      qDebug("Loading soundfont: %s", qPrintable(f.fileName()));
+       try {
+             readchunk(&chunk);
+             if (chunkid(chunk.id) != RIFF_ID)
+@@ -1599,9 +1629,7 @@ void SFont::load_shdr (int size)
+       for (int i = 0; i < size; i++) {
+             Sample* p = new Sample(this);
+             sample.append(p);
+-            char buffer[21];
+-            READSTR (buffer);
+-            // READSTR (p->name);
++            READSTR (p->name);
+             READD (p->start);
+             READD (p->end);         /* - end, loopstart and loopend */
+             READD (p->loopstart);     /* - will be checked and turned into */
+@@ -1616,16 +1644,19 @@ void SFont::load_shdr (int size)
+                   continue;
+                   }
+             if ((p->end > getSamplesize()) || (p->start > (p->end - 4))) {
+-                  qWarning("Sample start/end file positions are invalid, disabling");
++                  qWarning("Sample(%s) start/end file positions are invalid, disabling", p->name);
+                   p->setValid(false);
+                   continue;
+                   }
+             p->setValid(true);
+             if (p->sampletype & FLUID_SAMPLETYPE_OGG_VORBIS) {
++                  // done in Sample::decompressOggVorbis in sfont3.cpp
+                   }
+             else {
+                   // loop is fowled?? (cluck cluck :)
+                   if (p->loopend > p->end || p->loopstart >= p->loopend || p->loopstart <= p->start) {
++                        qWarning("Sample(%s) start(%u) startloop(%u) endloop(%u) end(%u) fowled (broken soundfont?), fixing up",
++                                 p->name, p->start, p->loopstart, p->loopend, p->end);
+                         /* can pad loop by 8 samples and ensure at least 4 for loop (2*8+4) */
+                         if ((p->end - p->start) >= 20) {
+                               p->loopstart = p->start + 8;
+@@ -1636,8 +1667,10 @@ void SFont::load_shdr (int size)
+                               p->loopend   = p->end - 1;
+                               }
+                         }
+-                  if ((p->end - p->start) < 8)
++                  if ((p->end - p->start) < 8) {
++                        qWarning("Sample(%s) too small, disabling", p->name);
+                         p->setValid(false);
++                        }
+                   }
+             }
+       FSKIP (SFSHDRSIZE);     /* skip terminal shdr */
+--- a/fluid/sfont.h
++++ b/fluid/sfont.h
+@@ -147,8 +147,10 @@ class Sample {
+           filled out automatically */
+       /* Set this to zero, when submitting a new sample. */
+-      bool amplitude_that_reaches_noise_floor_is_valid;
+       double amplitude_that_reaches_noise_floor;
++      bool amplitude_that_reaches_noise_floor_is_valid;
++
++      char name[21];
+       Sample(SFont*);
+       ~Sample();
+@@ -159,7 +161,7 @@ class Sample {
+       bool valid() const    { return _valid; }
+       void setValid(bool v) { _valid = v; }
+ #ifdef SOUNDFONT3
+-      bool decompressOggVorbis(char* p, int size);
++      bool decompressOggVorbis(char* p, unsigned int size);
+ #endif
+       };
+--- a/fluid/sfont3.cpp
++++ b/fluid/sfont3.cpp
+@@ -10,7 +10,7 @@ namespace FluidS {
+ //   decompressOggVorbis
+ //---------------------------------------------------------
+-bool Sample::decompressOggVorbis(char* src, int size)
++bool Sample::decompressOggVorbis(char* src, unsigned int size)
+       {
+       AudioFile af;
+       QByteArray ba(src, size);
+@@ -18,32 +18,37 @@ bool Sample::decompressOggVorbis(char* s
+       start = 0;
+       end   = 0;
+       if (!af.open(ba)) {
+-            qDebug("Sample::decompressOggVorbis: open failed: %s", af.error());
++            qDebug("SoundFont(%s) Sample(%s) decompressOggVorbis: open failed: %s", qPrintable(sf->get_name()), name, af.error());
+             return false;
+             }
+-      int frames = af.frames();
++      unsigned int frames = af.frames();
+       data = new short[frames * af.channels()];
+-      if (frames != af.readData(data, frames)) {
+-            qDebug("Sample read failed: %s", af.error());
++      if (frames != (unsigned int)af.readData(data, frames)) {
++            qDebug("SoundFont(%s) Sample(%s) read failed: %s", qPrintable(sf->get_name()), name, af.error());
+             delete[] data;
+             data = 0;
++            return false;
+             }
+-      end = frames - 1;
++      // cf. https://musescore.org/en/node/89216#comment-1068379 and following
++      end = frames;
+-      if (loopend > end ||loopstart >= loopend || loopstart <= start) {
++      // loop is fowled?? (cluck cluck :)
++      if (loopend > end || loopstart >= loopend || loopstart <= start) {
++            qWarning("SoundFont(%s) Sample(%s) start(%u) startloop(%u) endloop(%u) end(%u) fowled (broken soundfont?), fixing up",
++                     qPrintable(sf->get_name()), name, start, loopstart, loopend, end);
+             /* can pad loop by 8 samples and ensure at least 4 for loop (2*8+4) */
+             if ((end - start) >= 20) {
+                   loopstart = start + 8;
+-                  loopend = end - 8;
++                  loopend   = end - 8;
+                   }
+-            else { // loop is fowled, sample is tiny (can't pad 8 samples)
++            else {      // loop is fowled, sample is tiny (can't pad 8 samples)
+                   loopstart = start + 1;
+-                  loopend = end - 1;
++                  loopend   = end - 1;
+                   }
+             }
+       if ((end - start) < 8) {
+-            qDebug("invalid sample");
+-            setValid(false);
++            qWarning("SoundFont(%s) Sample(%s) too small, disabling", qPrintable(sf->get_name()), name);
++            return false;
+             }
+       return true;
+--- a/fluid/voice.cpp
++++ b/fluid/voice.cpp
+@@ -1795,7 +1795,6 @@ void Sample::optimize()
+       signed short peak;
+       float normalized_amplitude_during_loop;
+       double result;
+-      int i;
+       /* ignore ROM and other(?) invalid samples */
+       if (!s->valid())
+@@ -1803,7 +1802,7 @@ void Sample::optimize()
+       if (!s->amplitude_that_reaches_noise_floor_is_valid) { /* Only once */
+             /* Scan the loop */
+-            for (i = (int)s->loopstart; i < (int) s->loopend; i ++) {
++            for (size_t i = s->loopstart; i < s->loopend; i++) {
+                   signed short val = s->data[i];
+                   if (val > peak_max)
+                         peak_max = val;
index 32e471e..700cf24 100644 (file)
@@ -30,6 +30,7 @@ upstream/fix-accidental-paren-pos.diff
 upstream/statusline-pitch-onofftime.diff
 upstream/improve-mscore-font.diff
 upstream/5507.patch
+experiments/valid-soundfont.diff
 debian-specific/fixup-AppData.diff
 experiments/upstream-backend-fixes.diff
 experiments/lyrics-hyphen-5sp.diff