[vlc-devel] [PATCH 1/6] os2: implement vlc_once()

[Rémi Denis-Courmont]

Le 3 avril 2018 13:07:50 GMT+02:00, Kamil Rytarowski <n54 at gmx.com> a écrit :
>On 03.04.2018 12:48, Rémi Denis-Courmont wrote:
>> Le 3 avril 2018 12:35:15 GMT+02:00, Kamil Rytarowski <n54 at gmx.com> a
>écrit :
>>> On 03.04.2018 08:39, Rémi Denis-Courmont wrote:
>>>> Le mardi 3 avril 2018, 00:12:53 EEST Kamil Rytarowski a écrit :
>>>>> On 23.03.2018 09:54, Rémi Denis-Courmont wrote:
>>>>>> Le jeudi 22 mars 2018, 16:41:29 EET KO Myung-Hun a écrit :
>>>>>>> ---
>>>>>>>
>>>>>>>  include/vlc_threads.h |  6 ++++++
>>>>>>>  src/os2/thread.c      | 17 +++++++++++++++++
>>>>>>>  2 files changed, 23 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
>>>>>>> index 37054a3ded..b7505da266 100644
>>>>>>> --- a/include/vlc_threads.h
>>>>>>> +++ b/include/vlc_threads.h
>>>>>>> @@ -125,6 +125,12 @@ typedef struct
>>>>>>>
>>>>>>>  #define VLC_STATIC_COND { NULLHANDLE, 0, NULLHANDLE, 0 }
>>>>>>>  #define LIBVLC_NEED_SEMAPHORE
>>>>>>>  #define LIBVLC_NEED_RWLOCK
>>>>>>>
>>>>>>> +typedef struct
>>>>>>> +{
>>>>>>> +    unsigned done;
>>>>>>> +    vlc_mutex_t mutex;
>>>>>>> +} vlc_once_t;
>>>>>>> +#define VLC_STATIC_ONCE { 0, VLC_STATIC_MUTEX }
>>>>>>>
>>>>>>>  typedef struct vlc_threadvar *vlc_threadvar_t;
>>>>>>>  typedef struct vlc_timer *vlc_timer_t;
>>>>>>>
>>>>>>> diff --git a/src/os2/thread.c b/src/os2/thread.c
>>>>>>> index 8f7b335ea4..52f58023b8 100644
>>>>>>> --- a/src/os2/thread.c
>>>>>>> +++ b/src/os2/thread.c
>>>>>>> @@ -427,6 +427,23 @@ int vlc_cond_timedwait_daytime (vlc_cond_t
>>>>>>> *p_condvar,
>>>>>>> vlc_mutex_t *p_mutex, return vlc_cond_wait_common (p_condvar,
>>> p_mutex,
>>>>>>> ulTimeout);
>>>>>>>
>>>>>>>  }
>>>>>>>
>>>>>>> +void vlc_once(vlc_once_t *once, void (*cb)(void))
>>>>>>> +{
>>>>>>> +    if( !once->done )
>>>>>>
>>>>>> This looks like a violation of the memory model.
>>>>>
>>>>> This patch is correct.
>>>>
>>>> No, it is not.
>>>>
>>>
>>> Show a testcase breaking it with correct usage of this API,
>otherwise
>>> this claim is to be treated as unreasonable and shall be ignored,
>and
>>> patch merged.
>> 
>> Such a process could work in a fully defined language, such as single
>threaded Java. C is not a fully defined language in general and
>multithreaded C in particular.
>> 
>> For such a small function, it is pretty damn obvious that access to
>pto_done is not according to either types of accesses defined by the
>POSIX memory model. It only works if there is no race, but the whole
>point of the function is to handle potential races.
>> 
>
>A testcase is still missing.

Call the function on one thread. Then when it has taken the mutex, call the function on another thread with the same one-time initiliazer.

pto_done gets written on one thread and read on another one. UB. QED.

Yeah, this is really obvious on such simple code.
-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.