This is the mail archive of the cygwin-developers mailing list for the Cygwin project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |