Re: [PATCH] Don't divide by zero when trying to destroy an uninitialised barrier.

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 20-04-2016 16:16, Mark Thompson wrote:
> On 20/04/16 18:03, Szabolcs Nagy wrote:
>> On 20/04/16 17:48, Mark Thompson wrote:
>>> ---
>>>  nptl/pthread_barrier_destroy.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/nptl/pthread_barrier_destroy.c b/nptl/pthread_barrier_destroy.c
>>> index 92d2027..d114084 100644
>>> --- a/nptl/pthread_barrier_destroy.c
>>> +++ b/nptl/pthread_barrier_destroy.c
>>> @@ -36,6 +36,15 @@ pthread_barrier_destroy (pthread_barrier_t *barrier)
>>>       they have exited as well.  To get the notification, pretend that we have
>>>       reached the reset threshold.  */
>>>    unsigned int count = bar->count;
>>> +
>>> +  /* For an initialised barrier, count must be greater than zero here.  An
>>> +     uninitialised barrier may still have zero, however, and in this case it is
>>> +     preferable to fail immediately rather than to invoke undefined behaviour
>>> +     by dividing by zero on the next line.  (POSIX allows the implementation to
>>> +     diagnose invalid state and return EINVAL in this case.)  */
>>> +  if (__glibc_unlikely (count == 0))
>>> +    return EINVAL;
>>> +
>>
>> this case is undefined behaviour in posix, and
>> i think the best way to handle that is crashing.
>> (because no behaviour can be portably relied upon)
> 
> The undefined behaviour is not necessarily crashing - systems which do not trap on divide by zero (such as aarch64) will do something else, such as returning success or hanging forever.  Would you prefer an abort() be added to make the behavior consistent?
> 
>> nowadays posix says
>> "The [EINVAL] error for an uninitialized barrier
>> attributes object is removed; this condition
>> results in undefined behavior."
> 
> It also says:
> 
> "If an implementation detects that the value specified by the barrier argument to pthread_barrier_destroy() does not refer to an initialized barrier object, it is recommended that the function should fail and report an [EINVAL] error."
> 
> (<http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_barrier_destroy.html>)
> 

I do not see a compelling reason to not return EINVAL if the UB 
could be detected and if POSIX stated this behaviour is recommended.