bugGNUstep - Bugs: bug #46956, Thread safety issues in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #46956: Thread safety issues in NSUserDefaults.m cause unwarranted exceptions to be raised

Submitted by:  Larry Campbell <lcampbel>
Submitted on:  Sat 23 Jan 2016 09:19:50 PM UTC  
 
Category: Base/FoundationSeverity: 3 - Normal
Item Group: BugStatus: None
Privacy: PublicAssigned to: None
Open/Closed: Closed

Mon 15 Feb 2016 11:17:52 AM UTC, comment #2:

Thanks very much.
I applied your patch, then found/fixed a memory leak, and then ended up doing some more major restructuring of the argument handling to try to ensure that we avoid recursion when setting up the argument domain. I also removed some obsolete code and tidied a little ... please let me know how the current code works for you.

Richard Frith-Macdonald <CaS>
Project Member
Sat 23 Jan 2016 09:24:46 PM UTC, comment #1:

(sorry for the formatting; don't know how to get bugzilla to preserve indentation)

Part, but not all, of the problem is that parsingArguments is being protected by the _lock ivar, but because parsingArguments is file static, and you can have multiple NSUserDefaults objects in play, that's not sufficient. You need to use classLock here.

But you can still end up with the second thread trying to call _createArgumentDictionary getting a nil arguments dictionary. Since my applications don't use that, I stopped there. I'll shortly be attaching a patch that fixes that, and that adds a sleep-and-retry loop for the case where some thread needs the standardUserDefaults but another thread is still in the process of creating it. We probably need a similar sleep-and-retry loop for _createArgumentDictionary but I didn't go that far because, as I said, I don't use it.

Larry Campbell <lcampbel>
Sat 23 Jan 2016 09:19:50 PM UTC, original submission:

The fix for bug #40620 has caused a new problem. Since _createArgumentsDictionary can now return nil, this can cause this snippet of initWithContentsOfFile:

[_tempDomains setObject: [self _createArgumentDictionary]
forKey: NSArgumentDomain];

to raise an exception:

Tried to add nil value for key 'NSArgumentDomain' to dictionary

Since this code can be called implicitly when you call something, like NSLog, that you don't really expect ever to raise exceptions, this can cause problems. (In my case, a lock was left locked, because I really didn't expect to have to catch exceptions from NSLog, and this caused a deadlock later on.)

The following code snippet easily reproduces the problem:

- (void)cr3283311_thread0:(NSDate *)deadline
{
NSAutoreleasePool *pool = [NSAutoreleasePool new];
const NSTimeInterval sleepTime = 0.01;
unsigned frobs = 0;
const NSTimeInterval blagInterval = 2.0;
NSDate *nextBlag = [[NSDate dateWithTimeIntervalSinceNow:blagInterval] retain];

NSLog(@"cr3283311_thread0 starting");
while ([deadline timeIntervalSinceNow] > 0.0) {
NSAutoreleasePool *pool = [NSAutoreleasePool new];
[[NSUserDefaults standardUserDefaults] setBool:YES forKey:@"freddieboy"];
[NSUserDefaults resetStandardUserDefaults];
[NSThread sleepForTimeInterval:sleepTime];
[[NSUserDefaults standardUserDefaults] setBool:YES forKey:@"freddieboy"];
[NSUserDefaults resetStandardUserDefaults];
[NSThread sleepForTimeInterval:sleepTime];
frobs++;
if ([nextBlag timeIntervalSinceNow] < 0.0) {
NSTimeInterval t = -[deadline timeIntervalSinceNow];
NSLog(@" Time remaining: %.1f sec; frobs: %u", t, frobs);
[nextBlag release];
nextBlag = [[NSDate dateWithTimeIntervalSinceNow:blagInterval] retain];
}
[pool release];
}
[nextBlag release];
NSLog(@"cr3283311_thread0 ending");
[pool release];
}

- (void)cr3283311_threadn:(NSDate *)deadline
{
NSAutoreleasePool *pool = [NSAutoreleasePool new];
const NSTimeInterval sleepTime = 0.01;
NSUInteger counts = 0;
unsigned trips = 0;

NSLog(@"cr3283311_threadn starting");
while ([deadline timeIntervalSinceNow] > 0.0) {
NSAutoreleasePool *pool = [NSAutoreleasePool new];
NSUserDefaults *ud = [NSUserDefaults standardUserDefaults];
NSDictionary *d;

NSAssert(ud != nil, @"NSUserDefaults botch");
d = [ud dictionaryRepresentation];
counts += [d count];
trips++;
[NSThread sleepForTimeInterval:sleepTime];
[pool release];
}
NSLog(@"cr3283311_threadn ending");
[pool release];
}

- (void)cr3283311
{
const unsigned threadCount = 4;
unsigned i;
const NSTimeInterval runningTime = 60.0;
NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:runningTime];

[NSThread detachNewThreadSelector:@selector(cr3283311_thread0:) toTarget:self withObject:deadline];
for (i = 0; i < threadCount; i++)
[NSThread detachNewThreadSelector:@selector(cr3283311_threadn:) toTarget:self withObject:deadline];

while ([deadline timeIntervalSinceNow] > 0.0)
[NSThread sleepForTimeInterval:0.1];
}

Larry Campbell <lcampbel>

 

Attached Files
file #36150:  NSUserDefaults.m.patch added by lcampbel (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by CaS (Posted a comment)
  • -unavailable- added by lcampbel (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 15 Feb 2016 11:17:52 AM UTCCaSOpen/ClosedOpen=>Closed
    Sat 23 Jan 2016 09:25:47 PM UTClcampbelAttached File-=>Added NSUserDefaults.m.patch, #36150

    Back to the top


    Powered by Savane 3.1-cleanup1