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

Group
 
 

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

Submitter:  Larry Campbell <lcampbel>
Submitted:  Sat 23 Jan 2016 09:19:50 PM UTC
   
 
Category:  Base/Foundation Severity:  3 - Normal
Item Group:  Bug Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

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
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by lcampbel (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-02-15 CaS Open/ClosedOpen Closed
    2016-01-23 lcampbel Attached File- Added NSUserDefaults.m.patch, #36150

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code