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

Try again to fix Coverity warning in SDL Sound #3005

Merged
merged 2 commits into from
Oct 15, 2023
Merged

Try again to fix Coverity warning in SDL Sound #3005

merged 2 commits into from
Oct 15, 2023

Conversation

weirddan455
Copy link
Collaborator

Description

The previous PR did not solve the issue. Coverity saw my assert and still thinks it's freeing from the middle of the list.

coverity

It's possible it is seeing a data race. I've removed the use of the volatile keyword and used a mutex lock around the entire loop. Sound_FreeSample also takes a lock but SDL's mutex is recursive so this should be fine.

This also removes a temporary variable that was maybe confusing Coverity. Now Sound_FreeSample is explicitly being called with sample_list (head of the linked list).

Related issues

#2996

Manual testing

Started Quake with ripped .ogg sound files. Closed Dosbox and confirmed there was no crashes or deadlocks.

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

This is a 2nd attempt at fixing a Coverity issue #2996
It potentially fixes a data race (not sure if this is what Coverity saw)
Sound_FreeSample also takes a lock but SDL's mutex is recursive so this
should be fine.
@weirddan455 weirddan455 added the plumbing Issues related to low-level support functions and classes label Oct 15, 2023
@weirddan455 weirddan455 self-assigned this Oct 15, 2023
@weirddan455
Copy link
Collaborator Author

I'm not sure what's wrong with the macOS build. I've seen it fail like this before randomly. I think this is the machine you run locally @kcgen ?

Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under '/Volumes/RamDrive/ccache/tmp/_actions/actions/checkout/v4'. Did you forget to run actions/checkout before running your local action?

Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

Looks good, @weirddan455.
Let's see what Coverity thinks.

@kcgen kcgen merged commit d5cfed0 into main Oct 15, 2023
50 checks passed
@kcgen
Copy link
Member

kcgen commented Oct 15, 2023

I'm not sure what's wrong with the macOS build. I've seen it fail like this before randomly. I think this is the machine you run locally @kcgen ?

Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under '/Volumes/RamDrive/ccache/tmp/_actions/actions/checkout/v4'. Did you forget to run actions/checkout before running your local action?

Yes - this has eluded me. The machine runs a single GitHub's "runner" process; it's in charge of managing its workspace where it checks out the project and run the CI commands.

The runs happen serially, usually with a couple seconds between back-to-back runs.

When it goes off the rails, Meson reports that a bunch of critical project files are missing - like in this case, docs/dosbox.1, which should be impossible because the runner checks out the project (and the branch), sets up dependencies, etc.. before setting up the build.

My only hunch is that there's some kind of race-condition where the runner purges the content from a running job, and it impacts the current job, because the runner does re-use the same directory tree for subsequent jobs.

I've stopped the runner, purged all the directories, and restarted it - for what it's worth (probably nothing 🤦 ).

There isn't much to configure with the runners - you tie them into a project, point the runner service to its workspace area, and just let it run.

@weirddan455 weirddan455 deleted the wd/sdl_sound branch October 15, 2023 04:39
@johnnovak johnnovak added the audio Audio related issues or enhancements label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio Audio related issues or enhancements plumbing Issues related to low-level support functions and classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants