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

[Carlos O'Donell <carlos@redhat.com>]

On 04/21/2016 12:23 PM, Torvald Riegel wrote:
> On Wed, 2016-04-20 at 20:16 +0100, 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?
> 
> IMO, abort() would be better than returning EINVAL.  See
> https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program

Agreed.

It's easy to detect. We should abort().

-- 
Cheers,
Carlos.