From f62ecfab6fda38adbbd541159ea676fcbf44f91f Mon Sep 17 00:00:00 2001 From: Frederik Seiffert Date: Wed, 19 Jul 2023 12:33:15 +0200 Subject: [PATCH] Fix NSFileManager thread safety --- ChangeLog | 6 ++ Headers/Foundation/NSFileManager.h | 1 - Source/NSFileManager.m | 147 ++++++++++++++++++++--------- 3 files changed, 106 insertions(+), 48 deletions(-) diff --git a/ChangeLog b/ChangeLog index e49acf8220..49cc0035f6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2023-07-25 Frederik Seiffert + + * Headers/Foundation/NSFileManager.h: + * Source/NSFileManager.m: + Fixed NSFileManager thread safety. + 2023-06-10 Riccardo Mottola * Tests/base/NSURL/Helpers/Launch.h: diff --git a/Headers/Foundation/NSFileManager.h b/Headers/Foundation/NSFileManager.h index 7492883a41..22c029cacb 100644 --- a/Headers/Foundation/NSFileManager.h +++ b/Headers/Foundation/NSFileManager.h @@ -216,7 +216,6 @@ GS_EXPORT_CLASS #if GS_EXPOSE(NSFileManager) @private id _delegate; - NSString *_lastError; #endif #if GS_NONFRAGILE #else diff --git a/Source/NSFileManager.m b/Source/NSFileManager.m index 166bb4d7eb..a658971093 100644 --- a/Source/NSFileManager.m +++ b/Source/NSFileManager.m @@ -60,6 +60,7 @@ #import "Foundation/NSURL.h" #import "Foundation/NSValue.h" #import "GSPrivate.h" +#import "GSPThread.h" #import "GNUstepBase/NSString+GNUstepBase.h" #import "GNUstepBase/NSTask+GNUstepBase.h" @@ -269,6 +270,19 @@ + (NSEnumerator*) enumeratorFor: (NSDictionary*)d; @end +gs_thread_key_t thread_last_error_key; + +static void GS_WINAPI +exitedThread(void *lastErrorPtr) +{ +#if __has_feature(objc_arc) + NSString *oldError = (__bridge_transfer NSString*)lastErrorPtr; +#pragma unused(oldError) +#else + RELEASE(lastErrorPtr); +#endif +} + @interface NSFileManager (PrivateMethods) @@ -304,6 +318,10 @@ - (BOOL) _proceedAccordingToHandler: (id) handler fromPath: (NSString*) fromPath toPath: (NSString*) toPath; +/* Thread-local last error variable. */ +- (NSString*) _lastError; +- (void) _setLastError: (NSString*)error; + /* A convenience method to return an NSError object. * If the _lastError message is set, this creates an NSError using * that message in the NSCocoaErrorDomain, otherwise it used the @@ -358,11 +376,12 @@ + (void) initialize { defaultEncoding = [NSString defaultCStringEncoding]; GSAttrDictionaryClass = [GSAttrDictionary class]; + GS_THREAD_KEY_INIT(thread_last_error_key, exitedThread); } - (void) dealloc { - TEST_RELEASE(_lastError); + [self _setLastError: nil]; [super dealloc]; } @@ -439,7 +458,7 @@ - (BOOL) changeFileAttributes: (NSDictionary*)attributes atPath: (NSString*)path str = [NSString stringWithFormat: @"Unable to change NSFileOwnerAccountID to '%"PRIuPTR"' - %@", num, [NSError _last]]; - ASSIGN(_lastError, str); + [self _setLastError: str]; } } else @@ -481,7 +500,7 @@ - (BOOL) changeFileAttributes: (NSDictionary*)attributes atPath: (NSString*)path str = [NSString stringWithFormat: @"Unable to change NSFileOwnerAccountName to '%@' - %@", str, [NSError _last]]; - ASSIGN(_lastError, str); + [self _setLastError: str]; } } } @@ -505,7 +524,7 @@ - (BOOL) changeFileAttributes: (NSDictionary*)attributes atPath: (NSString*)path str = [NSString stringWithFormat: @"Unable to change NSFileGroupOwnerAccountID to '%"PRIuPTR"' - %@", num, [NSError _last]]; - ASSIGN(_lastError, str); + [self _setLastError: str]; } } else if ((str = [attributes fileGroupOwnerAccountName]) != nil @@ -545,7 +564,7 @@ - (BOOL) changeFileAttributes: (NSDictionary*)attributes atPath: (NSString*)path str = [NSString stringWithFormat: @"Unable to change NSFileGroupOwnerAccountName to '%@' - %@", str, [NSError _last]]; - ASSIGN(_lastError, str); + [self _setLastError: str]; } } #endif /* _WIN32 */ @@ -559,7 +578,7 @@ - (BOOL) changeFileAttributes: (NSDictionary*)attributes atPath: (NSString*)path str = [NSString stringWithFormat: @"Unable to change NSFilePosixPermissions to '%o' - %@", (unsigned)num, [NSError _last]]; - ASSIGN(_lastError, str); + [self _setLastError: str]; } } @@ -629,7 +648,7 @@ - (BOOL) changeFileAttributes: (NSDictionary*)attributes atPath: (NSString*)path str = [NSString stringWithFormat: @"Unable to change NSFileCreationDate to '%@' - %@", date, [NSError _last]]; - ASSIGN(_lastError, str); + [self _setLastError: str]; } } @@ -678,7 +697,7 @@ - (BOOL) changeFileAttributes: (NSDictionary*)attributes atPath: (NSString*)path str = [NSString stringWithFormat: @"Unable to change NSFileModificationDate to '%@' - %@", date, [NSError _last]]; - ASSIGN(_lastError, str); + [self _setLastError: str]; } } @@ -805,7 +824,7 @@ - (NSArray*) contentsOfDirectoryAtURL: (NSURL*)url NSDirectoryEnumerator *direnum; NSString *path; - DESTROY(_lastError); + [self _setLastError: nil]; if (![[url scheme] isEqualToString: @"file"]) { @@ -908,8 +927,8 @@ - (NSDirectoryEnumerator *)enumeratorAtURL: (NSURL *)url NSString *path; BOOL shouldRecurse; BOOL shouldSkipHidden; - - DESTROY(_lastError); + + [self _setLastError: nil]; if (![[url scheme] isEqualToString: @"file"]) { @@ -953,7 +972,8 @@ - (NSArray*) contentsOfDirectoryAtPath: (NSString*)path error: (NSError**)error { NSArray *result; - DESTROY(_lastError); + [self _setLastError: nil]; + result = [self directoryContentsAtPath: path]; if (error != NULL) @@ -982,7 +1002,8 @@ - (BOOL) createDirectoryAtPath: (NSString *)path { BOOL result = NO; - DESTROY(_lastError); + [self _setLastError: nil]; + if (YES == flag) { NSEnumerator *paths = [[path pathComponents] objectEnumerator]; @@ -1013,8 +1034,8 @@ - (BOOL) createDirectoryAtPath: (NSString *)path } else { - result = NO; - ASSIGN(_lastError, @"Could not create directory - intermediate path did not exist or was not a directory"); + result = NO; + [self _setLastError: @"Could not create directory - intermediate path did not exist or was not a directory"]; } } @@ -1062,7 +1083,7 @@ - (BOOL) createDirectoryAtPath: (NSString*)path /* This is consistent with MacOSX - just return NO for an invalid path. */ if ([path length] == 0) { - ASSIGN(_lastError, @"no path given"); + [self _setLastError: @"no path given"]; return NO; } @@ -1080,7 +1101,7 @@ - (BOOL) createDirectoryAtPath: (NSString*)path e = [NSString stringWithFormat: @"path %@ exists ... cannot create", path]; } - ASSIGN(_lastError, e); + [self _setLastError: e]; return NO; } else @@ -1125,7 +1146,7 @@ - (BOOL) createDirectoryAtPath: (NSString*)path e = [NSString stringWithFormat: @"Could not create '%@' - '%@'", path, [NSError _last]]; - ASSIGN(_lastError, e); + [self _setLastError: e]; return NO; } } @@ -1156,7 +1177,7 @@ - (BOOL) createFileAtPath: (NSString*)path /* This is consistent with MacOSX - just return NO for an invalid path. */ if ([path length] == 0) { - ASSIGN(_lastError, @"no path given"); + [self _setLastError: @"no path given"]; return NO; } @@ -1322,8 +1343,7 @@ - (BOOL) copyPath: (NSString*)source if ([[destination stringByAppendingString: @"/"] hasPrefix: [source stringByAppendingString: @"/"]]) { - ASSIGN(_lastError, - @"Could not copy - destination is a descendant of source"); + [self _setLastError: @"Could not copy - destination is a descendant of source"]; return NO; } @@ -1332,7 +1352,7 @@ - (BOOL) copyPath: (NSString*)source if ([self createDirectoryAtPath: destination attributes: attrs] == NO) { return [self _proceedAccordingToHandler: handler - forError: _lastError + forError: [self _lastError] inPath: destination fromPath: source toPath: destination]; @@ -1385,7 +1405,8 @@ - (BOOL) copyItemAtPath: (NSString*)src { BOOL result; - DESTROY(_lastError); + [self _setLastError: nil]; + result = [self copyPath: src toPath: dst handler: nil]; if (error != NULL) @@ -1455,7 +1476,7 @@ - (BOOL) movePath: (NSString*)source if (sourceIsDir && [[destination stringByAppendingString: @"/"] hasPrefix: [source stringByAppendingString: @"/"]]) { - ASSIGN(_lastError, @"Could not move - destination is a descendant of source"); + [self _setLastError: @"Could not move - destination is a descendant of source"]; return NO; } @@ -1499,7 +1520,8 @@ - (BOOL) moveItemAtPath: (NSString*)src { BOOL result; - DESTROY(_lastError); + [self _setLastError: nil]; + result = [self movePath: src toPath: dst handler: nil]; if (error != NULL) @@ -1570,14 +1592,14 @@ - (BOOL) linkPath: (NSString*)source if ([[destination stringByAppendingString: @"/"] hasPrefix: [source stringByAppendingString: @"/"]]) { - ASSIGN(_lastError, @"Could not link - destination is a descendant of source"); + [self _setLastError: @"Could not link - destination is a descendant of source"]; return NO; } if ([self createDirectoryAtPath: destination attributes: attrs] == NO) { return [self _proceedAccordingToHandler: handler - forError: _lastError + forError: [self _lastError] inPath: destination fromPath: source toPath: destination]; @@ -1624,7 +1646,7 @@ - (BOOL) linkPath: (NSString*)source [self changeFileAttributes: attrs atPath: destination]; return YES; #else - ASSIGN(_lastError, @"Links not supported on this platform"); + [self _setLastError: @"Links not supported on this platform"]; return NO; #endif } @@ -1646,7 +1668,7 @@ - (BOOL) removeFileAtPath: (NSString*)path lpath = [self fileSystemRepresentationWithPath: path]; if (lpath == 0 || *lpath == 0) { - ASSIGN(_lastError, @"Could not remove - no path"); + [self _setLastError: @"Could not remove - no path"]; return NO; } else @@ -1741,7 +1763,8 @@ - (BOOL) removeItemAtPath: (NSString*)path { BOOL result; - DESTROY(_lastError); + [self _setLastError: nil]; + result = [self removeFileAtPath: path handler: nil]; if (error != NULL) @@ -1767,7 +1790,8 @@ - (BOOL) createSymbolicLinkAtPath: (NSString*)path { BOOL result; - DESTROY(_lastError); + [self _setLastError: nil]; + result = [self createSymbolicLinkAtPath: path pathContent: destPath]; if (error != NULL) @@ -1797,7 +1821,7 @@ - (BOOL) fileExistsAtPath: (NSString*)path isDirectory: (BOOL*)isDirectory if (lpath == 0 || *lpath == _NUL) { - ASSIGN(_lastError, @"no path given"); + [self _setLastError: @"no path given"]; return NO; } @@ -1875,7 +1899,7 @@ - (BOOL) isReadableFileAtPath: (NSString*)path if (lpath == 0 || *lpath == _NUL) { - ASSIGN(_lastError, @"no path given"); + [self _setLastError: @"no path given"]; return NO; } @@ -1932,7 +1956,7 @@ - (BOOL) isWritableFileAtPath: (NSString*)path if (lpath == 0 || *lpath == _NUL) { - ASSIGN(_lastError, @"no path given"); + [self _setLastError: @"no path given"]; return NO; } @@ -1974,7 +1998,7 @@ - (BOOL) isExecutableFileAtPath: (NSString*)path if (lpath == 0 || *lpath == _NUL) { - ASSIGN(_lastError, @"no path given"); + [self _setLastError: @"no path given"]; return NO; } @@ -2033,7 +2057,7 @@ - (BOOL) isDeletableFileAtPath: (NSString*)path if (lpath == 0 || *lpath == _NUL) { - ASSIGN(_lastError, @"no path given"); + [self _setLastError: @"no path given"]; return NO; } @@ -2227,7 +2251,8 @@ - (NSDictionary*) attributesOfItemAtPath: (NSString*)path { NSDictionary *d; - DESTROY(_lastError); + [self _setLastError: nil]; + d = [GSAttrDictionaryClass attributesAt: path traverseLink: NO]; if (error != NULL) @@ -2368,7 +2393,7 @@ - (NSDictionary*) attributesOfFileSystemForPath: (NSString*)path return [NSDictionary dictionaryWithObjects: values forKeys: keys count: 5]; #else GSOnceMLog(@"NSFileManager", @"no support for filesystem attributes"); - ASSIGN(_lastError, @"no support for filesystem attributes"); + [self _setLastError: @"no support for filesystem attributes"]; return nil; #endif #endif /* _WIN32 */ @@ -2522,7 +2547,7 @@ - (BOOL) createSymbolicLinkAtPath: (NSString*)path return (symlink(oldpath, newpath) == 0); #else - ASSIGN(_lastError, @"symbolic links not supported on this system"); + [self _setLastError: @"symbolic links not supported on this system"]; return NO; #endif } @@ -2547,7 +2572,7 @@ - (NSString*) pathContentOfSymbolicLinkAtPath: (NSString*)path return nil; } #else - ASSIGN(_lastError, @"symbolic links not supported on this system"); + [self _setLastError: @"symbolic links not supported on this system"]; return nil; #endif } @@ -3334,7 +3359,7 @@ - (BOOL) _copyPath: (NSString*)source if (dirOK == NO) { if (![self _proceedAccordingToHandler: handler - forError: _lastError + forError: [self _lastError] inPath: destinationFile fromPath: sourceFile toPath: destinationFile]) @@ -3398,7 +3423,7 @@ - (BOOL) _copyPath: (NSString*)source s = [NSString stringWithFormat: @"cannot copy file type '%@'", fileType]; - ASSIGN(_lastError, s); + [self _setLastError: s]; NSDebugLog(@"%@: %@", sourceFile, s); continue; } @@ -3441,7 +3466,7 @@ - (BOOL) _linkPath: (NSString*)source attributes: attributes] == NO) { if ([self _proceedAccordingToHandler: handler - forError: _lastError + forError: [self _lastError] inPath: destinationFile fromPath: sourceFile toPath: destinationFile] == NO) @@ -3502,7 +3527,7 @@ - (BOOL) _linkPath: (NSString*)source LEAVE_POOL return result; #else - ASSIGN(_lastError, @"Links not supported on this platform"); + [self _setLastError: @"Links not supported on this platform"]; return NO; #endif } @@ -3552,6 +3577,28 @@ - (BOOL) _proceedAccordingToHandler: (id) handler return NO; } +- (NSString*) _lastError +{ +#if __has_feature(objc_arc) + return (__bridge NSString*)GS_THREAD_KEY_GET(thread_last_error_key); +#else + return (NSString*)GS_THREAD_KEY_GET(thread_last_error_key); +#endif +} + +- (void) _setLastError: (NSString*)error +{ +#if __has_feature(objc_arc) + NSString *oldError = (__bridge_transfer NSString*)GS_THREAD_KEY_GET(thread_last_error_key); + GS_THREAD_KEY_SET(thread_last_error_key, (__bridge_retained void*)error); +#pragma unused(oldError) +#else + NSString *oldError = (NSString*)GS_THREAD_KEY_GET(thread_last_error_key); + GS_THREAD_KEY_SET(thread_last_error_key, RETAIN(error)); + RELEASE(oldError); +#endif +} + - (NSError*) _errorFrom: (NSString *)fromPath to: (NSString *)toPath { NSError *error; @@ -3559,10 +3606,11 @@ - (NSError*) _errorFrom: (NSString *)fromPath to: (NSString *)toPath NSString *message; NSString *domain; NSInteger code; + NSString *lastError = [self _lastError]; - if (_lastError) + if (lastError) { - message = _lastError; + message = lastError; domain = NSCocoaErrorDomain; code = 0; } @@ -3599,7 +3647,12 @@ - (NSError*) _errorFrom: (NSString *)fromPath to: (NSString *)toPath error = [NSError errorWithDomain: domain code: code userInfo: errorInfo]; - DESTROY(_lastError); + + if (lastError) + { + [self _setLastError: nil]; + } + return error; }