Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a few memory issues found with ASan #6843

Merged
merged 6 commits into from
Sep 3, 2023
Merged

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented Sep 3, 2023

I used an AddressSanitizer build to find and fix some memory issues:

  • Memory leak in LADSPA effects
  • Memory leak in SimpleTextFloat
  • Invalid iterator in AutomationClip
    • Occurred when adding and removing nodes in the Automation Editor
    • Likely the cause of Automation Editor crashes that I've experienced before
  • Buffer overflow in PianoView

I'm sure there are more memory-related errors I could find and fix with ASan, but this is enough for now.

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan on contributing the CAPS changes upstream? I checked https://github.com/moddevices/caps-lv2 and it appears the destructor still isn't virtual there.

@messmerd
Copy link
Member Author

messmerd commented Sep 3, 2023

@DomClark
I just submitted a PR for the CAPS changes upstream: mod-audio/caps-lv2#32

I guess next I should revert those changes in this PR and then later update the CAPS submodule if/when it gets merged upstream?

@DomClark
Copy link
Member

DomClark commented Sep 3, 2023

I guess next I should revert those changes in this PR and then later update the CAPS submodule if/when it gets merged upstream?

I think it's fine to keep the changes here. It doesn't looks like we've updated CAPS in a long time, and doing so is probably a somewhat larger task that shouldn't be a prerequisite for this fix. We've made a number of changes to our vendored copy of CAPS anyway, so it's not as if there isn't precedent for fixing things here.

@sakertooth sakertooth merged commit 0768f5a into LMMS:master Sep 3, 2023
@messmerd messmerd deleted the asan-fixes branch September 3, 2023 21:42
@tresf
Copy link
Member

tresf commented Oct 20, 2023

I guess next I should revert those changes in this PR and then later update the CAPS submodule if/when it gets merged upstream?

I think it's fine to keep the changes here. It doesn't looks like we've updated CAPS in a long time, and doing so is probably a somewhat larger task that shouldn't be a prerequisite for this fix. We've made a number of changes to our vendored copy of CAPS anyway, so it's not as if there isn't precedent for fixing things here.

We have an open PR with the upstream fork here: #4027. I think it's worthwhile to send any patches there. Technically, they should probably go to Tim Goetze. I'm not sure how often moddevices sends their code to him. He's pretty picky about patches, quoting:

Contact
If you think you have found a bug in CAPS, or if you want to make a related or unrelated remark or inquiry please write to [email protected]. 

Patches
Please do not send patches without asking first.  By submitting a patch, you claim sole authorship and agree that your submission becomes mine to use in any way I see fit, in exchange for no obligation on my part except attribution in the CAPS documentation. 

If we decide not to, that's OK too... the patches will just get lost once we upgrade and we'll have to backtrack a little.

@messmerd
Copy link
Member Author

@tresf I contacted Tim Goetze in early September and we agreed upon a solution to the memory leak, but he hasn't released a new version of CAPS yet. I don't know when/if he plans to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LMMS crashes when going left down piano in any instrument.
4 participants