[vlc-devel] [PATCH] darwin/thread: Ignore vlc_mutex_unlock failures

[Rémi Denis-Courmont]

Le maanantaina 28. lokakuuta 2019, 22.35.49 EET Marvin Scholz a écrit :
> On 28 Oct 2019, at 18:46, Rémi Denis-Courmont wrote:
> > Le maanantaina 28. lokakuuta 2019, 19.24.44 EET Marvin Scholz a écrit
> > 
> >> macOS pthread implementation for pthread_cond_wait is buggy, causing
> >> sometimes the mutex to not be locked when running thread cancellation
> >> cleanup handlers.
> >> This causes random failures of vlc_mutex_unlock, as the cancellation
> >> cleanup handler does not hold a lock on the mutex in rare cases.
> > 
> > This won't work:
> > 
> > 1) Darwin does not define unlocking a mutex by a thread that did not
> > lock it:
> > "Calling pthread_mutex_unlock with a mutex that the calling thread
> > does not
> > hold will result in undefined behavior." So you cannot simply assume
> > that it
> > will return an error and have no side effects; it probably only works
> > that way
> > on sunny day cases.
> > 
> > 2) Even if Darwin did provide that guarantee, this would cause data
> > races if
> > the calling thread actually relied on the mutex being locked to access
> > lock-
> > protected data.
> 
> Hi, thanks for the review.
> I am aware of these issues, the intention of this is just to make debug
> enabled
> builds on macOS work again without randomly crashing all the time.

No, you don't seem to be aware of what UB means. You cannot recover from UB. 
Period. This is too late in code flow to put a workaround; we always put our 
workarounds before the bug, not after it.

The only ways to handle UB is to 1) avoid getting there in the first place and 
2) abort as simply and quickly as possible if it happens anyway. It's not even 
about debugging, it's about security/safety.

If your argument is that the error actual happens, the correct solution is to 
replace the assert with an abort. Otherwise you concretely might as well 
deadlock, or fail the following lock or destroy, which are even worst than.

-- 
レミ・デニ-クールモン
http://www.remlab.net/