iZettle / Flow

Flow is a Swift library for working with asynchronous flows and life cycles
MIT License
231 stars 14 forks source link

Wrap pthread mutex pointer in withUnsafeMutablePointer #104

Closed nataliq-pp closed 3 years ago

nataliq-pp commented 4 years ago

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 commented 4 years ago

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

digitaliz commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

Closing in favour of https://github.com/iZettle/Flow/issues/110 - let's discuss there what are the next steps. @digitaliz please populate the issue. Thanks!