Skip to content

Audio/MidiJack: Fix invalid reads #5452

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 15, 2020
Merged

Audio/MidiJack: Fix invalid reads #5452

merged 4 commits into from
May 15, 2020

Conversation

JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Apr 13, 2020 •
edited
Loading

Fixes #5408

Info for testers:

  • You need to have both JackAudio and JackMidi enabled in the settings to test this PR (restart required after changing settings).
  • I already tested that the warnings from valgrind are gone now, but if someone who uses JackMidi can confirm that it still works as usual, that would be nice.

@JohannesLorenz JohannesLorenz mentioned this pull request Apr 13, 2020
Closed
@JohannesLorenz JohannesLorenz force-pushed the fix-jack-issues branch 2 times, most recently from dbd5fec to ce38da3 Compare April 13, 2020 14:20
@JohannesLorenz JohannesLorenz mentioned this pull request Apr 13, 2020
Closed
@LmmsBot
Copy link

LmmsBot commented Apr 13, 2020 •
edited
Loading

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

🤖 
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/6xi82qkh2lvbsbmr/artifacts/build/lmms-1.2.1-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/32281936"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/udvouh4uhc0u4eh8/artifacts/build/lmms-1.2.1-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/32281936"}]}, "commit_sha": "3a0a14514da26076d1fd7d477013a16cfca25277"}

@JohannesLorenz
Audio/MidiJack: Fix invalid read: (1) of LMMS#5408
aef1dca 
This fixes reading from jack MIDI events in case where
there are no jack MIDI events.
@JohannesLorenz
Audio/MidiJack: Fix invalid read: (2) of LMMS#5408
9951dcd 
This patch
* makes `m_stopped` atomic
* initializes `m_stopped` correctly to `true`
* moves the initialization of `m_stopped` to the point where jack ports
  are already connected
@JohannesLorenz
Audio/MidiJack: Fix invalid read: (3) of LMMS#5408
d453d77 
This atomically unsets the MidiJack reference in AudioJack right before
MidiJack is destroyed. This avoids AudioJack using a destroyed MidiJack
object.
@JohannesLorenz
JackMidi: Remove confusing warning at shutdown
3a0a145 
Warning is:

```
jack_port_unregister called with an incorrect port 0
Failed to unregister jack midi output
```
@JohannesLorenz JohannesLorenz changed the title Fix uninitialized value read Audio/MidiJack: Fix invalid reads Apr 19, 2020
@PhysSong PhysSong marked this pull request as ready for review April 21, 2020 00:39
@JohannesLorenz JohannesLorenz added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing labels Apr 23, 2020
@JohannesLorenz JohannesLorenz mentioned this pull request Apr 23, 2020
Closed
@PhysSong PhysSong changed the base branch from master to stable-1.2 April 23, 2020 10:09
@JohannesLorenz
Copy link
Contributor Author

JohannesLorenz commented Apr 23, 2020 •
edited
Loading

Thanks to @Spekular for functional and style checks. Now it's time for testing (see notes in the original post).

@JohannesLorenz JohannesLorenz removed needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR labels Apr 23, 2020
@JohannesLorenz
Copy link
Contributor Author

Tested. JackMidi still works. Merge Time!

@JohannesLorenz JohannesLorenz merged commit 737fcd3 into LMMS:stable-1.2 May 15, 2020
@Spekular Spekular added this to the 1.2.2 milestone May 20, 2020
@JohannesLorenz JohannesLorenz mentioned this pull request May 22, 2020
Closed
@PhysSong PhysSong mentioned this pull request Dec 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs testing This pull request needs more testing
Projects
None yet
Milestone
1.2.2
Development

Successfully merging this pull request may close these issues.

JackAudio + JackMidi cause invalid and uninitialized reads
3 participants
@JohannesLorenz @LmmsBot @Spekular