Remove try! statement from FailableIterator

[ethansinjin]

My team has recently experienced crashes of an app in production related to SQLite.swift. Upon further review, we discovered that these crashes are occurring due to this try! encountering a throw from failableNext().

This PR resolves the issue by returning nil if a throw is encountered.

Here is the entire thread that crashes. Note that it occurs when using the Apollo-iOS framework, found here:

Thread 7 name:
Thread 7 Crashed:
0   libswiftCore.dylib            	0x000000010117f968 0x100fa0000 + 1964392
1   libswiftCore.dylib            	0x000000010117f968 0x100fa0000 + 1964392
2   libswiftCore.dylib            	0x000000010102f994 0x100fa0000 + 588180
3   SQLite                        	0x0000000100d7e80c _T06SQLite16FailableIteratorPAAE4next7ElementQzSgyFAA9StatementC_Tg5 + 212 (Statement.swift:211)
4   SQLite                        	0x0000000100d674c8 _T06SQLite10ConnectionC7prepares11AnySequenceVyAA3RowVGAA9QueryType_pKFs0D8IteratorVyAHGycfU_AHSgycfU_TA + 76 (Query.swift:927)
5   SQLite                        	0x0000000100d67578 _T06SQLite3RowVSgIxo_ADIxr_TRTA + 48 (Query.swift:0)
6   libswiftCore.dylib            	0x000000010114e9bc 0x100fa0000 + 1763772
7   libswiftCore.dylib            	0x000000010114e9fc 0x100fa0000 + 1763836
8   libswiftCore.dylib            	0x000000010114ead0 0x100fa0000 + 1764048
9   libswiftCore.dylib            	0x000000010102a8c8 0x100fa0000 + 567496
10  libswiftCore.dylib            	0x000000010114e16c 0x100fa0000 + 1761644
11  libswiftCore.dylib            	0x000000010118fd78 0x100fa0000 + 2030968
12  libswiftCore.dylib            	0x0000000100fa8774 0x100fa0000 + 34676
13  libswiftCore.dylib            	0x000000010115f5bc 0x100fa0000 + 1832380
14  libswiftCore.dylib            	0x000000010114f4fc 0x100fa0000 + 1766652
15  Apollo                        	0x0000000100a01c3c _T06Apollo21SQLiteNormalizedCacheC13selectRecords33_0382DE2BE0B0836588D3F2CA65C602E7LLSayAA6RecordVGSaySSG7forKeys_tKFTf4gn_n + 1284 (SQLiteNormalizedCache.swift:0)
16  Apollo                        	0x0000000100a02710 _T06Apollo21SQLiteNormalizedCacheC12mergeRecords33_0382DE2BE0B0836588D3F2CA65C602E7LLs3SetVySSGAA06RecordO0V7records_tKFTf4gn_n + 116 (SQLiteNormalizedCache.swift:0)
17  Apollo                        	0x0000000100a031c8 _T06Apollo21SQLiteNormalizedCacheC5mergeAA7PromiseCys3SetVySSGGAA06RecordG0V7records_tFTf4gn_n + 304 (SQLiteNormalizedCache.swift:0)
18  Apollo                        	0x00000001009ff290 _T06Apollo21SQLiteNormalizedCacheCAA0cD0A2aDP5mergeAA7PromiseCys3SetVySSGGAA06RecordG0V7records_tFTW + 44 (SQLiteNormalizedCache.swift:0)
19  Apollo                        	0x000000010099d1ec _T06Apollo0A5StoreC7publishAA7PromiseCyytGAA9RecordSetV7records_SvSg7contexttFyyyc_ys5Error_pctcfU_yycfU_Tf4ggng_n + 164 (ApolloStore.swift:52)
20  Apollo                        	0x000000010099f788 _T06Apollo0A5StoreC7publishAA7PromiseCyytGAA9RecordSetV7records_SvSg7contexttFyyyc_ys5Error_pctcfU_yycfU_TA + 104 (ApolloStore.swift:0)
21  Apollo                        	0x00000001009c0f1c _T0Ix_IyB_TR + 36 (GraphQLQueryWatcher.swift:0)
22  libdispatch.dylib             	0x00000001812b2a54 _dispatch_call_block_and_release + 24 (init.c:994)
23  libdispatch.dylib             	0x00000001812b2a14 _dispatch_client_callout + 16 (object.m:502)
24  libdispatch.dylib             	0x00000001812c1540 _dispatch_queue_concurrent_drain + 540 (inline_internal.h:2500)
25  libdispatch.dylib             	0x00000001812bd304 _dispatch_queue_invoke$VARIANT$mp + 348 (queue.c:5304)
26  libdispatch.dylib             	0x00000001812bfcf4 _dispatch_root_queue_drain + 600 (inline_internal.h:2539)
27  libdispatch.dylib             	0x00000001812bfa38 _dispatch_worker_thread3 + 120 (queue.c:6104)
28  libsystem_pthread.dylib       	0x000000018155b06c _pthread_wqthread + 1268 (pthread.c:2286)
29  libsystem_pthread.dylib       	0x000000018155ab6c start_wqthread + 4

[ethansinjin]

@stephencelis any chance this could get a review/merge soon?

[stephencelis]

Hi @ethansinjin! Thanks for the submission! I've handed off maintenance, but hopefully @jberkel could give it a quick look to make sure it doesn't break anything?

[jberkel]

@ethansinjin it's called FailableIterator for a reason, mapping errors to nil would be obscuring the actual error (and lose the distinction between error case and no more elements).

If I remember correctly the problem is that Swift's iterator protocol doesn't assume that the iteration can produce errors. Statement has a failableNext() throws -> [Binding?]? which could perhaps be used instead.

What's the error thrown in your case?

[ethansinjin]

@jberkel: EXC_BAD_INSTRUCTION fatal error: unexpectedly found nil while unwrapping an Optional value

I see what you mean; perhaps returning nil isn't the "correct" thing to do, but using try! will cause crashes, so I'm happy to modify the PR or hand off to you if you'd like. Since this is causing crashes in an App Store app, I'd love to see this fixed!

[jberkel]

@ethansinjin unwrapping an optional value? that sounds like it's not the immediate cause. would expect to see some error thrown from within SQLite.

[ethansinjin]

My apologies, I don't think that's the actual crash.
It looks like the following function is the root cause of the crash:

Query.swift line 927:
_T06SQLite10ConnectionC7prepares11AnySequenceVyAA3RowVGAA9QueryType_pKFs0D8IteratorVyAHGycfU_AHSgycfU_TA + 76 (Query.swift:927)

public func prepare(_ query: QueryType) throws -> AnySequence<Row> {
        let expression = query.expression
        let statement = try prepare(expression.template, expression.bindings)

        let columnNames = try columnNamesForQuery(query)

        return AnySequence {
            AnyIterator { statement.next().map { Row(columnNames, $0) } }
        }
    }

Thich calls statement.next().map, Statement.swift lines 209 to 213 will then be relevant:
_T06SQLite16FailableIteratorPAAE4next7ElementQzSgyFAA9StatementC_Tg5 + 212 (Statement.swift:211)

extension FailableIterator {
    public func next() -> Element? {
        return try! failableNext()
    }
}

Because next() receives a throw, it throws. But, since it is a try!, this triggers a crash.

The Swift Programming Language Docs section on Error Handling, specifically "Disabling Error Propagation":

Sometimes you know a throwing function or method won’t, in fact, throw an error at runtime. On those occasions, you can write try! before the expression to disable error propagation and wrap the call in a runtime assertion that no error will be thrown. If an error actually is thrown, you’ll get a runtime error.

Please let me know if there's any more information I can provide!

Also of note is the Statement.swift lines 185 to 187, which are the next step up the call stack:
_T06SQLite3RowVSgIxo_ADIxr_TRTA + 48

    public func step() throws -> Bool {
        return try connection.sync { try self.connection.check(sqlite3_step(self.handle)) == SQLITE_ROW }
    }

I think this may have something to do with the initial crash. I think the throw from step() is being passed up to the try!

[jberkel]

Have a look at the check function (Connection:642)

func check(_ resultCode: Int32, statement: Statement? = nil) throws -> Int32 {
  guard let error = Result(errorCode: resultCode, connection: self, statement: statement) else {
    return resultCode
  }
  throw error
}

It's very likely that the error gets thrown from there, which means that SQLite returned an error code. You should see this error somewhere in the log output. If not, try to put a breakpoint at the line where the error is thrown and run it in the debugger.

[erichoracek]

We've run into this crash as well when sqlite3_step() encounters an error while iterating rows.

From my understanding, and as @ethansinjin discusses above, the prepare function returns a sequence that is effectively an AnyIterator { try! sqlite3_step() }. This means that any failure that occurs during iteration will crash your application, e.g. the table was deleted on one connection while iterating its rows on a separate connection. This is a very unexpected behavior in Swift, especially from a function that throws and isn't marked with Unsafe name or a ! operator.

After looking at the options for a fix, it seems like this is perhaps a deeper design issue. I see two options two prevent the crash:

• Return a Sequence that ends when an error is encountered (@ethansinjin's fix), which would be a nonbreaking change but would unexpectedly suppress the error that occurred.

• Adopt a construct like Cursor from GRDB that is effectively an iterator that can throw on each next(). There's a discussion here Expose reading errors groue/GRDB.swift#148 about this construct, with the following summary:

  Until now, GRDB would assume that both SQLite and the database file(s) were fundamentally trustable. Well, the file system is not trustable. Applications have to be able to deal with I/O errors.

  This would require all consumers to migrate to a different API, but would be more correct than the current implementation, in that it could produce an error during iteration, just as the sqlite3_step() does.

Let me know which option seems like the best next step, or if there's another alternative I'm not considering.

on an entirely separate note 👋 @ethansinjin hope things are well!

[hellohuanlin]

We have run into the same crash. Any chance that you can take a look? thanks! CC: @jberkel

[ghost]

Hey @jberkel, i am getting "Thread 1: Fatal error: 'try!' expression unexpectedly raised an error: disk I/O error (code: 10)" with

extension FailableIterator { public func next() -> Element? { return try! failableNext() } }

Do you think Is this pr resolves this issue or i should consider other solutions?

[jimisaacs]

Hello, I am having the same issue, and I've read this thread and I think I understand?

2 thoughts.

1. FailableIterator should be named to FatalIterator if this change or something like it doesn't go in. Unwrapping optionals is inherently "failable", but it's also not an exception. If the intent was to be failable yet catchable, it should not be unwrapping try!.

2. If this fix does not go in, I suggest this change instead, and the respective required changes to go with it.

extension FailableIterator {
    public func next() -> Element? throws {
        return try failableNext()
    }
}

Though the function signature is curious as it returns nullable. So in conclusion, what is "failable" in your definition here? I realize the proposal doesn't conform to IteratorProtocol, so I just need more context.

It should be noted that another library which uses this library, wraps it in a Promise API. And the way promises work, is that they catch exceptions from within the promise context. So in essence you use a promise to have safe error recoverable interface that is simply analogous to do {} catch {}. Though if something does not mark as throws, and it can fail, there is nothing a consumer API can do about that, but crash.

[jimisaacs]

How about this change?

extension Connection {

    public func prepare(_ query: QueryType) throws -> AnySequence<Row> {
        let expression = query.expression
        let statement = try prepare(expression.template, expression.bindings)

        let columnNames = try columnNamesForQuery(query)

        return AnySequence {
            AnyIterator { statement.next().map { Row(columnNames, $0) } }
        }
    }
    ...

to this:

extension Connection {

    public func prepare(_ query: QueryType) throws -> AnySequence<Row> {
        let expression = query.expression
        let statement = try prepare(expression.template, expression.bindings)

        let columnNames = try columnNamesForQuery(query)

        return try AnySequence {
            AnyIterator { statement.failableNext().map { Row(columnNames, $0) } }
        }
    }

[jimisaacs]

Actually, about this the previous change I just mentioned, and the change for this PR?

The reason behind this is because now that I see the use case of FailableIterator, it is to provide failableNext(). We just need to use that where appropriate, and not crash where it isn't appropriate and next() is used.

[0xced]

You could simply write return try? failableNext() instead of return try! failableNext(). There's no need to use if let here.

[gsabran]

No this raises Value of optional type 'Self.Element??' not unwrapped; did you mean to use 'try!' or chain with '?'?

[eriksenfredrik]

Hi,

We are experiencing the same issues. Will this patch be merged into master? We need to find a solution that will last, seen many people implement this change locally but we are keen on not branching out this library for future compatibility

Thanks!

@ethansinjin @jberkel

[georgbachmann]

I'd also like to know if this is going to be merged? Cause I am seeing the same issue in my crashreports. Can't reproduce it locally though..

[gsabran]

FWIW my company ended up forking this repo as it doesn't seem to be actively maintained anymore. We did this mainly to just pull the change in this PR.

[nathanfallet]

Closing as this was fixed by another PR (replacing the ! by a ?)

[kylebrowning]

@nathanfallet which PR?

[groue]

@kylebrowning it was #1030, which was later reverted in #1075.

[kylebrowning]

So its not fixed?

[groue]

See #1030 if you need more details