Re: sem_init() fails (when used in a certain way)

[Jon TURNEY <jon dot turney at dronecode dot org dot uk>]

On 30/03/2011 06:35, Christopher Faylor wrote:
> On Tue, Mar 29, 2011 at 11:38:32PM +0200, Corinna Vinschen wrote:
>> On Mar 29 13:24, Christopher Faylor wrote:
>>> On Tue, Mar 29, 2011 at 05:53:31PM +0200, Corinna Vinschen wrote:
>>>> On Mar 29 14:41, Jon TURNEY wrote:
>>>>> $ ./sem_init_malloc_testcase
>>>>> sem_init: Device or resource busy
>>>>> [...]
>>>>> I'm not sure how to fix this:
>>>>>
>>>>> Changing sem_t from a pointer to an instance of class semaphore is not a good
>>>>> idea as it would change a lot of code, and a non-starter as it breaks ABI by
>>>>> changing sizeof(sem_t), and I have to assume there is a reason it was
>>>>> implemented using a pointer in the first place.
>>>>>
>>>>> Removing the is_good_object() check from semaphore::init() (and thus changing
>>>>> the undefined behaviour when a sem_init() is used twice from 'return EBUSY' to
>>>>> 'leak some memory') would work.  Perhaps downgrading the error to strace
>>>>> output "potential repeated semaphore initialization"?
>>>>
>>>> This sounds like a good idea to me.  Given that the test can accidentally
>>>> identify the content of the semaphore as valid, the test is somewhat
>>>> dangerous.
>>>>
>>>>> I hope someone has some better ideas?
>>>>
>>>> I don't think there's any other way.  Glibc does not check the semaphore
>>>> storage at all when calling sem_init and SUSv4 states
>>>>
>>>>  "Attempting to initialize an already initialized semaphore results in
>>>>   undefined behavior."
>>>>
>>>> I'd say, just go ahead.
>>>
>>> I think we should put a
>>>
>>>  myfault efault;
>>>  if (efault.faulted ())
>>>     ...
>>>
>>> in place of the is_good_object() test and sprinkle those throughout the
>>> other sem_* functions, if they're not already there.
>>
>> You can't just replace all is_good_object tests with myfault handlers,
>> afaics.  The only case where the is_good_object test doesn't make sense
>> for the reason outlined in Jon's mail are the init methods of the
>> various object types.  In all other methods the is_good_object test is
>> still necessary to check the object pointer and to generate the EINVAL
>> error code reliably.  So the myfault handler could (and probably
>> should) be added to these methods while keeping the is_good_object
>> test.
> 
> The reason I mentioned putting them in the functions rather than an init
> function is to catch any subsequent problems with dereferencing invalid
> pointers.  If you put a handler in the init function then it is only
> valid for the life of the init function.  I wasn't suggesting replacing
> all of the is_good_object tests, though, just the one that Jon
> identified.

I don't think I understand what you are suggesting here.

That seems to be about the orthogonal issue of how sem_init() should deal with
being handed an invalid pointer.  is_good_object() uses efault, but only in so
far as if it's an invalid pointer, it can't be a good object, so sem_init()
trundles on and tries to store the address of the newly allocated semaphore
object through that invalid pointer :-)

Attached is a minimal patch to fix the issue I've identified with sem_init.  I
haven't looked at the other classes in that file.

I am suspicious of arguments that this should be changed some other way 'for
performance' which aren't backed up by profiling data.

Attachment: sem_init_good_object.patch
Description: Text document