Fixed some static analysis warnings #8762

PatrickNorton · 2021-07-29T16:41:43Z

Partially resolves: #8547

Fixes some of the bugs brought up in this article.
Bugs fixed:

V501 (importmidi_simplify.cpp 44)
V1063 (lyrics.h 85)
V1065 (scorediff.cpp 444)
V595 (notationselectionrange.cpp 129)
V774 (importgtp-gp6.cpp 2592)
V506 (ove.cpp 4391)
V630 (IntervalTree.h 70)

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

Jojo-Schmitz · 2021-08-01T09:48:12Z

That merge commit doesn't belong there. Use git pull --rebase instead (followed by git push --force)
And then the remaining 2 comits should get squashed IMHO

Jojo-Schmitz · 2021-08-01T09:53:24Z

src/importexport/guitarpro/internal/importgtp-gp6.cpp

                delete slur;
-                legatos[slur->track()] = 0;


Good catch, wonder why this ever worked without crashing?

Jojo-Schmitz · 2021-08-01T09:58:57Z

thirdparty/intervaltree/IntervalTree.h

@@ -67,7 +67,7 @@ class IntervalTree {
        center = other.center;
        intervals = other.intervals;
        if (other.left) {
-            left = (intervalTree*) malloc(sizeof(intervalTree));
+            left = new intervalTree();


Not sure we really want to change third party code?

Although new is used in this file just a few lines below...

You're right; it looks like upstream has fixed this; are there any plans to merge it?

I'm not aware of any such plans.

Alright, I've reverted that and created a new issue merging upstream at #8808.

Ah, I see, even better

Jojo-Schmitz · 2021-08-01T10:12:48Z

src/engraving/libmscore/lyrics.h

@@ -92,7 +92,7 @@ class Lyrics final : public TextBase
    QString subtypeName() const override { return QObject::tr("Verse %1").arg(_no + 1); }
    void setNo(int n) { _no = n; }
    int no() const { return _no; }
-    bool isEven() const { return _no % 1; }
+    bool isEven() const { return _no % 2; }


What is actually really meant here is _no & 1 (or (_no + 1) % 2 or !(no % 2) , _no is 0 based, so 0 is actually verse 1 and as such odd.

See #7628 (for 3.x, got merged) and #7627 (got closed, so master still needs this fix)

Got it, although wouldn't _no & 1 give the same results as _no % 2? Godbolt seems to show the same assembly generated for both.

_no & 1is used in other places, for the very same purpose. So using it here too is a) consistent and b) the save bet

Jojo-Schmitz · 2021-08-01T10:16:37Z

src/importexport/midi/internal/midiimport/importmidi_simplify.cpp

@@ -62,7 +62,7 @@ bool areDurationsEqual(
        sum += ReducedFraction(d.second.fraction()) / d.first;
    }

-    return desiredLen == desiredLen;
+    return sum == desiredLen;


See #7627 (review) where this had come up before, and also #7179 (for 3.x), this change results in failing (actually crashing) mtests (on 3.x at least, which is why that PR is still in Draft).

Seems those tests are not yet enabled for master. Also that code only ever gets hit in a Debug build, so not in any release, nor in a 'normal' local MSVC (RelWithDebInfo) build.

See also https://musescore.org/en/node/314899

#7179 has just been merged, if you like integrate that one into your PR, else I'd port it over (it basically removes that stuff)

Jojo-Schmitz · 2021-08-01T10:23:47Z

src/notation/internal/notationselectionrange.cpp

    if (!startSegment) {
        return nullptr;
    }

+    startSegment->measure()->firstEnabled();


Outch, another good catch!

Note to self: the only change of this PR that won't apply to 3.x, as far as I can tell, except for the lyrics.h one, which is 3.x already)

Jojo-Schmitz · 2021-08-01T12:44:04Z

src/importexport/ove/internal/ove.cpp

@@ -4459,6 +4459,8 @@ bool OvscParse::parse()
    }
    m_ove->setShowColor(placeHolder.toBoolean());

+    m_handle = nullptr;
+


Here you left out the other 3 locations that document mentioned. And there are even more of those, many more, as far as can tell, even a few lines up, all these return false.

Good point. Since there's a lot of branching here, would a RAII-based solution make more sense than adding this line everywhere?

Huh? No idea what a a RAII-based solution might be. What I know is that this change is is not enough, jumping (way) too short.

But if you find a simpler one than you just did (it iis rather big), go ahead.

My terminology might be strange, but I've written up my idea and pushed it to here.

partial backport of musescore#8762

Jojo-Schmitz · 2021-08-07T12:49:12Z

src/importexport/ove/internal/ove.cpp

@@ -4392,6 +4405,7 @@ bool OvscParse::parse()
    Block placeHolder;

    m_handle = &handle;
+    auto _ = ResetToNull(&m_handle);


For some reason I don't get that to compile in QtCreator/MinGW (and on top of the 3.x branch):

...\importexport\ove\ove.cpp:3873: Error: missing template arguments before '(' token ...\importexport\ove\ove.cpp: In member function 'virtual bool OVE::TrackParse::parse()': ...\importexport\ove\ove.cpp:3873:27: error: missing template arguments before '(' token auto _ = ResetToNull(&handle_); ^

Changing those calls to auto _ = ResetToNull<typeof(*m_handle)>(&handle_); fixes that

Interesting... it looks like MinGW doesn't like class template argument deduction for some reason.

No wonder, as this is C++17. We're not requiring this anywhere yet, just C++14 (for MSVC), C++11 for Xcode, and gnu++11 for gcc (which includes MinGW), although I checked 3.x only, maybe master is a step ahead.

From here, it looks like C++17 is required for master.

As of 484f8dc, 09Oct2020, Qt 5.15 (and a C++17 capable toolchain) is required for the master branch.

Regardless, I do think there's no reason to break anything for this, so I've written in a fix.

I see (and yes, I now remember). Still using typeof might be better

Jojo-Schmitz · 2021-08-07T13:50:32Z

src/importexport/ove/internal/ove.cpp

@@ -4405,7 +4405,7 @@ bool OvscParse::parse()
    Block placeHolder;

    m_handle = &handle;
-    auto _ = ResetToNull(&m_handle);
+    auto _ = ResetToNull<StreamHandle>(&m_handle);


I like my <typeof(*m_handle)> better

Would it be better to add a free function so the template-deduction happens regardless of version?

If possible without jumping through burning hoops ;-)

But for master it might not be needed, your original version is just fine, as we're having C++17 there as a fixed requirement.

The hoops were lukewarm at best, so I added it—I think it looks a lot nicer.

partial backport of musescore#8762

RomanPudashkin · 2021-08-09T08:40:57Z

src/importexport/midi/internal/midiimport/importmidi_simplify.cpp

@@ -62,6 +62,8 @@ bool areDurationsEqual(
        sum += ReducedFraction(d.second.fraction()) / d.first;
    }

+    // Note: This looks really weird, but fixing it seems to break several


It seems that this function is used only for debugging. I believe we should just remove it completely because it does more harm than good

Yes, it is used for debugging only, And there causes issues when fixed to do a usefull comparison.

RomanPudashkin · 2021-08-09T09:05:56Z

src/importexport/ove/internal/ove.cpp

@@ -4392,6 +4411,7 @@ bool OvscParse::parse()
    Block placeHolder;

    m_handle = &handle;
+    auto _ = resetToNull(&m_handle);


It seems that this problem should be solved in another way. Better to remove m_handle and add the handle to arguments of BasicParse::readBuffer and BasicParse::jump:

BasicParse::readBuffer(StreamHandle& handle, Block& placeHolder, int size)
BasicParse::jump(StreamHandle& handle, int offset)

This is more explicit and doesn't cause the invalid pointer problem

doesn't seem to be an easy or small change

by removing the entire thing, it seems useless, esp. that `return desiredLen == desiredLen;` and even harmful and wrong anyway. Duplicate of musescore#7179, but also part of the backport of musescore#8762

partial backport of musescore#8762

Jojo-Schmitz · 2021-11-30T09:26:28Z

rebase needed

partial backport of musescore#8762

RomanPudashkin · 2022-12-20T10:02:44Z

closing due to a long period of inactivity

RobFog · 2022-12-20T10:11:24Z

@PatrickNorton, perhaps, if you're interested, the parts of this pull request that did not receive requests for modification could be submitted in a new pull request?

partial backport of musescore#8762

Partial fix musescore#8547: Fixed some static analysis warnings

29514ba

Jojo-Schmitz reviewed Aug 1, 2021

View reviewed changes

Jojo-Schmitz mentioned this pull request Aug 1, 2021

[MU3] Fix #314899: Identical expression #7179

Merged

Jojo-Schmitz reviewed Aug 1, 2021

View reviewed changes

This was referenced Aug 1, 2021

Lyric line spacing is broken #8638

Closed

[MU4 Issue] Lyrics have too much vertical spacing between verses and hyphens too high #8615

Closed

[MU3 Issue] MuseScore 3.6.3 or 3.7.0, PRs to be merged and issues to be fixed. #7449

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 1, 2021

Fixed some static analysis warnings

c473081

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 2, 2021

Fixed some static analysis warnings

ac8394c

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 3, 2021

Fixed some static analysis warnings

97ef893

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 3, 2021

Fixed some static analysis warnings

a59835b

partial backport of musescore#8762

PatrickNorton force-pushed the fix-static-warnings branch from 1c5cf15 to 29514ba Compare August 5, 2021 17:24

PatrickNorton added 2 commits August 5, 2021 14:03

Fixed potential use-after-free with BasicParse.m_handle

dea53c5

Changed Lyrics::isEven implementation

6f0e763

PatrickNorton force-pushed the fix-static-warnings branch from 13f2857 to d374119 Compare August 5, 2021 18:44

Made it harder to accidentally dangle a pointer

ae4ba94

PatrickNorton force-pushed the fix-static-warnings branch from d374119 to ae4ba94 Compare August 5, 2021 18:49

Reverted patch in areDurationsEqual

c7e325f

PatrickNorton force-pushed the fix-static-warnings branch from 38bba33 to c7e325f Compare August 6, 2021 13:59

Jojo-Schmitz reviewed Aug 7, 2021

View reviewed changes

Added explicit template arguments

e3d2f11

Jojo-Schmitz reviewed Aug 7, 2021

View reviewed changes

Added resetToNull free function

429ec27

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 7, 2021

Fixed some static analysis warnings

c8c7510

partial backport of musescore#8762

RomanPudashkin reviewed Aug 9, 2021

View reviewed changes

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021

Fixed some static analysis warnings

f0399b1

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021

Fixed some static analysis warnings

d616c46

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021

Fixed some static analysis warnings

2119ad7

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021

Fixed some static analysis warnings

2f0d6cd

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 30, 2021

Fixed some static analysis warnings

6c7f0cf

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 1, 2021

Fixed some static analysis warnings

bd9ad72

partial backport of musescore#8762

Jojo-Schmitz mentioned this pull request Sep 1, 2021

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 2, 2021

Fixed some static analysis warnings

3cc3495

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 9, 2021

Fixed some static analysis warnings

c848b01

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 10, 2021

Fixed some static analysis warnings

1c707cf

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 23, 2021

Fixed some static analysis warnings

f7b1348

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 26, 2021

Fixed some static analysis warnings

a0b185b

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 29, 2021

Fixed some static analysis warnings

e0141ef

partial backport of musescore#8762

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 12, 2022

Fixed some static analysis warnings

c6bcaba

partial backport of musescore#8762

RomanPudashkin closed this Dec 20, 2022

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023

Fixed some static analysis warnings

41a17da

partial backport of musescore#8762

Fixed some static analysis warnings #8762

Fixed some static analysis warnings #8762

Uh oh!

Conversation

PatrickNorton commented Jul 29, 2021

Uh oh!

Jojo-Schmitz commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jojo-Schmitz Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jojo-Schmitz Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Jojo-Schmitz commented Aug 1, 2021 •

edited

Loading

Jojo-Schmitz Aug 1, 2021 •

edited

Loading

Jojo-Schmitz Aug 1, 2021 •

edited

Loading

Jojo-Schmitz Aug 1, 2021 •

edited

Loading

Jojo-Schmitz Aug 1, 2021 •

edited

Loading

Jojo-Schmitz Aug 1, 2021 •

edited

Loading

Jojo-Schmitz Aug 1, 2021 •

edited

Loading

Jojo-Schmitz Aug 5, 2021 •

edited

Loading

Jojo-Schmitz Aug 7, 2021 •

edited

Loading

Jojo-Schmitz Aug 7, 2021 •

edited

Loading

Jojo-Schmitz Aug 7, 2021 •

edited

Loading

Jojo-Schmitz Aug 7, 2021 •

edited

Loading

Jojo-Schmitz Aug 7, 2021 •

edited

Loading