Handling of numeric overflows (was: NSNumber.databaseValue crash (EXC_BAD_INSTRUCTION))

[swiftlyfalling]

Repro case:
let _ = NSNumber(unsignedLongLong: UInt64.max - 1).databaseValue

Crashes on line 28, in NSNumber.swift:

case "Q":
    return Int64(unsignedLongLongValue).databaseValue

Suggestion:
Modify to include a more informative error for the caller? Something like:

case "Q":
    guard unsignedLongLongValue <= UInt64(Int64.max) else { fatalError("DatabaseValueConvertible: Cannot store NSNumber unsignedLongLongValue - it exceeds Int64.max. Value: \(unsignedLongLongValue)") }
    return Int64(unsignedLongLongValue).databaseValue

(Unless you want to add special support for storing larger values. But I don't think it's unreasonable to match SQLite's limitation and just fail if the value is greater than Int64.max.)

[groue]

Hello @swiftlyfalling

I understand. Many error messages in GRDB come from my own frustration.

Now do we want to perform double checks in this particular case (one in GRDB, and the second in Swift runtime)? The stack trace clearly shows the NSNumber.databaseValue function, so at least we know when something has turned wrong.

But the information in our hands does not include the value of the NSNumber that did cause the error, and we may overlook the limits of SQLite. So we're left with EXC_BAD_INSTRUCTION, and a stack trace. GRDB is certainly less tested than Swift and SQLite. It must be a GRDB bug.

Frankly, I don't know what to answer yet. I see this issue as a psychological one, not a technical one. Maybe you're right, we should cover our ass and perform double checks. Or see this issue as a UX issue, and jump straight to the double check without further hesitation, for the user's convenience.

Anyway, thanks for talking about this topic.

And now, some overflow fun.

Swift overflow errors are sometimes clear, sometimes not:

func int64(x: UInt64) -> Int64 { return Int64(x) }
// Unclear error: EXC_BAD_INSTRUCTION
int64(UInt64.max - 1)

func int64(x: Float) -> Int64 { return Int64(x) }
// Clear error: "floating point value cannot be converted to Int64 because it is greater than Int64.max"
int64(FLT_MAX)

Generally expect odd behaviors in GRDB regarding overflows. When you put SQLite itself in the story, things turn even more difficult to describe. Let's take Int, for example.

Int is interesting because it adopts StatementColumnConvertible, the protocol for optimized types that go as directly as possible to SQLite, and allow GRDB to be fast. In some situations, we'll see SQLite turn a big double into a big int:

// Calls Int.init(sqliteStatement:index:), which does Int(sqlite3_column_int64(...))
// -> 9223372036854775807 (0x7FFFFFFFFFFFFFFF) on 64-bits platforms
// -> Crash on 32-bits platforms
Int.fetchOne(db, "SELECT ?", arguments: [FLT_MAX])
for row in Row.fetch(db, "SELECT ?", arguments: [FLT_MAX]) {
    row.value(atIndex: 0) as Int
}

... But not always. When you deal with rows that have been copied from the database, we have copied DatabaseValues that contain raw SQLite values. And DatabaseValue does not implement the same conversions as SQLite. It happily turns 1.0 (float) to 1 (int), but it won't turn FLT_MAX to a big Int:

// Calls Int.fromDatabaseValue(), which does Int(FLT_MAX)
// -> Crash on all platforms
let row = Row.fetchOne(db, "SELECT ?", arguments: [FLT_MAX])!
row.value(atIndex: 0) as Int

This is raw information :-) Your comments are welcome.

[groue]

More thoughts... Quoting https://github.com/groue/GRDB.swift#databasevalue :

Invalid conversions from non-NULL values raise a fatal error. This fatal error can be avoided with the DatabaseValueConvertible.fromDatabaseValue() method.

And indeed I want Type.fromDatabaseValue(...) to be the way to avoid crashes when one can't trust the database values (I expect most users to know what is stored in their database - maybe I'm a fool).

This means that Int.fromDatabaseValue(FLT_MAX.databaseValue) should return nil. This is not the case today. There's room for improvements here.

[swiftlyfalling]

Frankly, I don't know what to answer yet. I see this issue as a psychological one, not a technical one. Maybe you're right, we should cover our ass and perform double checks. Or see this issue as a UX issue, and jump straight to the double check without further hesitation, for the user's convenience.

I wasn't sure either. I'm glad it's sparked this discussion!

NSNumber is a bit of an edge case because we can't handle it through protocol conformance.

We can prevent the user from trying to automatically persist UInt64 by not extending it to conform to DatabaseValueConvertible, but we can't do the same for NSNumber. Most will be fine - unless the internal value is larger than Int64.max.

I figured since you already have an explicit failure case in NSNumber.databaseValue, it might make sense to add another. Or, at least, add information to the documentation about NSNumber.databaseValue failing if the NSNumber is > Int64.max.

And indeed I want Type.fromDatabaseValue(...) to be the way to avoid crashes when one can't trust the database values

That makes sense to me.

This means that Int.fromDatabaseValue(FLT_MAX.databaseValue) should return nil. This is not the case today. There's room for improvements here.

I absolutely agree that handling it from that end makes sense.

[groue]

NSNumber is a bit of an edge case because we can't handle it through protocol conformance.

We can prevent the user from trying to automatically persist UInt64 by not extending it to conform to DatabaseValueConvertible, but we can't do the same for NSNumber. Most will be fine - unless the internal value is larger than Int64.max.

True. You know, if today only Int, Int32, and Int64 adopt DatabaseValueConvertible, it's sheer laziness: there is no reason preventing Int8, Int16, unsigned types, and even UInt64 to be directly stored and fetched. I just wrote the code for the types I stumbled upon :-)

I figured since you already have an explicit failure case in NSNumber.databaseValue, it might make sense to add another. Or, at least, add information to the documentation about NSNumber.databaseValue failing if the NSNumber is > Int64.max.

NSNumber documentation is a good idea.

And regarding the main topic of this issue: I'm starting to err on the side of the nice option of the explicit overflow messages. They will bubble up from crash reporting tools like Xcode and Crashlytics, and will help figuring out issues in deployed applications. Without those messages, those bugs would be much harder to decipher.

This means that Int.fromDatabaseValue(FLT_MAX.databaseValue) should return nil. This is not the case today. There's room for improvements here.

I absolutely agree that handling it from that end makes sense.

I'll take care of that.

[groue]

@swiftlyfalling, when a numeric overflow is noticed, it is now handled with an explicit error message that contains the faulty value. Crash logs exonerate GRDB, and users have enough information to jump straigh to their issue.

Precisely speaking:

• All integer types but Int64 trap on overflow (this is consistent with the general handling of conversions in Swift, and I'm happy with that).
• Int64 can miss overflows: you'll sometimes get Int64.max when you load a big double value. Well, one rarely stores doubles to load integers afterwards - we can live with this SQLite singularity.

For example, Int32 won't load from big values:

let row = Row.fetchOne(db, "SELECT ?", arguments: [Int64.max])!
// nil
Int32.fromDatabaseValue(row.databaseValue(atIndex: 0))
// fatal error: could not convert database value 9223372036854775807 to Int32
row.value(atIndex: 0) as Int32

NSNumber won't store big values:

// fatal error: could not convert 18446744073709551615 to an Int64 that can be stored in the database
Row.fetchOne(db, "SELECT ?", arguments: [NSNumber(unsignedLongLong: UInt64.max)])

An example of missed Int64 overflow:

for row in Row.fetch(db, "SELECT ?", arguments: [DBL_MAX]) {
    // 9223372036854775807 (SQLite has converted DBL_MAX to Int64.max)
    row.value(atIndex: 0) as Int64
}

[groue]

The fixes have shipped in v0.71.0