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

Tue Oct 29 12:37:46 CET 2019

I didn't see any explanations, no.

Le 29 octobre 2019 12:50:25 GMT+02:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>
>On Tue, Oct 29, 2019, at 11:39, Rémi Denis-Courmont wrote:
>> How do you even know that a crash report is related to this issue?
>
>We can't really know.
>
>> What I see is that this patch makes two assumptions,neither of them
>are documented nor backed in any way. But sure send it to the TC.
>Obviously, you will provide explanations what the assumptions
>underlying this patch are, how they are verified, and why no proper
>workaround can be made instead, so that the TC can make an informed
>decision,
>> and you won't abuse your ties with Hugo to distort the vote, hmm...
>
>I think Marvin already provided the explanation. The most important
>thing here is that this issue exists since a long time and don't seem
>to impact releases builds (we can't be 100% sure). We don't now how to
>fix it. This assert is preventing macOS developers to enable assert on
>debug build. This patch is just making sure that debug and release
>build have the same behavior regarding threading.
>
>
>> 
>> Le 29 octobre 2019 11:48:16 GMT+02:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>>> On Tue, Oct 29, 2019, at 08:24, Rémi Denis-Courmont wrote:
>>>> Did you investigate all crash reports to verify that none of them
>were not caused by the unspecified constraints that you claim exist?
>>> 
>>> Not me personally, but iOS maintainers and Marvin forward to me
>every native crashes happening on iOS and macOS. Never saw one related
>to this specific issue.
>>> 
>>>> Did you check what the implementations on affected macOS versions
>actually does apply those constraints? All that I see is that the
>Darwin doc contradicts your statement.
>>> 
>>> It won't be the first time that an Apple documentation contradict
>the actual behavior.
>>> 
>>> Every one but Rémi are in favor of pushing this patch. Should we
>vote on TC to resolve this conflict ?
>>> 
>>>> 
>>>> Le 29 octobre 2019 09:10:36 GMT+02:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>>>>> OK, it's an UB, but on a constrained environment, that is macOS.
>For now, this UB seem to have 0 impact on users. As Marvin said, we had
>this issue since 7 years, VLC 3.x and 2.x users don't seem to be
>bothered by this.
>>>>> 
>>>>> We can't abort VLC for an UB that won't have any impact.
>>>>> 
>>>>> I'm OK with this patch and with a 3.0 backport. It is was I do
>locally every time I develop on macOS (in fact, I just define NDEBUG
>for this file).
>>>>> 
>>>>> One other argument in favor: this may be only temporary since
>Marvin finally reported this issue to Apple.
>>>>> 
>>>>> 
>>>>> On Tue, Oct 29, 2019, at 04:26, Rémi Denis-Courmont wrote:
>>>>>> 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/vlc-devel mailing list
>>>>>> To unsubscribe or modify your subscription options:
>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>> vlc-devel mailing list
>>>>> To unsubscribe or modify your subscription options:
>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>> 
>>>> -- 
>>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>excuser ma brièveté. 
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>> 
>> 
>> -- 
>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>excuser ma brièveté. 
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20191029/f96dc0ae/attachment.html>