Skip to content

Commit

Permalink
Fix #89216: Multiple possible causes of crashes or audible artefacts
Browse files Browse the repository at this point in the history 
Duplicate of musescore#7728

 - 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

Fixes https://bugs.debian.org/985129 and
https://musescore.org/en/node/89216 and probably others
  • Loading branch information
@mirabilos @Jojo-Schmitz
mirabilos authored and Jojo-Schmitz committed Aug 19, 2021
1 parent 8151a57 commit c72ae21
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 31 deletions.
51 changes: 42 additions & 9 deletions audio/midi/fluid/sfont.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,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;
}

//---------------------------------------------------------
Expand Down Expand Up @@ -651,18 +652,22 @@ void Sample::load()
std::vector<char> p;
p.resize(size);
if (fd.read(p.data(), size) != size) {
qDebug("read %d failed", size);
qWarning("SoundFont(%s) Sample(%s) read %u bytes failed", qPrintable(sf->get_name()), name, size);
setValid(false);
return;
}
decompressOggVorbis(p.data(), size);
setValid(decompressOggVorbis(p.data(), size));
#endif
}
else {
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;
Expand All @@ -676,11 +681,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();
}

Expand Down Expand Up @@ -749,6 +777,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)
Expand Down Expand Up @@ -1628,9 +1658,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 */
Expand All @@ -1645,16 +1673,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;
Expand All @@ -1665,8 +1696,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 */
Expand Down
6 changes: 4 additions & 2 deletions audio/midi/fluid/sfont.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,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();
Expand All @@ -162,7 +164,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
};

Expand Down
29 changes: 17 additions & 12 deletions audio/midi/fluid/sfont3.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,40 +10,45 @@ namespace FluidS {
// decompressOggVorbis
//---------------------------------------------------------

bool Sample::decompressOggVorbis(char* src, int size)
bool Sample::decompressOggVorbis(char* src, unsigned int size)
{
AudioFile af;
QByteArray ba(src, size);

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;
Expand Down
8 changes: 0 additions & 8 deletions audio/midi/fluid/voice.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1845,14 +1845,6 @@ void Sample::optimize()
if (!s->valid())
return;

IF_ASSERT_FAILED(s->loopstart >= s->start) {
s->loopstart = s->start;
}

if (s->loopend > s->end) {
s->loopend = s->end;
}

if (!s->amplitude_that_reaches_noise_floor_is_valid) { /* Only once */
/* Scan the loop */
for (size_t i = s->loopstart; i < s->loopend; i++) {
Expand Down

0 comments on commit c72ae21

Please sign in to comment.