Use of uninitialised memory in Ms::TextLineBaseSegment::layout() (textlinebase.cpp:235)

• Nov 28, 2018 - 01:28
Reported version
3.0
Priority
P2 - Medium
Type
Development
Frequency
Once
Severity
S4 - Minor
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project
MuseScore

Valgrind reports:

==21024== Conditional jump or move depends on uninitialised value(s)
==21024== at 0xBA6043: Ms::TextLineBaseSegment::layout() (textlinebase.cpp:235)
==21024== by 0xAB846E: Ms::HairpinSegment::layout() (hairpin.cpp:115)
==21024== by 0xAF2AE3: Ms::SLine::layoutSystem(Ms::System) (line.cpp:816)
==21024== by 0xAE0937: Ms::processLines(Ms::System, std::vector >, bool) (layout.cpp:2939)
==21024== by 0xAEBFF4: Ms::Score::collectSystem(Ms::LayoutContext&) (layout.cpp:3378)
==21024== by 0xAEDB36: Ms::LayoutContext::collectPage() (layout.cpp:3586)
==21024== by 0xAEEC19: Ms::LayoutContext::layout() (layout.cpp:3875)
==21024== by 0xAEF564: Ms::Score::doLayoutRange(int, int) (layout.cpp:3861)
==21024== by 0xBDE8D6: Ms::Score::update() (cmd.cpp:222)
==21024== by 0x812C71: Ms::readScore(Ms::MasterScore*, QString, bool) (file.cpp:2330)
==21024== by 0x81334C: Ms::MuseScore::readScore(QString const&) (file.cpp:346)
==21024== by 0x680D63: Ms::convert(QString const&, QJsonArray const&, QString const&) (musescore.cpp:3389)

This is the following line:

232 qreal l = 0.0;
233 if (!_text->empty()) {
234 qreal textlineTextDistance = _spatium * .5;
235 if (((isSingleType() || isBeginType()) && (tl->beginTextPlace() == PlaceText::LEFT)) || ((isMiddleType() || isEndType()) && (tl->continueTextPlace() == PlaceText::LEFT)))
236 l = _text->pos().x() + _text->bbox().width() + textlineTextDistance;
237 qreal h = _text->height();

This happens when batch-converting some MuseScore 2.x-saved scores.

Comments

• Dec 1, 2018 - 22:23

Pretty sure this is either tl->beginTextPlace() or tl->continueTextPlace() (actually probably both) because those isSomethingType() macros all check spannerSegmentType(), and at least one of them was called earlier in the code.

I can’t figure out where this needs to be initialised, though.

Reply

• Dec 5, 2018 - 20:29

Unfortunately, valgrind and dr.memory both crash as soon as I start MuseScore on my pc, and memory sanitizer gives tons of false positives (because I did not recompile licxx with -fsanitize=memory) so I can't reproduce.
@mirabilos : do you have the output for such an error when running valgrind with "--track-origins=yes"? In principle it should show where in the code the uninitialised variable was allocated.

Reply

• Dec 5, 2018 - 21:48

Does this help?

==30306== Memcheck, a memory error detector
==30306== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==30306== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==30306== Command: mscore-snapshot -j t2.jsn
==30306==
==30306== Conditional jump or move depends on uninitialised value(s)
==30306==    at 0xBA36FB: Ms::TextLineBaseSegment::layout() (textlinebase.cpp:235)
==30306==    by 0xAB425E: Ms::HairpinSegment::layout() (hairpin.cpp:123)
==30306==    by 0xAEEE13: Ms::SLine::layoutSystem(Ms::System*) (line.cpp:816)
==30306==    by 0xADC8A7: Ms::processLines(Ms::System*, std::vector >, bool) (layout.cpp:2954)
==30306==    by 0xAE8234: Ms::Score::collectSystem(Ms::LayoutContext&) (layout.cpp:3428)
==30306==    by 0xAE9EA6: Ms::LayoutContext::collectPage() (layout.cpp:3636)
==30306==    by 0xAEAF79: Ms::LayoutContext::layout() (layout.cpp:3924)
==30306==    by 0xAEB894: Ms::Score::doLayoutRange(int, int) (layout.cpp:3910)
==30306==    by 0xC440EC: Ms::MasterScore::read206(Ms::XmlReader&) (read206.cpp:3804)
==30306==    by 0xBE7304: Ms::MasterScore::read1(Ms::XmlReader&, bool) (scorefile.cpp:958)
==30306==    by 0xBEA7B2: Ms::MasterScore::loadMsc(QString, QIODevice*, bool) (scorefile.cpp:876)
==30306==    by 0xBEA864: Ms::MasterScore::loadMsc(QString, bool) (scorefile.cpp:864)
==30306==  Uninitialised value was created by a heap allocation
==30306==    at 0x4835DEF: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==30306==    by 0xAA17C1: Ms::Element::create(Ms::ElementType, Ms::Score*) (element.cpp:920)
==30306==    by 0xAA1F32: Ms::Element::name2Element(QStringRef const&, Ms::Score*) (element.cpp:1034)
==30306==    by 0xC3ED9D: Ms::readMeasure(Ms::Measure*, int, Ms::XmlReader&) (read206.cpp:2774)
==30306==    by 0xC41647: readStaffContent (read206.cpp:3303)
==30306==    by 0xC41647: Ms::readScore(Ms::Score*, Ms::XmlReader&) (read206.cpp:3428)
==30306==    by 0xC43DA1: Ms::MasterScore::read206(Ms::XmlReader&) (read206.cpp:3752)
==30306==    by 0xBE7304: Ms::MasterScore::read1(Ms::XmlReader&, bool) (scorefile.cpp:958)
==30306==    by 0xBEA7B2: Ms::MasterScore::loadMsc(QString, QIODevice*, bool) (scorefile.cpp:876)
==30306==    by 0xBEA864: Ms::MasterScore::loadMsc(QString, bool) (scorefile.cpp:864)
==30306==    by 0x818743: Ms::readScore(Ms::MasterScore*, QString, bool) (file.cpp:2243)
==30306==    by 0x818F4C: Ms::MuseScore::readScore(QString const&) (file.cpp:346)
==30306==    by 0x6863E2: Ms::convert(QString const&, QJsonArray const&, QString const&) (musescore.cpp:3397)

It’s not finished running, but this is the error I mean.

The code line in question is:

  920             case ElementType::HAIRPIN:           return new Hairpin(score);

Reply

• Dec 7, 2018 - 14:13

Thank you. Actually, it really helps a lot :-)
From what I see, when a hairpin is initialized, _beginTextPlace and _continueTextPlace are created as part of textLineBase, here:
https://github.com/musescore/MuseScore/blob/428542a52a5862474b8bca59d5b…
_beginTextPlace is then initialized here: https://github.com/musescore/MuseScore/blob/428542a52a5862474b8bca59d5b… to its default value for hairpins: https://github.com/musescore/MuseScore/blob/428542a52a5862474b8bca59d5b…
However, _continueTextPlace is left uninitialized; most of the times it gives me PlaceText::AUTO when querying the hairpin, but once I managed to see it return -120.
I think a solution could be to add
resetProperty(Pid::CONTINUE_TEXT_PLACE); after line 412
and a default
case Pid::CONTINUE_TEXT_PLACE:
return int(PlaceText::AUTO); after line 624, or, maybe even better, add it as a fallthrough in case Pid::BEGIN_TEXT_PLACE to a default PlaceText::LEFT.

Reply

• Dec 10, 2018 - 23:17

Thanks a lot for tracking down all these (memory corruption, instability, crashes, data corruption, etc.) bugs! I’m trying, but I’m not yet familiar enough with the tools, let alone the codebase, to do them all on my own.

Reply