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

Wrap pthread mutex pointer in withUnsafeMutablePointer #104

Closed
wants to merge 2 commits into from

Conversation

nataliq-pp
Copy link
Contributor

@nataliq-pp nataliq-pp commented Jul 7, 2020

This is done to address such warnings:
Initialization of 'PThreadMutex' (aka 'UnsafeMutablePointer<_opaque_pthread_mutex_t>') results in a dangling pointer
which we started seeing recently.

This is what Xcode suggests:

1. Implicit argument conversion from 'pthread_mutex_t' (aka '_opaque_pthread_mutex_t') to 'UnsafeMutablePointer<pthread_mutex_t>' (aka 'UnsafeMutablePointer<_opaque_pthread_mutex_t>') produces a pointer valid only for the duration of the call to 'init(_:)'

2. Use 'withUnsafeMutablePointer' in order to explicitly convert argument to pointer valid for a defined scope

I don't fully understand if this fixes a potential problem or workarounds it -I hope that this is enough for safer memory usage. Tests pass.

@moglistree
Copy link
Contributor

A version bump with this would be nice as well so we can release it.

@nataliq-pp nataliq-pp mentioned this pull request Jul 7, 2020
@digitaliz
Copy link

digitaliz commented Jul 7, 2020

I don't think the fix here is to wrap construction of the pointer in a withUnsafeMutablePointer and assign that to a property. That will still lead to use of the pointer outside of the block. Instead I think we should remove the property and use the mutex directly but via withUnsafeMutablePointer blocks.

Eg.

withUnsafeMutablePointer(to: &_mutex) { $0.initialize() } instead of mutex.initialize() when initializing the mutex.

But I'm no expert on undefined behaviour, one potential way to verify that we fixed an actual issue is to run with Address Sanitiser and the new Undefined Behaviour Sanitiser to see if they output anything.

@moglistree
Copy link
Contributor

Initial findings show that having the Address Sanitiser on causes crashes on the Future in var mutex: PThreadMutex before and after the changes. Will do further investigation to find a way around this.

@nataliq-pp
Copy link
Contributor Author

Closing in favour of #110 - let's discuss there what are the next steps. @digitaliz please populate the issue. Thanks!

@nataliq-pp nataliq-pp closed this Mar 22, 2021
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.

3 participants