Browse Source

Fix isOpen logic

`FMDatabase` was previously checking `_db` to determine whether the opening of a database was successful or not. But that's not a valid assumption. If you had previously attempted to open a database and it failed, _it still returns a `sqlite*` pointer._ As the [`sqlite_open` documentation](http://sqlite.org/c3ref/open.html) says (emphasis added):

> _Whether or not an error occurs_ when it is opened, resources associated with the [database connection](http://sqlite.org/c3ref/sqlite3.html) handle should be released by passing it to [sqlite3_close()](http://sqlite.org/c3ref/close.html) when it is no longer required.

The reason this is important is that if an `openWithOptions` failed (e.g. opening from application support directory without `SQLITE_OPEN_CREATE` option), you want to be able to fix the problem (copy database from bundle to destination directory) and then try `openWithOptions` again. But because the `open` method was checking the existence of `_db` as evidence whether it was successfully opened or not, it was falsely reporting that the second attempt to reopen the database was successful, even though it didn't even try.

So, I created a test case that manifested this problem, `testOpenFailure`, and then modified `FMDatabase` accordingly. I could have made this a private property, but I saw no downside in exposing this in the public interface.

As a more general note, the reason I'm doing this is I've been seeing examples on the interwebs where people were (a) checking for existence of database; (b) if not there, copying one from bundle; and (c) then opening database. But this is not a practice that Apple advocates. One should really just (a) try to open (without create option) and if it failed, only then (b) copy from bundle and try again. As [they say](https://developer.apple.com/documentation/foundation/nsfilemanager/1415645-fileexistsatpath?language=objc):

> Note
>
> Attempting to predicate behavior based on the current state of the file system or a particular file on the file system is not recommended. Doing so can cause odd behavior or race conditions. It’s far better to attempt an operation (such as loading a file or creating a directory), check for errors, and handle those errors gracefully than it is to try to figure out ahead of time whether the operation will succeed. For more information on file-system race conditions, see [Race Conditions and Secure File Operations](https://developer.apple.com/library/content/documentation/Security/Conceptual/SecureCodingGuide/Articles/RaceConditions.html#//apple_ref/doc/uid/TP40002585) in [Secure Coding Guide](https://developer.apple.com/library/content/documentation/Security/Conceptual/SecureCodingGuide/Introduction.html#//apple_ref/doc/uid/TP40002415).
Robert M. Ryan 8 years ago
parent
commit
884ee3dc41
3 changed files with 73 additions and 5 deletions
  1. 44 0
      Tests/FMDatabaseTests.m
  2. 4 0
      src/fmdb/FMDatabase.h
  3. 25 5
      src/fmdb/FMDatabase.m

+ 44 - 0
Tests/FMDatabaseTests.m

@@ -1486,4 +1486,48 @@ - (void)testImmediateTransaction {
     XCTAssertEqualObjects([db lastError].localizedDescription, @"cannot start a transaction within a transaction");
     XCTAssertEqualObjects([db lastError].localizedDescription, @"cannot start a transaction within a transaction");
 }
 }
 
 
+- (void)testOpenFailure {
+    NSURL *tempURL = [NSURL fileURLWithPath:NSTemporaryDirectory()];
+    NSURL *fileURL1 = [tempURL URLByAppendingPathComponent:[[NSUUID UUID] UUIDString]];
+    NSURL *fileURL2 = [tempURL URLByAppendingPathComponent:[[NSUUID UUID] UUIDString]];
+    NSFileManager *manager = [NSFileManager defaultManager];
+    
+    // ok, first create one database
+    
+    FMDatabase *db = [FMDatabase databaseWithURL:fileURL1];
+    BOOL success = [db open];
+    XCTAssert(success, @"Database not created correctly for purposes of test");
+    success = [db executeUpdate:@"create table if not exists foo (bar text)"];
+    XCTAssert(success, @"Table created correctly for purposes of test");
+    [db close];
+    
+    // now, try to create open second database even though it doesn't exist
+    
+    db = [FMDatabase databaseWithURL:fileURL2];
+    success = [db openWithFlags:SQLITE_OPEN_READWRITE];
+    XCTAssert(!success, @"Opening second database file that doesn't exist should not have succeeded");
+    
+    // OK, everything so far is fine, opening a db without CREATE option above should have failed,
+    // but so fix the missing file issue and re-opening
+    
+    success = [manager copyItemAtURL:fileURL1 toURL:fileURL2 error:nil];
+    XCTAssert(success, @"Copying of db should have succeeded");
+    
+    // now let's try opening it again
+    
+    success = [db openWithFlags:SQLITE_OPEN_READWRITE];
+    XCTAssert(success, @"Opening second database should now succeed");
+
+    // now let's try using it
+    FMResultSet *rs = [db executeQuery:@"select * from foo"];
+    XCTAssertNotNil(rs, @"Should successfully be able to use re-opened database");
+    
+    // let's clean up
+    
+    [rs close];
+    [db close];
+    [manager removeItemAtURL:fileURL1 error:nil];
+    [manager removeItemAtURL:fileURL2 error:nil];
+}
+
 @end
 @end

+ 4 - 0
src/fmdb/FMDatabase.h

@@ -221,6 +221,10 @@ typedef NS_ENUM(int, FMDBCheckpointMode) {
 /// @name Opening and closing database
 /// @name Opening and closing database
 ///-----------------------------------
 ///-----------------------------------
 
 
+/// Is the database open or not?
+
+@property (nonatomic) BOOL isOpen;
+
 /** Opening a new database connection
 /** Opening a new database connection
  
  
  The database is opened for reading and writing, and is created if it does not already exist.
  The database is opened for reading and writing, and is created if it does not already exist.

+ 25 - 5
src/fmdb/FMDatabase.m

@@ -68,6 +68,7 @@ - (instancetype)initWithPath:(NSString *)path {
         _logsErrors                 = YES;
         _logsErrors                 = YES;
         _crashOnErrors              = NO;
         _crashOnErrors              = NO;
         _maxBusyRetryTimeInterval   = 2;
         _maxBusyRetryTimeInterval   = 2;
+        _isOpen                     = NO;
     }
     }
     
     
     return self;
     return self;
@@ -160,10 +161,18 @@ - (const char*)sqlitePath {
 #pragma mark Open and close database
 #pragma mark Open and close database
 
 
 - (BOOL)open {
 - (BOOL)open {
-    if (_db) {
+    if (_isOpen) {
         return YES;
         return YES;
     }
     }
     
     
+    // if we previously tried to open and it failed, make sure to close it before we try again
+    
+    if (_db) {
+        [self close];
+    }
+    
+    // now open database
+
     int err = sqlite3_open([self sqlitePath], (sqlite3**)&_db );
     int err = sqlite3_open([self sqlitePath], (sqlite3**)&_db );
     if(err != SQLITE_OK) {
     if(err != SQLITE_OK) {
         NSLog(@"error opening!: %d", err);
         NSLog(@"error opening!: %d", err);
@@ -175,6 +184,7 @@ - (BOOL)open {
         [self setMaxBusyRetryTimeInterval:_maxBusyRetryTimeInterval];
         [self setMaxBusyRetryTimeInterval:_maxBusyRetryTimeInterval];
     }
     }
     
     
+    _isOpen = YES;
     
     
     return YES;
     return YES;
 }
 }
@@ -182,12 +192,21 @@ - (BOOL)open {
 - (BOOL)openWithFlags:(int)flags {
 - (BOOL)openWithFlags:(int)flags {
     return [self openWithFlags:flags vfs:nil];
     return [self openWithFlags:flags vfs:nil];
 }
 }
+
 - (BOOL)openWithFlags:(int)flags vfs:(NSString *)vfsName {
 - (BOOL)openWithFlags:(int)flags vfs:(NSString *)vfsName {
 #if SQLITE_VERSION_NUMBER >= 3005000
 #if SQLITE_VERSION_NUMBER >= 3005000
-    if (_db) {
+    if (_isOpen) {
         return YES;
         return YES;
     }
     }
     
     
+    // if we previously tried to open and it failed, make sure to close it before we try again
+    
+    if (_db) {
+        [self close];
+    }
+    
+    // now open database
+    
     int err = sqlite3_open_v2([self sqlitePath], (sqlite3**)&_db, flags, [vfsName UTF8String]);
     int err = sqlite3_open_v2([self sqlitePath], (sqlite3**)&_db, flags, [vfsName UTF8String]);
     if(err != SQLITE_OK) {
     if(err != SQLITE_OK) {
         NSLog(@"error opening!: %d", err);
         NSLog(@"error opening!: %d", err);
@@ -199,6 +218,8 @@ - (BOOL)openWithFlags:(int)flags vfs:(NSString *)vfsName {
         [self setMaxBusyRetryTimeInterval:_maxBusyRetryTimeInterval];
         [self setMaxBusyRetryTimeInterval:_maxBusyRetryTimeInterval];
     }
     }
     
     
+    _isOpen = YES;
+    
     return YES;
     return YES;
 #else
 #else
     NSLog(@"openWithFlags requires SQLite 3.5");
     NSLog(@"openWithFlags requires SQLite 3.5");
@@ -206,7 +227,6 @@ - (BOOL)openWithFlags:(int)flags vfs:(NSString *)vfsName {
 #endif
 #endif
 }
 }
 
 
-
 - (BOOL)close {
 - (BOOL)close {
     
     
     [self clearCachedStatements];
     [self clearCachedStatements];
@@ -466,7 +486,7 @@ - (NSString *)stringFromDate:(NSDate *)date {
 
 
 - (BOOL)goodConnection {
 - (BOOL)goodConnection {
     
     
-    if (!_db) {
+    if (!_isOpen) {
         return NO;
         return NO;
     }
     }
     
     
@@ -493,7 +513,7 @@ - (void)warnInUse {
 
 
 - (BOOL)databaseExists {
 - (BOOL)databaseExists {
     
     
-    if (!_db) {
+    if (!_isOpen) {
         
         
         NSLog(@"The FMDatabase %@ is not open.", self);
         NSLog(@"The FMDatabase %@ is not open.", self);